Uh oh!
There was an error while loading. Please reload this page.
Fix HWIntrinsic codegen for elided scalar/vector reinterprets - #131155
Conversation
After dotnet#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>
|
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 |
tannergooding
commented
Jul 21, 2026
@dotnet/jit-contrib, @EgorBo, @dhartglassMSFT A fix for #131137. I did an audit of the other code paths and found one additional failure that I also added handling for. |
There was a problem hiding this comment.
Pull request overview
This PR fixes two x64 RyuJIT HWIntrinsic codegen bugs caused by lowering eliding “transparent” scalar/vector reinterpret nodes, where a couple of codegen decisions incorrectly depended on the post-lowering operand type instead of stable node metadata.
Changes:
- Update xarch lowering to preserve width-changing reinterprets when they are used as the AVX2 gather index operand, preventing VSIB width (xmm vs ymm) from being mis-encoded.
- Update xarch HWIntrinsic codegen to select vector vs pointer overloads for the
ConvertToVector{128,256}Int*intrinsics usingnode->OperIsMemoryLoad()rather thanvarTypeIsSIMD(op1->TypeGet()). - Add a JitBlue regression test covering both the convert-from-scalar and gather-with-narrowed-index scenarios.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/lowerxarch.cpp | Skips elision of GetLower/ToVector*Unsafe-style reinterprets when the user is an AVX2 gather and the node is the index operand, preserving VSIB width selection. |
| src/coreclr/jit/hwintrinsiccodegenxarch.cpp | Uses OperIsMemoryLoad() (aux-type-driven) to choose the correct convert overload path after reinterpret elision changes operand TypeGet(). |
| src/tests/JIT/Regression/JitBlue/Runtime_131137/Runtime_131137.cs | Adds xUnit-based regression coverage for Sse41/Avx2 convert-from-scalar and gather with index.GetLower(). |
| src/tests/JIT/Regression/JitBlue/Runtime_131137/Runtime_131137.csproj | Adds the test project for the new JitBlue regression scenario. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 0
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Follow up to #131155 (comment). `Runtime_131137` was added with a standalone `.csproj` and `RequiresProcessIsolation=true`, but the test meets none of the isolation rules in [`requiresprocessisolation.md`](https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/coreclr/requiresprocessisolation.md) -- it sets no environment variables, no host config, and no process-wide state. It''s just a `[ConditionalFact]` in a `Runtime_131137` namespace with no custom `Main`, so it merges cleanly. This removes the standalone project and adds the source into the shared `Regression_ro_2.csproj` runner, matching the rest of the regression tests. ---------- I also audited the other recently-added `JitBlue` tests that still carry standalone csprojs. All of them are justified: the immediate neighbors `Runtime_130844`/`130845`/`130846` set `CLRTestEnvironmentVariable`, the remaining ISO ones set `CLRTestEnvironmentVariable`/`CLRTestTargetUnsupported`, and `Runtime_8980` disables the xunit wrapper generator. `Runtime_131137` was the only one with no trigger. CC. @EgorBo > [!NOTE] > This PR description was drafted by Copilot. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fixes#131137.
PR #130444 started removing transparent scalar/vector reinterpret
HWINTRINSICnodes (CreateScalarUnsafe,GetLower/GetLower128,ToVector256Unsafe/ToVector512Unsafe) during lowering when the consumer is anotherHWINTRINSIC. That's correct in general -- the consumer reads the value from a register at its own size -- but two x64 codegen sites keyed their decision off the operand's post-lowering type, which the elision changes. Both now produce wrong code.ConvertToVector128Int*/ConvertToVector256Int*(the reported crash)These have a vector overload
(Vector128<T>)and a pointer overload(T*), both lowering topmovzx*. Codegen picked between them withvarTypeIsSIMD(op1). Once an elidedCreateScalarUnsafeleaves the vector overload's operand scalar-typed, that proxy misfires and codegen takes the memory-load path -- reading the scalar value as if it were an address (the reportedNullReferenceException; a checked JIT asserts inemitxarch.cpp). Fixed by selecting the overload from the stablenode->OperIsMemoryLoad()metadata (aux-type driven), matching the generic table path already used elsewhere in the file.AVX2 gather VSIB index width
A gather selects its VSIB index width (xmm vs ymm) from the index operand's own width (
indexOp->TypeIs(TYP_SIMD32)). An elidedGetLower/ToVector*Unsafeon the index changes that width, so the wrong VEX.L is encoded and the hardware reads the wrong number of indices (e.g. avpgatherqdwith aGetLower()-narrowed index gathered 4 elements instead of 2 -- a silent wrong result, not visible in the JIT disasm since it always prints the index asxmm). The index width can't be recovered in codegen, so this is fixed in lowering: the reinterpret elision is skipped when the node is a gather's index operand, since that width is load-bearing.I also audited the rest of
hwintrinsiccodegenxarch.cpp, the store-containment paths incodegenxarch.cpp, andemitxarch.cpp: every other size/attr decision derives from node metadata (node->TypeGet(),GetSimdSize(),GetSimdBaseType()), and theoperandSize >= expectedSizecheck inIsContainableHWIntrinsicOpprevents a narrowed operand from being contained undersized. These two sites were the only operand-width-driven exceptions.Added
Runtime_131137covering both fixed paths (Sse41/Avx2 convert-from-scalar and the gather-with-narrowed-index case); it fails without the fix and passes with it.Note
This PR was authored with the assistance of GitHub Copilot.