[Wasm RyuJIT] Fix for assert in obscure block store scenario#126392
[Wasm RyuJIT] Fix for assert in obscure block store scenario#126392kg wants to merge 2 commits intodotnet:mainfrom
Conversation
kg
added
arch-wasm
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Fixes a WASM RyuJIT assertion/regalloc issue in an edge-case block-store lowering/codegen path by avoiding creation/consumption of temporary regs when using native memory.copy/memory.fill and the corresponding address is marked non-faulting.
Changes:
- Adjusted WASM lowering (
LowerBlockStore) to only mark src/dst addresses as multiply-used when null-checking (or non-native lowering) requires it. - Updated WASM codegen for
GT_STORE_BLKto skipGetMultiUseOperandRegin native-op + non-faulting cases, and tightened an invariant with a debug assert.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/coreclr/jit/lowerwasm.cpp |
Refines when block-store src/dst addresses are marked multiply-used to avoid unnecessary temps for non-faulting native ops. |
src/coreclr/jit/codegenwasm.cpp |
Avoids fetching temp regs for native memory.copy/fill when no null check is needed; adds an assert guarding a contained-src path. |
|
@dotnet/jit-contrib PTAL |
| // For native opcode copies/fills with a non-faulting destination, we can't reliably mark the address as | ||
| // multiply-used - it will cause failures later in compilation. So avoid those cases too. | ||
| ((blkNode->gtBlkOpKind != GenTreeBlk::BlkOpKindNativeOpcode) || | ||
| ((blkNode->gtFlags & GTF_IND_NONFAULTING) == 0))) |
There was a problem hiding this comment.
LowerBlockStore’s destination MultiplyUsed decision only checks GTF_IND_NONFAULTING, but codegen’s nullCheckDest also depends on dstOnStack = blkOp->IsAddressNotOnHeap(...). For native memory.copy/fill, destinations that are stack-only / GTF_IND_TGT_NOT_HEAP can still have GTF_IND_NONFAULTING cleared, so lowering may mark dstAddr multiply-used while codegen won’t ever request/consume a multi-use dest reg. Consider mirroring codegen’s condition here (e.g., include !blkNode->IsAddressNotOnHeap(m_compiler)), so lowering/codegen stay in sync and you don’t reintroduce the temp-reg ordering failures this PR is addressing.
| ((blkNode->gtFlags & GTF_IND_NONFAULTING) == 0))) | |
| ((blkNode->gtFlags & GTF_IND_NONFAULTING) == 0)) && | |
| // We also don't nullcheck destinations that are known not to reside on the heap (e.g. stack-only), so avoid | |
| // marking those addresses as multiply-used to keep lowering and codegen behavior in sync. | |
| !blkNode->IsAddressNotOnHeap(m_compiler)) |
| // skip generating the temporary for that case. If we don't do this, we get an error during regalloc caused | ||
| // by RewriteLocalStackStore creating and lowering a block store too 'late' to mark the dstAddr as multiply | ||
| // used successfully. | ||
| !dstAddr->OperIs(GT_LCL_ADDR) && |
There was a problem hiding this comment.
Remove this condition? It is now subsumed by the nonfaulting check.
| // For native opcode copies/fills with a non-faulting destination, we can't reliably mark the address as | ||
| // multiply-used - it will cause failures later in compilation. So avoid those cases too. |
There was a problem hiding this comment.
we can't reliably mark the address as multiply-used
What do you mean here? The same RewriteLocalStackStore restriction?
There was a problem hiding this comment.
I never figured out specifically what was going wrong, the jitdump was enormous.
This fixes the assert about temporary regs from
System.ValueTuple`4[System.__Canon,System.__Canon,System.__Canon,System.__Canon]:System.IComparable.CompareTo(System.Object):int:thisin #125756