[RyuJIT Wasm] SIMD Codegen for ExtractScalar/ReplaceScalar, Unsigned Compares#130272
[RyuJIT Wasm] SIMD Codegen for ExtractScalar/ReplaceScalar, Unsigned Compares#130272adamperlin wants to merge 22 commits into
Conversation
…asm; recursively lower newly introduced nodes in LowerCompareUnsignedLong
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR extends the WASM RyuJIT SIMD pipeline by enabling table-driven codegen for additional PackedSimd intrinsics (notably ExtractScalar/ReplaceScalar), adding lowering support for Vector128<ulong> ordered compares via a signed-compare rewrite, and introducing a non-constant-immediate fallback path using a structured-control-flow “jump table” emission pattern.
Changes:
- Add lowering rewrite for
Vector128<ulong>ordered compares by XOR’ing the sign bit and switching to signed compares. - Implement immediate-operand handling for WASM SIMD: containment for constant immediates and a jump-table fallback for non-constant immediates (plus RA support).
- Enable/route additional WASM SIMD intrinsics through the table-driven pipeline and update lane-emission plumbing.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/targetwasm.cpp | Adjusts ABI classification to treat TYP_SIMD16 similarly to structs for WASM ABI lowering decisions. |
| src/coreclr/jit/regallocwasm.h | Adds a regalloc hook to handle HWIntrinsic nodes needing special multi-use handling. |
| src/coreclr/jit/regallocwasm.cpp | Consumes temp regs for HWIntrinsic operands when jump-table fallback is required. |
| src/coreclr/jit/lowerwasm.cpp | Adds immediate handling for HW intrinsics and a rewrite for unsigned i64x2 ordered compares. |
| src/coreclr/jit/lower.h | Declares a WASM-specific lowering helper for unsigned-long SIMD compares. |
| src/coreclr/jit/hwintrinsicwasm.cpp | Implements WASM immediate upper-bound computation and improves immediate operand discovery. |
| src/coreclr/jit/hwintrinsiclistwasm.h | Updates intrinsic table entries to enable more PackedSimd ops (ordered compares, extract/replace lane, shifts, splat). |
| src/coreclr/jit/hwintrinsiccodegenwasm.cpp | Adds codegen support for HW_Category_IMM, including a jump-table fallback for non-constant immediates. |
| src/coreclr/jit/hwintrinsic.h | Extends WASM HWIntrinsic info with immediate-position helpers and jump-table-fallback querying. |
| src/coreclr/jit/hwintrinsic.cpp | Updates the WASM callsite to lookupImmUpperBound with the new signature. |
| src/coreclr/jit/gentree.h | Introduces a WASM-only HWIntrinsic flag for “needs jump table fallback”. |
| src/coreclr/jit/emitwasm.h | Updates the emitIns_Lane signature to derive element size from the instruction. |
| src/coreclr/jit/emitwasm.cpp | Implements the new emitIns_Lane behavior and makes jump debug-info tolerant of null targets. |
| src/coreclr/jit/compiler.h | Makes Compiler::getSIMDVectorLength(simdSize, baseType) publicly callable for WASM intrinsic helpers. |
| src/coreclr/jit/codegenwasm.cpp | Adjusts range-check throw kind usage; adds helper block emit APIs; updates SIMD emitter unit tests; tweaks SIMD16 call-arg guard. |
| src/coreclr/jit/codegen.h | Declares new WASM helper APIs for emitting raw block/end pairs and HWIntrinsic jump-table fallback. |
| GTF_HW_EM_OP = 0x10000000, // GT_HWINTRINSIC -- node is used as an operand to an embedded mask | ||
| GTF_HW_USER_CALL = 0x20000000, // GT_HWINTRINSIC -- node is implemented via a user call | ||
| #ifdef TARGET_WASM | ||
| GTF_HW_NEEDS_JUMP_TABLE = 0x40000000, // GT_HWINTRINSIC -- (WASM) intrinsic has a non-constant immediate |
There was a problem hiding this comment.
This is really a small optimization to avoid having to re-check the operands for a GT_HWINTRINSIC over and over again to see if a jump table is needed (this mostly comes up in regalloc, where we only need to mark the operands for an intrinsic as MultiplyUsed if it needs a jump table)
There was a problem hiding this comment.
Is it expensive to do for wasm?
On most platforms this is just checking if (lastOp->IsCnsIntOrI()) and we already have the last op, so it avoids needing to burn any flag.
There was a problem hiding this comment.
The immediate value isn't always lastOp on Wasm, so I think we'd have to use HWIntrinsicInfo::GetImmOpsPositions or cache that information, I think.
There was a problem hiding this comment.
We have to pull all the operands anyways, so is that necessarily a bad thing?
The main concern I have is that the available GenTree flags are scarce. The need for a jump table is then expected to be rare and such nodes are rare enough that it's unlikely to even show up on the SPMI throughput tracking, so just doing this the "same way" as all other architectures (i.e. checking immOp->IsCnsIntOrI()) feels "best"
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…-simd-unsigned-compare-lane-ops
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/coreclr/jit/hwintrinsic.cpp:2329
- Non-constant lane immediates for Wasm
HW_Category_IMMintrinsics still won't get anArgumentOutOfRangeExceptionrange-check:addRangeCheckIfNeededis only applied in thenumArgs == 2(op2) andnumArgs == 4(op4) cases, butPackedSimd.ReplaceScalar,PackedSimd.LoadScalarAndInsert, andPackedSimd.StoreSelectedScalarare 3-arg intrinsics. With the new jump-table fallback, an out-of-range lane can still fall into thebr_tabledefault and trap (unreachable) instead of throwingSCK_ARG_RNG_EXCPN.
To match the existing immediate-operand contract, the importer should apply addRangeCheckIfNeeded to whichever operand is the immediate for numArgs == 3 (using immOp1 discovered by getHWIntrinsicImmOps / GetImmOpsPositions, or by matching the popped operand pointer against immOp1).
immUpperBound = HWIntrinsicInfo::lookupImmUpperBound(intrinsic);
hasFullRangeImm = HWIntrinsicInfo::HasFullRangeImm(intrinsic);
#elif defined(TARGET_WASM)
immUpperBound = HWIntrinsicInfo::lookupImmUpperBound(intrinsic, simdSize, simdBaseType);
#endif
if (!CheckHWIntrinsicImmRange(intrinsic, simdBaseType, immOp1, mustExpand, immLowerBound, immUpperBound,
hasFullRangeImm, &useFallback))
…-simd-unsigned-compare-lane-ops
AndyAyersMS
left a comment
AndyAyersMS
left a comment
There was a problem hiding this comment.
Mostly nits on top of what copilot saw.
| } | ||
| #endif // TARGET_ARM64 | ||
| #elif defined(TARGET_WASM) | ||
| static void CheckImmOpSignature(NamedIntrinsic id, CORINFO_SIG_INFO* sig) |
There was a problem hiding this comment.
Nit: missing header comments (here and elsewhere)
…ub.com:adamperlin/runtime into adamperlin/wasm-simd-unsigned-compare-lane-ops
| // emit `immUpperBound+1` blocks. The iteration order of the loop doesn't matter here, | ||
| // we just need to stage the right number of blocks on the control flow stack. | ||
| for (int i = 0; i <= immUpperBound; i++) | ||
| { | ||
| // emit a block for each case | ||
| genEmitBeginBlock(); | ||
| } |
| // emit `immUpperBound+1` blocks. The iteration order of the loop doesn't matter here, | ||
| // we just need to stage the right number of blocks on the control flow stack. | ||
| for (int i = 0; i <= immUpperBound; i++) | ||
| { | ||
| // emit a block for each case | ||
| genEmitBeginBlock(); | ||
| } |
| if (type == TYP_SIMD16) | ||
| { | ||
| // Storing a SIMD16 value emits v128.store, but the data operand is not | ||
| // materialized as a v128 (it comes through as an i32), producing an invalid | ||
| // module. Bail until SIMD16 store is properly supported. | ||
| NYI_WASM_SIMD("SIMD16 store indirect"); | ||
| // NYI_WASM_SIMD("SIMD16 store indirect"); |
There was a problem hiding this comment.
Is this meant to be commented out?
| case NI_PackedSimd_CompareLessThan: | ||
| case NI_PackedSimd_CompareLessThanOrEqual: | ||
| case NI_PackedSimd_CompareGreaterThan: | ||
| case NI_PackedSimd_CompareGreaterThanOrEqual: | ||
| { | ||
| if (node->GetSimdBaseType() == TYP_ULONG) | ||
| { | ||
| return LowerHWIntrinsicCompareUnsignedLong(node); | ||
| } |
| uint8_t elemSize = CodeGenInterface::instSimdElemSize(ins); | ||
|
|
||
| // Add element width as an emit attribute | ||
| emitAttr attr = EA_UNKNOWN; |
There was a problem hiding this comment.
can this not just be EA_ATTR(size) or similar?
This PR implements Wasm codegen for ExtractScalar and ReplaceScalar, as well as enabling codegen for SIMD shifts. As an additional change, lowering for
ulongtype compares is implemented with a rewrite to an equivalent signed compare.ExtractScalar and ReplaceScalar require immediate operands, so we need to generate some kind of jump-table fallback for each possible immediate value in cases where we can't guarantee that the operand is an immediate. The approach I've taken here is to generate the table as a series of inline nested wasm blocks. This also means we need to mark the non-immediate operands as
MultiplyUsedand re-materialize the computed values on the wasm value stack in each case in the jump table (since nested blocks can't reach "up" the value stack, unless there are explicit block parameters).As an additional change, we treat SIMD types like structs for ABI handling purposes (before they were considered pass-by-value types which conflicted with ABI classification elsewhere). We aren't planning to introduce by-value
v128in the ABI for .NET 11.