From 9b0724a2a640de4633b4c9616b7542a97040e746 Mon Sep 17 00:00:00 2001 From: Tanner Gooding Date: Tue, 21 Jul 2026 08:36:44 -0700 Subject: [PATCH] Fix HWIntrinsic codegen for elided scalar/vector reinterprets After #130444 began removing transparent CreateScalarUnsafe/GetLower/ ToVector*Unsafe nodes during lowering, two x64 codegen sites that keyed off an operand's post-lowering type produced wrong code: * ConvertToVector128Int*/ConvertToVector256Int* selected the pointer (memory-load) overload via varTypeIsSIMD(op1); an elided CreateScalarUnsafe left the vector overload's operand scalar-typed, so it loaded from the value as if it were an address. Use node->OperIsMemoryLoad() instead. * An AVX2 gather selects its VSIB index width (xmm vs ymm) from the index operand's own width; an elided GetLower/ToVector*Unsafe on the index changed that width and gathered the wrong number of elements. Skip the elision when the node is a gather's index operand, since that width is load-bearing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- src/coreclr/jit/hwintrinsiccodegenxarch.cpp | 4 +- src/coreclr/jit/lowerxarch.cpp | 36 +++++++++-- .../JitBlue/Runtime_131137/Runtime_131137.cs | 64 +++++++++++++++++++ .../Runtime_131137/Runtime_131137.csproj | 12 ++++ 4 files changed, 109 insertions(+), 7 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_131137/Runtime_131137.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_131137/Runtime_131137.csproj diff --git a/src/coreclr/jit/hwintrinsiccodegenxarch.cpp b/src/coreclr/jit/hwintrinsiccodegenxarch.cpp index 2863faecba5df4..c445bd8a7a0150 100644 --- a/src/coreclr/jit/hwintrinsiccodegenxarch.cpp +++ b/src/coreclr/jit/hwintrinsiccodegenxarch.cpp @@ -2762,7 +2762,7 @@ void CodeGen::genX86BaseIntrinsic(GenTreeHWIntrinsic* node, insOpts instOptions) GenTree* op1 = node->Op(1); instruction ins = HWIntrinsicInfo::lookupIns(intrinsicId, baseType, m_compiler); - if (!varTypeIsSIMD(op1->TypeGet())) + if (node->OperIsMemoryLoad()) { // Until we improve the handling of addressing modes in the emitter, we'll create a // temporary GT_IND to generate code with. @@ -2965,7 +2965,7 @@ void CodeGen::genAvxFamilyIntrinsic(GenTreeHWIntrinsic* node, insOpts instOption { instruction ins = HWIntrinsicInfo::lookupIns(intrinsicId, baseType, m_compiler); - if (!varTypeIsSIMD(op1->gtType)) + if (node->OperIsMemoryLoad()) { // Until we improve the handling of addressing modes in the emitter, we'll create a // temporary GT_IND to generate code with. diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index fdc6032f8a9209..cab6d199c59d10 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -2662,16 +2662,42 @@ GenTree* Lowering::LowerHWIntrinsic(GenTreeHWIntrinsic* node) // be read from an undersized contained memory operand. Any other consumer (a store, // return, or call argument) materializes a value of the node's own type and size via the // ABI, so removing the node there would corrupt the copy size; keep the node for those. + // + // The one exception is an AVX2 gather index: the VSIB encoding (xmm vs ymm) is selected + // from the index operand's own width rather than the gather's size, so a width-changing + // reinterpret feeding it is load-bearing and must be kept. LIR::Use use; if (BlockRange().TryGetUse(node, &use) && use.User()->OperIsHWIntrinsic()) { - GenTree* op1 = node->Op(1); - GenTree* next = node->gtNext; + GenTreeHWIntrinsic* user = use.User()->AsHWIntrinsic(); + GenTree* gatherIndex = nullptr; - use.ReplaceWith(op1); - BlockRange().Remove(node); - return next; + switch (user->GetHWIntrinsicId()) + { + case NI_AVX2_GatherVector128: + case NI_AVX2_GatherVector256: + gatherIndex = user->Op(2); + break; + + case NI_AVX2_GatherMaskVector128: + case NI_AVX2_GatherMaskVector256: + gatherIndex = user->Op(3); + break; + + default: + break; + } + + if (gatherIndex != node) + { + GenTree* op1 = node->Op(1); + GenTree* next = node->gtNext; + + use.ReplaceWith(op1); + BlockRange().Remove(node); + return next; + } } break; } diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_131137/Runtime_131137.cs b/src/tests/JIT/Regression/JitBlue/Runtime_131137/Runtime_131137.cs new file mode 100644 index 00000000000000..d7f0f4a38b14c2 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_131137/Runtime_131137.cs @@ -0,0 +1,64 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +// Lowering elides transparent scalar/vector reinterprets (CreateScalarUnsafe, GetLower, ...). +// Codegen for the dual-overload x64 intrinsics below must therefore distinguish the vector and +// pointer/index overloads using stable node metadata rather than the post-lowering operand type: +// * ConvertTo*Int* picked the pointer (memory-load) overload from the operand type, so an elided +// CreateScalarUnsafe made it load from the scalar value as if it were an address. +// * An AVX2 gather selects its VSIB width (xmm vs ymm index) from the index operand's width, so an +// elided GetLower on the index widened it and gathered too many elements. + +namespace Runtime_131137; + +using System.Runtime.CompilerServices; +using System.Runtime.Intrinsics; +using System.Runtime.Intrinsics.X86; +using Xunit; + +public static class Runtime_131137 +{ + [MethodImpl(MethodImplOptions.NoInlining)] + static Vector128 ConvertI32(int packed) + => Sse41.ConvertToVector128Int32(Vector128.CreateScalarUnsafe(packed).AsByte()); + + [MethodImpl(MethodImplOptions.NoInlining)] + static Vector128 ConvertI16(int packed) + => Sse41.ConvertToVector128Int16(Vector128.CreateScalarUnsafe(packed).AsSByte()); + + [MethodImpl(MethodImplOptions.NoInlining)] + static Vector128 ConvertI64(int packed) + => Sse41.ConvertToVector128Int64(Vector128.CreateScalarUnsafe(packed).AsByte()); + + [ConditionalFact(typeof(Sse41), nameof(Sse41.IsSupported))] + public static void ConvertToVectorFromScalar() + { + Assert.Equal(Vector128.Create(1, 2, 3, 4), ConvertI32(0x04030201)); + Assert.Equal(Vector128.Create((short)1, 2, 3, 4, 0, 0, 0, 0), ConvertI16(0x04030201)); + Assert.Equal(Vector128.Create(1L, 2), ConvertI64(0x00000201)); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static Vector256 ConvertI32x8(long packed) + => Avx2.ConvertToVector256Int32(Vector128.CreateScalarUnsafe(packed).AsByte()); + + [ConditionalFact(typeof(Avx2), nameof(Avx2.IsSupported))] + public static void ConvertToVector256FromScalar() + { + Assert.Equal(Vector256.Create(1, 2, 3, 4, 5, 6, 7, 8), ConvertI32x8(0x0807060504030201L)); + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static unsafe Vector128 GatherQD(int* baseAddr, Vector256 index) + => Avx2.GatherVector128(baseAddr, index.GetLower(), 4); + + [ConditionalFact(typeof(Avx2), nameof(Avx2.IsSupported))] + public static unsafe void GatherWithNarrowedIndex() + { + int* buf = stackalloc int[4] { 20, 21, 22, 23 }; + Vector256 index = Vector256.Create(0L, 2, 1, 3); + + // Only the low 128 bits of the index ({0, 2}) are in play, so lanes 2 and 3 stay zero. + Assert.Equal(Vector128.Create(20, 22, 0, 0), GatherQD(buf, index)); + } +} diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_131137/Runtime_131137.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_131137/Runtime_131137.csproj new file mode 100644 index 00000000000000..8b1746dac08044 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_131137/Runtime_131137.csproj @@ -0,0 +1,12 @@ + + + Exe + true + + + + + + + +