[Wasm RyuJit] More crossgen assert fixes by AndyAyersMS · Pull Request #125102 · dotnet/runtime

AndyAyersMS · 2026-03-03T04:40:05Z

use gtOverflowEx instead of gtOverflow
not all block stores need null checks
more cases where we don't have putarg stacks
add lowering was bypassing Wasm overflow checking

* use gtOverflowEx instead of gtOverflow * not all block stores need null checks * more cases where we don't have putarg stacks * add lowering was bypassing Wasm overflow checking

AndyAyersMS · 2026-03-03T04:40:52Z

We might be able to be even more fine-grained with the null checks.

Also not 100% sure yet what to do with all the operand reordering for calls, but this at least tolerates the way Wasm does things.

dotnet-policy-service · 2026-03-03T04:41:18Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Copilot

Pull request overview

This PR fixes several assert failures in the WASM RyuJIT cross-compilation codepath ("crossgen assert fixes"):

Changes:

Replace incorrect gtOverflow() calls with gtOverflowEx() in WASM-specific lowering and codegen, since gtOverflow() asserts OperMayOverflow() and these sites can be reached with non-overflow-capable operators (AND, OR, XOR).
Fix overflow checking for GT_ADD on WASM: LowerAdd was not calling LowerBinaryArithmetic, so the SetMultiplyUsed() calls needed to set up overflow-checking registers were skipped for overflow ADDs.
Guard null checks in genCodeForCpObj behind a doNullChecks flag (GTF_EXCEPT && OperMayThrow) since not all block-copy operations need null checks.
Fix asserts in MovePutArgUpToCall/MovePutArgNodesUpToCall that assumed nodes must be GT_PUTARG_* or GT_FIELD_LIST, which is not true on WASM where HAS_FIXED_REGISTER_SET = 0.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`src/coreclr/jit/lowerwasm.cpp`	Use `gtOverflowEx()` instead of `gtOverflow()` in `LowerBinaryArithmetic` for correctness when called with non-overflow-capable operators (AND/OR/XOR)
`src/coreclr/jit/lower.cpp`	Fix `MovePutArgUpToCall`/`MovePutArgNodesUpToCall` asserts for WASM (no PutArg nodes); add new `else` branch for plain arg nodes; call `LowerBinaryArithmetic` from `LowerAdd` on WASM to fix missing overflow setup for ADD
`src/coreclr/jit/codegenwasm.cpp`	Use `gtOverflowEx()` instead of `gtOverflow()` in `genCodeForBinary`; gate null checks in `genCodeForCpObj` behind `doNullChecks` for correctness when copies are known safe

Copilot · 2026-03-03T04:49:17Z

src/coreclr/jit/lower.cpp

+    else
+    {
+        GenTree* operand = node;
+        JITDUMP("Checking if we can move node:\n");
+        DISPTREE(operand);
+        if (((operand->gtFlags & GTF_ALL_EFFECT) == 0) && IsCFGCallArgInvariantInRange(operand, call))
+        {
+            JITDUMP("...yes, moving to after validator call\n");
+            BlockRange().Remove(operand);
+            BlockRange().InsertBefore(call, operand);
+        }
+        else
+        {
+            JITDUMP("...no, operand has side effects or is not invariant\n");
+        }
+    }


In the new else branch (for WASM where node is neither a PutArg nor a FieldList), the variable operand is set to node itself. This means the inner move in lines 4012–4013 (BlockRange().Remove(operand); BlockRange().InsertBefore(call, operand)) moves the same node that the unconditional code at lines 4024–4025 also moves. As a result, when the invariance check succeeds, the node is removed and re-inserted before call twice — which is redundant. The inner if/else block in this else branch serves no useful purpose (there is no separate child operand to optionally pre-move), and could be removed entirely, leaving only the unconditional lines 4024–4025 to move node.

Suggested change

else

{

GenTree* operand = node;

JITDUMP("Checking if we can move node:\n");

DISPTREE(operand);

if (((operand->gtFlags & GTF_ALL_EFFECT) == 0) && IsCFGCallArgInvariantInRange(operand, call))

{

JITDUMP("...yes, moving to after validator call\n");

BlockRange().Remove(operand);

BlockRange().InsertBefore(call, operand);

}

else

{

JITDUMP("...no, operand has side effects or is not invariant\n");

}

}

Copilot · 2026-03-03T04:49:17Z

