JIT: Remove transparent scalar/vector conversion HWINTRINSIC nodes during lowering#130444
Conversation
Floating-point CreateScalarUnsafe is a pure reinterpret: the scalar value already resides in the lowest element of a SIMD register with the upper elements explicitly undefined. Rather than marking such nodes contained (which produces two-deep containment, genOperandDesc look-throughs, and special-cased size asserts), remove the node in LIR lowering and rewire its user to consume the underlying scalar directly. The scalar is intentionally left at its natural (scalar) type; retyping it would corrupt spill and memory-access sizes for other consumers. Correctness is preserved because a register consumer sees the full SIMD register and any memory-fold consumer is gated by the existing width-aware containment logic. Integral scalars (and decomposed longs) still require a real movd/movq and so keep the CreateScalarUnsafe node and fall through to standard containment. The scalar FMA negation loop is restructured accordingly: a removed FP CreateScalarUnsafe leaves a bare GT_NEG operand which is folded into the negated FMA variant. A codegen store size assert is relaxed to allow a narrow FP scalar feeding a wider SIMD local. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fold the floating-point CreateScalarUnsafe elision into the shared InsertNewSimdCreateScalarUnsafeNode: for a non-constant floating-point scalar it now returns the bare operand (the scalar already occupies the lowest element of a SIMD register with undefined upper elements, matching the Unsafe contract) instead of manufacturing a node that lowering would immediately remove. Integral and constant scalars still create a real node. LowerNode is folded into the shared helper so callers no longer lower the result themselves. This removes the xarch-only InsertNewSimdCreateScalarUnsafeNodeIfNeeded helper and applies the elision on arm64 as well (guarded to xarch/arm64; wasm is excluded because its SIMD model cannot hold a bare scalar in a v128 register). The user-level removal case is also tightened to always remove the node, transferring the unused marker to the operand when there is no use. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… lowering CreateScalar/CreateScalarUnsafe are not themselves memory operations. They were only marked contained so codegen could look through them to the underlying scalar, producing a second level of containment plus special handling in LSRA and codegen. Instead of containing such a node when it is consumed as a low-lane scalar by a parent hardware intrinsic (exactly the set IsContainableHWIntrinsicOp already selects), remove it in lowering and let the parent consume the scalar directly. The scalar keeps whatever contained/regOptional state it was already given, so codegen is unchanged (the memory operand or register is folded straight into the parent). Decomposed longs still require a real movd/movq and keep the node. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
After lowering was changed to remove CreateScalar/CreateScalarUnsafe when a parent consumes it as a low-lane scalar (rather than containing the node), no CreateScalar/CreateScalarUnsafe is ever marked contained. Remove the now-dead support for a contained CreateScalar/Unsafe: - gentree.cpp: drop the CreateScalar/Unsafe case from isContainableHWIntrinsic. canBeContained() now returns false for these, so the DEBUG isContained() assert actively verifies none are ever contained on any target. - instr.cpp: remove the genOperandDesc look-through that treated a contained CreateScalar/Unsafe as a load from memory. - lsraxarch.cpp: remove SkipContainedUnaryOp (an identity function once no such node is contained) and read the operands directly. - Refresh stale comments in instr.cpp and hwintrinsiccodegenxarch.cpp that referenced the removed look-through. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…HWIntrinsicIdForBinOp Removing a floating-point CreateScalarUnsafe during lowering can leave a bare scalar operand feeding a SIMD binary op, such as a bitwise combine that is formed during lowering. The scalar occupies the low element of a SIMD register and is consumed at full register width, which matches the Unsafe contract. Relax the operand-shape asserts in GetHWIntrinsicIdForBinOp to permit such a floating scalar in addition to a full SIMD operand. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
These intrinsics widen a narrower vector into a wider one where the upper bits are undefined. At the register level that is a no-op: the value already occupies the low bits of the wider register. Remove the node during lowering and let the consumer read the source operand directly, avoiding a register-to-register copy when the source is still live and keeping the node out of LIR. The consumer reads the operand from a register at its own wider size with the upper bits undefined, which is exactly the contract of these nodes. Containment performs its own size check, so the operand can never be widened into an undersized contained memory operand. Relax the genCodeForStoreLclVar size assert to allow a narrower SIMD source feeding a wider SIMD local. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
GetLower and GetLower128 narrow a wider vector by reading only its low bits, which at the register level is a no-op: the value already occupies the low bits of a register. Remove the node during lowering and let the consumer read the source operand directly at its own (narrower) size, which avoids a register-to-register copy when the source is still live and keeps the node out of LIR. Containment performs its own size check, and the source of these nodes is always a full-width vector whose memory is fully allocated, so reading it at the consumer's size is always in bounds. Extend the genCodeForStoreLclVar size assert to also allow a wider SIMD source feeding a narrower SIMD local. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Mirror the xarch GetLower removal on arm64. GetLower narrows a Vector128 to a Vector64 by reading its low 64 bits, which at the register level is free because a Q register and its low D register alias the same physical register. Remove the node during lowering so the consumer reads the source directly at its own size. Release codegen is correct by construction: consumers size their operands from their own node and all SIMD values share the arm64 V register class, so no register-class mismatch or size assumption is broken. This change is code-reviewed only; the arm64 cross-compilation toolchain is not available in the authoring environment, so it relies on checked arm64 CI for DEBUG-assert validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR updates CoreCLR JIT lowering/codegen to treat several “transparent” scalar/vector conversion intrinsics as HIR-only by eliminating them during lowering, and adjusting containment / asserts accordingly. The goal is to remove fragile 2-deep containment patterns and reduce special-case handling in LSRA/codegen while preserving operand sizing guarantees via existing containment checks.
Changes:
- Remove floating-point
Vector.CreateScalarUnsafenodes (and other transparent widen/narrow nodes likeToVector*Unsafe,GetLower*) during lowering and have consumers directly use the underlying operand. - Centralize xarch containment handling for CreateScalar wrappers via
ContainHWIntrinsicOperand, avoiding 2-deep containment and associated LSRA/codegen special-casing. - Relax/adjust debug assertions and operand handling in codegen/IR helpers to accept scalar-typed operands that physically occupy SIMD registers after lowering.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/lsraxarch.cpp | Removes LSRA “skip contained unary” helper now that transparent wrappers are eliminated during lowering. |
| src/coreclr/jit/lowerxarch.cpp | Removes transparent HWIntrinsic nodes in LIR (CreateScalarUnsafe / ToVectorUnsafe / GetLower) and introduces ContainHWIntrinsicOperand to avoid 2-deep containment. |
| src/coreclr/jit/lowerwasm.cpp | Stops manually lowering newly inserted CreateScalarUnsafe nodes (now lowered in the shared helper). |
| src/coreclr/jit/lowerarmarch.cpp | Removes Vector.GetLower during lowering on arm64 and stops manually lowering inserted CreateScalarUnsafe nodes. |
| src/coreclr/jit/lower.h | Declares the new xarch-only ContainHWIntrinsicOperand helper. |
| src/coreclr/jit/lower.cpp | Updates InsertNewSimdCreateScalarUnsafeNode to elide floating-point reinterprets on xarch/arm64 and to lower created nodes internally. |
| src/coreclr/jit/instr.cpp | Removes operand-desc special-casing for contained CreateScalar nodes and updates contained broadcast commentary/behavior accordingly. |
| src/coreclr/jit/hwintrinsiccodegenxarch.cpp | Updates ToScalar/AsVector* comment to match new lowering (no CreateScalar lookthrough in genOperandDesc). |
| src/coreclr/jit/gentree.cpp | Removes CreateScalar containability hook and relaxes DEBUG operand-shape asserts to allow scalar operands after lowering. |
| src/coreclr/jit/codegenxarch.cpp | Relaxes DEBUG store-size assert to account for scalar/width-mismatched SIMD sources after transparent-node elimination. |
Copilot's findings
- Files reviewed: 10/10 changed files
- Comments generated: 0
The FP CreateScalarUnsafe, ToVector256Unsafe/ToVector512Unsafe, and GetLower/GetLower128 removals rewired every use to the underlying operand, including stores, returns, and call arguments. For those non-hwintrinsic consumers the value is materialized at the node's own type and size via the ABI, so replacing the node with a wider or narrower operand corrupts the copy size. This produced oversized reads (e.g. a 32-byte vmovups over a 16-byte Vector128 stack slot) that could read out of bounds and, on the SysV ABI where vectors larger than 16 bytes are passed through memory, crash with a SIGSEGV during the mis-sized argument copy. Gate each removal on the consumer being a GenTreeHWIntrinsic. Such a consumer reads the operand from a register at the intrinsic's own size (the widening cases rely on the upper bits being undefined, which matches the Unsafe contract) or as a contained memory operand whose size is independently validated by containment. Non-hwintrinsic consumers keep the node so the reinterpret is materialized at the correct size. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… is hwintrinsic-only Removal of transparent SIMD reinterprets is now gated on the use being a hwintrinsic, so a store consumer always keeps the node and its op1 matches the store's target type/size. The relaxed genCodeForStoreLclVar assert is therefore dead and is tightened back to the strict form. TryGetUse only returns true with a non-null user, so the use.User() null checks at the three lowering sites are redundant and removed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Drop the isValidBinOpOperand lambda in favor of inlining the 'op->TypeIs(simdType) || varTypeIsFloating(op)' check directly at each assert site. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…operands The assert assumed op3 could never share the target register. That held while the multi-element Vector.Create Insert-chain always fed a distinct CreateScalarUnsafe def as op1, but once a floating-point CreateScalarUnsafe is elided into a bare scalar the same enregistered value can be used as both op1 and op3 (e.g. Vector.Create(x, x)). op3 is delay-free, so a distinct op3 interval can never land on the def register; targetReg == op3Reg is only reachable when op3 aliases op1, which also forces targetReg == op1Reg. In that case the leading mov is skipped, so op3 is preserved and the ins is correct. Relax the assert to the precise safety condition accordingly. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
… arm64 lowering These are pure register-level reinterprets: ToVector128Unsafe widens a Vector64 into a Vector128 with undefined upper bits, and a floating-point CreateScalarUnsafe places a scalar in the low element of a SIMD register with undefined upper elements. When the consumer is another GenTreeHWIntrinsic it already reads the value from the low bits of the register, so the node is a no-op and can be removed in lowering, avoiding a register-to-register copy when the source is still live. The underlying operand is left at its natural type - retyping it would corrupt spill and memory-access sizes for other consumers. FusedMultiplyAddScalar is excluded because its lowering folds a size-8 CreateScalarUnsafe wrapping a GT_NEG into the negated fused-multiply variant. Integral CreateScalarUnsafe still needs an explicit fmov and is left unchanged. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The transparent-node elision during lowering can leave a size-changing SIMD reinterpret operand -- e.g. an elided GetLower or ToVectorXXXUnsafe -- feeding a reconstructed binop. In LIR the operand still occupies a full SIMD register and is consumed at the node's width, and containment validates the memory-operand size before allowing a load, so the exact-shape asserts only need to enforce in HIR. Gate them on fgNodeThreading == NodeThreading::LIR and treat any SIMD-typed operand as a full-vector operand in GetHWIntrinsicIdForBinOp. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@MihuBot -nuget |
|
CC. @EgorBo, @dotnet/jit-contrib This removes about 15.5k bytes of codegen from arm64 and around 20.9k bytes from x64 by removing the "unnecessary" Almost all diffs are of a form similar to: - vmovaps ymm1, ymm0
- ins ymm0, ymm1, xmm0
+ ins ymm0, ymm0, xmm0or on Arm64 - mov v18.16b, v16.16b
- uxtl v18.8h, v18.8b
+ uxtl v18.8h, v16.8bThis is a safe optimization given that containment already checks for the operand size and it being at least the memory access size for the operation to succeed, so it really just removes the unnecessary node by making it legal in LIR to have something like It provides a small TP improvement across all targets as well |
|
Most of the change is just adjusting asserts and comments that were validating the shape was correct for HIR and then adding some explanatory comments about where its safe and why. |
The transparent-node elision during lowering can feed a size-changing SIMD reinterpret operand (e.g. an elided GetLower or ToVectorXXXUnsafe) into a reconstructed comparison via TryInvertMask. Such an operand still occupies a full SIMD register and is consumed at the node's width, so the exact operand size only needs to be enforced in HIR. Mirror the gtNewSimdBinOpNode relaxation using fgNodeThreading == NodeThreading::LIR, guarding the debug-only helper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…e-scalar-conversion-hwintrinsics
|
@MihuBot -nuget |
Summary
Today, hwintrinsic lowering in the JIT specially marks
CreateScalarUnsafeas contained on xarch so that no register is allocated and the value is truly "free". This is a fragile design: it produces 2-deep containment, requires special-case checks throughout LSRA/codegen, and doesn't generalize to the many other conceptually-identical "transparent" nodes (ToScalarfor floating-point,GetLower/GetLower128,ToVector256Unsafe/ToVector512Unsafe, etc.).Rather than extend that design to the other node types, this treats these nodes as effectively HIR-only and removes them during lowering, updating codegen to handle the resulting patterns directly. The nodes are not retyped (retyping would change spill size / memory-access size for certain containment cases); instead the consuming intrinsic consumes the underlying value directly, which already lives in the correct register and is implicitly consumable at full width.
As expected, the primary impact is on asserts that checked operands were an exact size/shape (e.g. requiring both operands of a
TYP_SIMD32-producing intrinsic to themselves beTYP_SIMD32). Containment checks already validate that a value is at least as large as the memory access requires, so removing the transparent node does not weaken those guarantees.Changes
Each commit is a self-contained step:
CreateScalarUnsafeduring xarch lowering, and stop the shared builders from manufacturing it in the first place.CreateScalar/CreateScalarUnsafewhen containing it during xarch lowering.CreateScalarUnsafesupport across LSRA/codegen and theGenTreecontainability plumbing.ToVector256Unsafe/ToVector512Unsafeduring xarch lowering.GetLower/GetLower128during xarch lowering, andGetLowerduring arm64 lowering.ToVector128Unsafeand floating-pointCreateScalarUnsafeduring arm64 lowering.gtNewSimdBinOpNode/GetHWIntrinsicIdForBinOpoperand-shape asserts HIR-only (gated onfgNodeThreading == NodeThreading::LIR), so a reconstructed binop in LIR may consume a size-changing SIMD reinterpret operand left behind by an elided transparent node. This keeps the aggressive elision for all hwintrinsic consumers (e.g.ConditionalSelect/Dotreconstruction) rather than falling back to a codegen-costing gate.Deferred (need additional design work, not included here): the
AsVector*family (sub-16-byte SIMD8/SIMD12 memory-access-size hazard) and floating-pointToScalar(SIMD→scalarvar_typeflip at ABI boundaries).Validation
SPMI asmdiffs (all-up PR vs merge-base, Windows, Checked):
IndexOfAnyAsciiSearcher,Ascii.Equals/IsValidCore,ProbabilisticMap,StringSearchValues, plus Vector128/Numerics. The arm64 regressions are a few bytes of alignment/ordering noise from removing nodes and are dwarfed by the improvements.JIT tests (priority-1, Checked, this JIT):
JIT/HardwareIntrinsics/Generalmerged_r: 2551/2551 passedJIT/HardwareIntrinsics/Generalmerged_ro: 2551/2551 passedJIT/SIMD: 114/114 real tests passed (the 2Vector3Interopcases fail only due to a missing native interop DLL from a local-SkipNativebuild, unrelated to codegen)Note
This pull request was authored with the assistance of GitHub Copilot.