Arm64 SVE: Support scalable constant vectors and masks#127520
Arm64 SVE: Support scalable constant vectors and masks#127520a74nh wants to merge 22 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
4d39083 to
4754486
Compare
Adds support to GenTreeVecCon and GenTreeMskCon for constants with unknown sizes. Instead of having a blob of data, the constant is represented as being one of either: a repeated value, an sequence with start and step values, or a value in the first lane and the rest zeroed. To handle this the base type is also required. As this new structure is slightly bigger than a simd16, the simd_t typedef is pushed up to simd32 sized. For vector constants, a vector is scalable because if it is of TYP_SIMD. For mask constants, the type is always TYP_MASK. However on Arm64, masks are only used by SVE. Therefore to tell if a mask is scalable then JitUseScalableVectorT is checked. The IsAllBitsSet() on mask constants is updated to include a base type. A mask that is all set for TYP_LONG will not be all set for TYP_BYTE, and instead will be 100010001000... Given two scalable constants it may not be possible to add them together to produce a third scalable constant. Instead they will remain as two vectors in the IR. To show this implementation is workable, scalable support is added for: Sve.CreateTrueMask*() Sve.CreateFalseMask*() Vector.Create() Vector.CreateScalar() Vector.CreateScalarUnsafe() Vector.CreateSequence() Fixes dotnet#125057
4754486 to
7fac1f9
Compare
|
Taking this out of draft now. Because of the very limited support for scalable SVE, this is currently very hard to test. I've been working off the top of @snickolls-arm's WIP branch with all his code in, which allows me to to call handwritten tests. In current HEAD, there are too many errors before getting to my code. There's still a lot of work to do on top of this. Eg, I need to get generic ops working, plus all the other Vector APIs which create constants. But, I didn't want this PR to grow too big. The important part is this serves as a base for further constant work. @dotnet/arm64-contrib @jakobbotsch @tannergooding |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds Arm64 SVE “scalable VectorT” support across the JIT, including new encodings for scalable vector/mask constants and updates to value numbering, folding, lowering, LSRA, and codegen to recognize and emit SVE-friendly patterns.
Changes:
- Introduce new scalable constant representations (
simdscalable_t,simdmaskscalable_t) and plumb them throughGenTreeconstant nodes and hashing. - Extend value numbering and folding to create/consume scalable SIMD constants on Arm64.
- Implement Arm64 SVE
VectorTintrinsics import and codegen pathways (create/broadcast/sequence), plus mask handling updates.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/valuenum.h | Adds VN support for scalable SIMD constants on Arm64 |
| src/coreclr/jit/valuenum.cpp | Creates/broadcasts scalable SIMD VN constants and dumps them |
| src/coreclr/jit/simd.h | Defines new scalable vector/mask constant encodings and helper APIs |
| src/coreclr/jit/simd.cpp | Implements scalable vector/mask helpers and conversions |
| src/coreclr/jit/lsraarm64.cpp | Reserves temps for scalable vector constants that can’t be directly encoded |
| src/coreclr/jit/lowerarmarch.cpp | Updates mask lowering + VectorT intrinsic handling |
| src/coreclr/jit/hwintrinsiclistarm64sve.h | Enables VectorT intrinsics for SVE |
| src/coreclr/jit/hwintrinsiccodegenarm64.cpp | Emits SVE instructions for VectorT intrinsics |
| src/coreclr/jit/hwintrinsicarm64.cpp | Imports VectorT intrinsics and updates true/false mask creation |
| src/coreclr/jit/hwintrinsic.h | Marks VectorT_* as special cases for scalar/broadcast creation |
| src/coreclr/jit/gentree.h | Extends vector/mask constants to support scalable encodings |
| src/coreclr/jit/gentree.cpp | Adds scalable constant construction, hashing, folding, and printing |
| src/coreclr/jit/emitarm64.h | Repositions signed-immediate helpers used by new SVE paths |
| src/coreclr/jit/compiler.hpp | Extends bitmask helpers for >64-register targets |
| src/coreclr/jit/compiler.h | Adds new compiler helpers for scalable vector/mask constants |
| src/coreclr/jit/codegenarm64.cpp | Adds emission for scalable vector/mask constants |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/coreclr/jit/gentree.cpp:1
- The size comparison for scalable true masks looks inverted. A predicate that is 'all-true' at a smaller element granularity (e.g.,
.h) should be safe to use for larger element operations (e.g.,.d), but not vice-versa. The current>=check will incorrectly treat a large-granularity mask (e.g.,.d) as 'true' for smaller element types (e.g., byte), which could enable incorrect optimizations. Consider changing this togenTypeSize(maskBaseType) <= genTypeSize(simdBaseType)(and update the comment accordingly), so only equal-or-finer masks are treated as universally true.
src/coreclr/jit/gentree.cpp:1 - Correct spelling in comment: 'optimizatize' -> 'optimize'.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/coreclr/jit/gentree.cpp:1
gtNewSimdVconNodedoesn't zero-initializegtSimdScalableValbefore populating fields. Becausesimdscalable_tcontains small fields plus padding/union storage, this can leave padding bytes indeterminate and create nondeterminism when the struct is memcpy'd/printed/hashed elsewhere. Fix (recommended): initializevecCon->gtSimdScalableVal = {};(or equivalent) before setting the individual fields.
Change-Id: I627878463cd19d781d6fedaa4e7d3cc9257c4b1b
| static unsigned GetHashCode(const simdscalable_t& val) | ||
| { | ||
| unsigned hash = 0; |
There was a problem hiding this comment.
The hash for simdscalable_t is a straight XOR of multiple fields, which tends to create many collisions (especially when fields are correlated or differ only by swapped/masked bits). Consider using a stronger mixing strategy (e.g., multiplying by an odd constant + rotate/xor steps, or an existing JIT hash combiner) to reduce collisions in VNMap lookups.
There was a problem hiding this comment.
This is consistent with all the other hashing in the file. I didn't plan on switching it. Unless someone else thinks I should
| bool operator==(const simdscalable_t& other) const | ||
| { | ||
| if (IsZero() && other.IsZero()) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| return (gtSimdScalableBaseType == other.gtSimdScalableBaseType) && | ||
| (gtSimdScalableKind == other.gtSimdScalableKind) && (gtSimdScalableIndex == other.gtSimdScalableIndex) && | ||
| (gtSimdScalableStep == other.gtSimdScalableStep); | ||
| } |
There was a problem hiding this comment.
operator== canonicalizes all-zero encodings but does not canonicalize other semantically equivalent encodings (notably 'all-bits-set' repeated values across different base types). This can lead to multiple distinct constants/VNs/IR nodes that are semantically identical, reducing CSE/value-numbering effectiveness and complicating reasoning. Consider also canonicalizing 'all bits set' (and potentially other universally-representable patterns) either in equality/hash or during construction (e.g., normalize to a single base type/encoding when IsAllBitsSet() is true).
There was a problem hiding this comment.
Agreed. I was leaving that for a later PR. Didn't want to overcomplicate here. Worst case is fewer optimisations.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/coreclr/jit/gentree.cpp:1
- For
TYP_SIMDscalable constants, integral broadcasts currently store the full 64-bitIntegralValue()intogtSimdScalableIndexwithout truncating/canonicalizing to the element width. This breakssimdscalable_t::IsAllBitsSet()(and possibly equality/hash behavior) for smaller element types (e.g., broadcasting-1forTYP_BYTEwill store0xFFFF...FFFFand fail the0xFFall-bits-set check). Consider canonicalizing the stored value to the base-type width (e.g., mask to 8/16/32 bits, or memcpy from a correctly-sized scalar likeBroadcastConstantToSimdScalabledoes) at construction time for broadcast/scalar/scalarUnsafe/sequence.
src/coreclr/jit/gentree.cpp:1 - Grammar: change 'Attempts to folds' to 'Attempts to fold'.
| // Ensure simd_t is big enough to contain any simd type | ||
| #if defined(TARGET_XARCH) | ||
| typedef simd64_t simd_t; | ||
| #elif defined(TARGET_ARM64) | ||
| typedef simd32_t simd_t; |
There was a problem hiding this comment.
On ARM64, redefining simd_t as simd32_t increases the storage footprint of any structure that embeds simd_t (notably GenTreeVecCon via gtSimdVal). If simd_t is only required as a fixed-size backing store for fixed-width SIMD values, consider keeping it at 16 bytes and relying on the separate gtSimdScalableVal field/union for scalable constants, or otherwise minimizing where the 32-byte type is used. This can reduce IR node size and memory traffic during compilation.
| // Ensure simd_t is big enough to contain any simd type | |
| #if defined(TARGET_XARCH) | |
| typedef simd64_t simd_t; | |
| #elif defined(TARGET_ARM64) | |
| typedef simd32_t simd_t; | |
| // Ensure simd_t is big enough to contain any fixed-width simd type without | |
| // unnecessarily inflating structures that embed it. Larger/scalable ARM64 | |
| // constants should use their dedicated storage rather than increasing simd_t. | |
| #if defined(TARGET_XARCH) | |
| typedef simd64_t simd_t; | |
| #elif defined(TARGET_ARM64) | |
| typedef simd16_t simd_t; |
There was a problem hiding this comment.
No, I think we need it bigger due to all the mempy's of simd_t that exist
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Comments suppressed due to low confidence (1)
src/coreclr/jit/gentree.cpp:1
- The parameter name
indexis ambiguous here: it is assigned togtSimdMaskScalableIndexand seems to represent a boolean-like 'all false (0) vs all true (1)' encoding rather than an index. Renaming it to something likemaskValue,isAllTrue, orpredicateValue(and possibly using auint8_tparameter) would make call sites much clearer.
| hash = static_cast<unsigned>(hash ^ val.scalable.gtSimdMaskScalableBaseType); | ||
| hash = static_cast<unsigned>(hash ^ val.scalable.gtSimdMaskScalableIndex); |
There was a problem hiding this comment.
simdmaskscalable_t::operator== treats all-zero masks as equal regardless of base type (IsZero() short-circuit), but this hash includes gtSimdMaskScalableBaseType. That breaks the required invariant for hash maps (equal keys must hash the same), causing failed lookups or duplicate entries. Canonicalize scalable-zero in GetHashCode (e.g., hash only isScalable + a fixed base type + index 0, or special-case val.scalable.IsZero() similarly to the simdscalable_t hash canonicalization).
| hash = static_cast<unsigned>(hash ^ val.scalable.gtSimdMaskScalableBaseType); | |
| hash = static_cast<unsigned>(hash ^ val.scalable.gtSimdMaskScalableIndex); | |
| // simdmaskscalable_t::operator== treats all-zero scalable masks as equal | |
| // regardless of base type, so canonicalize that case in the hash as well. | |
| if (!val.scalable.IsZero()) | |
| { | |
| hash = static_cast<unsigned>(hash ^ val.scalable.gtSimdMaskScalableBaseType); | |
| hash = static_cast<unsigned>(hash ^ val.scalable.gtSimdMaskScalableIndex); | |
| } |
| struct simdmaskvalue_t | ||
| { | ||
| uint8_t isScalable; | ||
| simdmaskscalable_t scalable; | ||
| simdmask_t fixed; | ||
|
|
||
| static simdmaskvalue_t FromFixed(const simdmask_t& mask) |
There was a problem hiding this comment.
simdmaskvalue_t is defined under #if defined(TARGET_ARM64) but is used by non-ARM64 code paths in ValueNumStore changes (e.g., VarTypConv<TYP_MASK> and GetConstantSimdMaskValue are guarded only by FEATURE_MASKED_HW_INTRINSICS). If FEATURE_MASKED_HW_INTRINSICS is enabled on other targets (notably xarch), this will fail to compile because simdmaskvalue_t is incomplete/undefined. Either (1) define simdmaskvalue_t for all targets (with a fixed-only encoding on non-ARM64), or (2) keep simdmaskvalue_t usages fully TARGET_ARM64-guarded and preserve simdmask_t storage on other architectures.
| if (emitter::isValidSimm<8>(simdVal.gtSimdScalableIndex) || | ||
| emitter::isValidSimm_MultipleOf<8, 256>(simdVal.gtSimdScalableIndex)) |
There was a problem hiding this comment.
isValidSimm takes an ssize_t but the argument is uint64_t. Converting an out-of-range uint64_t to a signed type is implementation-defined in C++, which can make the check non-portable and harder to reason about (notably for negative values represented in two’s complement). Prefer storing gtSimdScalableIndex/Step as int64_t (if these are conceptually signed immediates) or cast explicitly through int64_t/ssize_t in a way that documents the intended interpretation before calling isValidSimm.
Adds support to GenTreeVecCon and GenTreeMskCon for constants with unknown sizes. Instead of having a blob of data, the constant is represented as being one of either: a repeated value, an sequence with start and step values, or a value in the first lane and the rest zeroed. To handle this the base type is also required.
As this new structure is slightly bigger than a simd16, the simd_t typedef is pushed up to simd32 sized.
For vector constants, a vector is scalable because if it is of TYP_SIMD.
For mask constants, the type is always TYP_MASK. However on Arm64, masks are only used by SVE. Therefore to tell if a mask is scalable then JitUseScalableVectorT is checked.
The IsAllBitsSet() on mask constants is updated to include a base type. A mask that is all set for TYP_LONG will not be all set for TYP_BYTE, and instead will be 100010001000...
Given two scalable constants it may not be possible to add them together to produce a third scalable constant. Instead they will remain as two vectors in the IR.
To show this implementation is workable, scalable support is added for:
Fixes #125057