[arm64] Use the scalar shift when scaling Vector64 shuffle indices - #131488
[arm64] Use the scalar shift when scaling Vector64 shuffle indices#131488lewing wants to merge 2 commits into
Conversation
gtNewSimdShuffleVariableNode scales variable shuffle indices into byte offsets with AdvSimd.ShiftLeftLogical. For a Vector64 with a 64-bit element type that is a 1D arrangement, which the vector form of the shift has no encoding for, so the JIT tripped 'opt != INS_OPTS_1D' in the arm64 emitter. gtNewSimdBinOpNode already selects the scalar form for this case; do the same here. nint/nuint are the only Vector64 shuffle overloads with a 64-bit element type, and only on a 64-bit platform, so this needs variable indices on arm64 to hit. ILC reached it compiling CoreLib, which blocked NativeAOT builds using a checked JIT on arm64. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77f83e48-ef2e-4f33-b6f4-5c20e251b20d
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR fixes an arm64 checked-JIT assertion when compiling Vector64.Shuffle / Vector64.ShuffleNative with variable indices for a Vector64 whose element size is 64-bit (a 1D arrangement). The JIT now selects the scalar shift intrinsic when scaling shuffle indices in that specific Vector64/64-bit-element case, avoiding emission of a reserved vector shift encoding.
Changes:
- Update
gtNewSimdShuffleVariableNode(arm64) to useNI_AdvSimd_ShiftLeftLogicalScalarwhen(simdSize == 8) && (elementSize == 8)while scaling shuffle indices. - Add regression tests for
nint/nuintcoveringShuffleandShuffleNativewith variable indices (via non-inlined wrappers).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/gentree.cpp | Selects scalar vs vector shift intrinsic for the Vector64+64-bit-element shuffle-index scaling case on arm64. |
| src/libraries/System.Runtime.Intrinsics/tests/Vectors/Vector64Tests.cs | Adds xUnit coverage to exercise the variable-indices shuffle path for Vector64<nint> / Vector64<nuint> (Shuffle + ShuffleNative). |
The nint/nuint shuffle overloads exist on 32-bit too; it is the element that is 64-bit only on a 64-bit platform. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 77f83e48-ef2e-4f33-b6f4-5c20e251b20d
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
src/libraries/System.Runtime.Intrinsics/tests/Vectors/Vector64Tests.cs:2486
- Same issue as above: the inserted indices are compile-time constants, so this may not reliably hit the JIT variable-indices shuffle path. Wrap the inserted values with
GetNonConstantto keep them variable.
for (int index = 0; index < Vector64<nuint>.Count; index++)
{
vector = vector.WithElement(index, (nuint)(index + 1));
indices = indices.WithElement(index, (nuint)(Vector64<nuint>.Count - index - 1));
}
src/libraries/System.Runtime.Intrinsics/tests/Vectors/Vector64Tests.cs:2506
- Same issue as above: constant indices may allow the JIT to take the constant-indices shuffle path and skip the index fix-up logic. Wrap inserted values with
GetNonConstantso the indices remain variable.
for (int index = 0; index < Vector64<nint>.Count; index++)
{
vector = vector.WithElement(index, (nint)(index + 1));
indices = indices.WithElement(index, (nint)(Vector64<nint>.Count - index - 1));
}
src/libraries/System.Runtime.Intrinsics/tests/Vectors/Vector64Tests.cs:2526
- Same issue as above: the indices are inserted as constants, which can prevent coverage of the variable-index shuffle path this regression is targeting. Wrap inserted values with
GetNonConstant.
for (int index = 0; index < Vector64<nuint>.Count; index++)
{
vector = vector.WithElement(index, (nuint)(index + 1));
indices = indices.WithElement(index, (nuint)(Vector64<nuint>.Count - index - 1));
}
src/libraries/System.Runtime.Intrinsics/tests/Vectors/Vector64Tests.cs:2466
- These tests intend to cover the JIT’s variable indices shuffle path, but the indices being inserted are compile-time constants. Especially on 64-bit where
Vector64<nint>.Count == 1, the JIT can treat the indices as a constant vector and take the constant-indices path, which would not exercise the index fix-up/shift sequence this PR is guarding. Use the existingGetNonConstanthelper when inserting elements to prevent constant folding.
This issue also appears in the following locations of the same file:
- line 2482
- line 2502
- line 2522
for (int index = 0; index < Vector64<nint>.Count; index++)
{
vector = vector.WithElement(index, (nint)(index + 1));
indices = indices.WithElement(index, (nint)(Vector64<nint>.Count - index - 1));
}
Fixes #131487
gtNewSimdShuffleVariableNodescales variable shuffle indices into byte offsets withAdvSimd.ShiftLeftLogical. For aVector64with a 64-bit element type that is a1Darrangement, which the vector form of the arm64 shift-by-immediate has no encoding for, so a checked JIT tripsassert(opt != INS_OPTS_1D)inemitarm64.cpp.gtNewSimdBinOpNodealready selects the scalar form for exactly this case; the shuffle path builds the HWINTRINSIC node directly and so never got the guard. This applies the same condition.Impact
ILC instantiates
ShuffleNative<nuint>when compiling CoreLib whole-program, so this fails any NativeAOT build using a checked arm64 JIT (ilc … exited with code 133), which blockssrc/tests/build.sh nativeaot checkedon an arm64 host. It escapes CI becausewindows-x64 Checked NativeAOTis the only checked NativeAOT leg and the arm64 NativeAOT legs are Release, where the assert is compiled out.Tests
Four tests added to
Vector64Tests.cscoveringnint/nuint×Shuffle/ShuffleNative. Two details drove their shape:nint/nuintare the onlyVector64shuffle overloads with a 64-bit element type — there are nolong/ulong/doubleones — and only on a 64-bit platform. The tests are written pointer-size agnostic.[MethodImpl(NoInlining)]wrapper. The existing constant/local-indices style folds and would not cover this.Verified on osx-arm64, checked runtime:
Assertion failed 'opt != INS_OPTS_1D'System.Runtime.Intrinsics.TestsNote
This change was prepared with GitHub Copilot assistance and reviewed by the submitting developer.