src/coreclr/jit/lower.cpp

 //------------------------------------------------------------------------
 // MovePutArgUpToCall: Given a call that will be transformed using the
 // CFG validate+call or similar scheme, and an argument GT_PUTARG_* or GT_FIELD_LIST node,
 // move that node right before the call.
 //
 // Arguments:
 //    call - The call that is being transformed
 //    node - The argument node
 //
 // Remarks:
 //    We can always move the GT_PUTARG_* node further ahead as the side-effects
 //    of these nodes are handled by LSRA. However, the operands of these nodes
 //    are not always safe to move further ahead; for invariant operands, we
 //    move them ahead as well to shorten the lifetime of these values.
 //
 void Lowering::MovePutArgUpToCall(GenTreeCall* call, GenTree* node)
 {
-    assert(node->OperIsPutArg() || node->OperIsFieldList());
+    assert(!HAS_FIXED_REGISTER_SET || node->OperIsPutArg() || node->OperIsFieldList());


The function comment at lines 3961–3962 still says "GT_PUTARG_* or GT_FIELD_LIST node", but with the new else branch, this function can now be called on WASM with arbitrary argument nodes (not wrapped in a PutArg or FieldList). Similarly, the Remarks section at line 3970 mentions "the GT_PUTARG_* node" exclusively. These comments should be updated to reflect the new WASM behavior.

jakobbotsch · 2026-03-03T06:54:19Z

src/coreclr/jit/lower.cpp

+        {
+            JITDUMP("...no, operand has side effects or is not invariant\n");
+        }
+    }


Why are we calling this function without PUTARG nodes to move? That doesn't seem right. Should these calls be skipped instead?

IMO it feels like wasm should almost have its own call lowering given how different it is in this area. Or it should be switched to have PUTARG nodes to be less different.

MovePutArgUpToCall should return without doing anything for non-putarg nodes under !HAS_FIXED_REGISTER_SET. There is no need to do anything with the args.

That doesn't seem right. Should these calls be skipped instead?

That could be done (return early from MovePutArgUpToCall), but it would bake-in the assumption that !HAS_FIXED_REGISTER_SET / TARGET_WASM implies no PUTARG_STK, which seems unnecessary.

SingleAccretion · 2026-03-03T12:14:07Z

src/coreclr/jit/codegenwasm.cpp

+    var_types srcAddrType  = TYP_BYREF;
+    regNumber dstReg       = GetMultiUseOperandReg(dstAddr);
+    unsigned  dstOffset    = 0;
+    bool      doNullChecks = ((cpObjNode->gtFlags & GTF_EXCEPT) != 0) && cpObjNode->OperMayThrow(m_compiler);


This should be based on gtFlags & GTF_IND_NONFAULTING of the individual indirection nodes. cpObjNode throwing does not imply the source IND throwing and vice-versa.

So, seems like genCodeForCpObj should actually be calling fgAddrCouldBeNull here (and similar logic in stacklevelsetter).

And yes null checks for GT_IND are also needed; I suppose I can try and add those now too.

fgAddrCouldBeNull

I don't think that's needed. All indirections that have fgAddrCouldBeNull == false should be marked non-faulting at this point.

(and similar logic in stacklevelsetter).

Yep, these two places need to be in sync.

And yes null checks for GT_IND are also needed; I suppose I can try and add those now too.

What I meant is that there needs to be a separate check for the contained IND that induces the null on the source, which is separate from the null check on the destination.

[Wasm RyuJit] More crossgen assert fixes

5f11a79

* use gtOverflowEx instead of gtOverflow * not all block stores need null checks * more cases where we don't have putarg stacks * add lowering was bypassing Wasm overflow checking

Copilot AI review requested due to automatic review settings March 3, 2026 04:40

github-actions bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Mar 3, 2026

dotnet-policy-service bot assigned AndyAyersMS Mar 3, 2026

Copilot started reviewing on behalf of AndyAyersMS March 3, 2026 04:40 View session

Copilot AI reviewed Mar 3, 2026

View reviewed changes

jakobbotsch reviewed Mar 3, 2026

View reviewed changes

SingleAccretion reviewed Mar 3, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Wasm RyuJit] More crossgen assert fixes#125102

[Wasm RyuJit] More crossgen assert fixes#125102
AndyAyersMS wants to merge 1 commit intodotnet:mainfrom
AndyAyersMS:WasmFixes5

AndyAyersMS commented Mar 3, 2026

Uh oh!

AndyAyersMS commented Mar 3, 2026

Uh oh!

dotnet-policy-service bot commented Mar 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

Copilot AI Mar 3, 2026

Uh oh!

jakobbotsch Mar 3, 2026 •

edited

Loading

Uh oh!

SingleAccretion Mar 3, 2026

Uh oh!

SingleAccretion Mar 3, 2026

Uh oh!

SingleAccretion Mar 3, 2026 •

edited

Loading

Uh oh!

AndyAyersMS Mar 3, 2026

Uh oh!

SingleAccretion Mar 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

AndyAyersMS commented Mar 3, 2026

Uh oh!

AndyAyersMS commented Mar 3, 2026

Uh oh!

dotnet-policy-service bot commented Mar 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Mar 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Mar 3, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jakobbotsch Mar 3, 2026 •

edited

Loading

SingleAccretion Mar 3, 2026 •

edited

Loading