Repository navigation
[wasm][JIT] Remove assertions assuming the SP arg of a fast tail call is a GT_PHYSREG - #135293
Conversation
…erence When a call argument contains many stores (e.g. a long chain of calls sharing temps), gtMayHaveStoreInterference gives up and conservatively reports interference, which caused the wasm shadow stack pointer argument to be evaluated into a temp. For a fast tail call the register allocator expects that argument to be a GT_PHYSREG and asserted. The SP is never stored to, so skip it when marking earlier args as needing temps. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12580074-dce3-426d-bcc2-8c83aa5a4a7d
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Azure Pipelines: Successfully started running 6 pipeline(s). 10 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Why does it do that? There are many reasons operands of nodes can be evaluated into temporaries. We should not be making assumptions like this lightly. |
that is a question for @AndyAyersMS |
…e interference" The regalloc no longer assumes the SP arg is a GT_PHYSREG, so avoiding the spill is just an optimization and not needed for correctness. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12580074-dce3-426d-bcc2-8c83aa5a4a7d
The regalloc no longer assumes the SP arg is a GT_PHYSREG, so avoiding the spill is only an optimization and not needed for correctness. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12580074-dce3-426d-bcc2-8c83aa5a4a7d
lewing
changed the title
|
Agreed. I've updated the PR to drop the assumption: The temp holds the same post-prolog SP value, so Note This reply was generated with GitHub Copilot. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12580074-dce3-426d-bcc2-8c83aa5a4a7d
lewing
changed the title
Explicit `tail.` calls in wasm R2R code become `return_call`, but R2R↔interpreter transitions use regular calls, so frames accumulate and deep tail recursion across them overflows the stack. crossgen2 now rejects wasm methods that contain an explicit tail call (`RequiresRuntimeJitException` from `reportTailCallDecision`, R2R only) and leaves them to the interpreter, which handles tail calls. This re-enables `DevDiv_902271`, `more_tailcalls` and `mutual_recursion`. The cost is R2R coverage: about 11% of FSharp.Core methods (1,324 of ~12.1k) are now interpreted. A real fix needs tail-call-preserving transitions; a thunk-only change isn't enough because the thunk must stay on the stack to restore SP and load the return value. NativeAOT isn't covered by this bail-out; its related regalloc assert is tracked in #135292 and fixed in #135293. Resolves #133467 > [!NOTE] > This PR description was generated with GitHub Copilot. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 12580074-dce3-426d-bcc2-8c83aa5a4a7d
For a wasm fast tail call,
WasmRegAlloc::CollectReferencesForCallwraps the shadow stack pointer (SP) argument inADD(SP, FRAME_SIZE)so the callee sees the incoming SP. It asserted that the SP arg is aGT_PHYSREG, but an operand can be evaluated into a temp for several reasons. For example, a longsb.Append(..).Append(..)...ToString()chain makesgtMayHaveStoreInterferencegive up after 8 stores, so arg morphing spills the SP arg to a temp, and the assert fired (regallocwasm.cpp:589).The temp holds the same post-prolog SP value, so wrapping it is equally correct. This change deletes the asserts that assumed a
GT_PHYSREGSP arg, per review feedback.Validation: a checked wasm crossgen2 without the explicit-tail-call bail-out from #135291 now compiles all of FSharp.Core (exit 0); before it asserted on
FSharpList<int>.ToString()(exit 133). I read the generated code for that method: the temp is set from SP and the tail call passestemp + 192. No wasm tests were run.No regression test is included: reproducing needs IL with an explicit
tail.call and only fails under a checked wasm JIT without the crossgen2 bail-out, so wasm R2R CI would not exercise it.The branch history contains an earlier morph change and its revert; the net diff is only
regallocwasm.cpp(5 deletions).Resolves #135292
Note
This PR description was generated with GitHub Copilot.