Repository navigation
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 5 pipeline(s). 11 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. |
| assert(physReg != nullptr); | ||
| assert(physReg->OperIs(GT_PHYSREG)); | ||
| assert(physReg->AsPhysReg()->gtSrcReg == m_perFuncletData[m_currentFunclet]->m_spReg); | ||
| assert(physReg->OperIs(GT_PHYSREG, GT_LCL_VAR)); | ||
| assert(!physReg->OperIs(GT_PHYSREG) || | ||
| (physReg->AsPhysReg()->gtSrcReg == m_perFuncletData[m_currentFunclet]->m_spReg)); |
There was a problem hiding this comment.
I think these asserts should just be deleted
This branch has not been deployed
2 participants
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 accepts
GT_LCL_VARas well asGT_PHYSREGand only checksgtSrcRegfor aGT_PHYSREG.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.Resolves #135292
Note
This PR description was generated with GitHub Copilot.