Remove dead SIMD contiguous-args code in the JIT#128965
Conversation
Co-authored-by: Copilot <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 removes a set of SIMD “contiguous arguments / contiguous field stores” helpers and the (now unreachable) optimization paths that depended on them, simplifying the JIT’s SIMD import/morph pipeline.
Changes:
- Deletes the contiguous-args detection helpers and related address-building/statement-combining code paths.
- Removes the importer/morph hooks that attempted to detect and coalesce contiguous SIMD field stores.
- Simplifies
Vector*.Create(...)handling on xarch/arm64 to always build the HWIntrinsic node (no contiguous-args fast-path).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/coreclr/jit/simd.cpp | Removes contiguous-args / contiguous-field-store helper routines previously under FEATURE_SIMD. |
| src/coreclr/jit/morph.cpp | Removes getSIMDStructFromField helper that supported the deleted SIMD store combining logic. |
| src/coreclr/jit/lclmorph.cpp | Removes the local-morph pass logic that attempted to combine contiguous SIMD field stores. |
| src/coreclr/jit/importer.cpp | Removes the importer hook that tried to mark contiguous SIMD field stores per-statement. |
| src/coreclr/jit/hwintrinsicxarch.cpp | Removes contiguous-args-based Create fast path; always emits the intrinsic node. |
| src/coreclr/jit/hwintrinsicarm64.cpp | Same as xarch: removes contiguous-args-based Create fast path. |
| src/coreclr/jit/compiler.h | Removes declarations/fields tied to the deleted SIMD contiguous-args/store-combining implementation. |
|
I think with this we no longer coalesce vector field stores? |
|
PTAL @jakobbotsch @tannergooding @dotnet/jit-contrib This was targeting a very specific fragile pattern: public Vector128<float> Test1(float[] arr)
{
return Vector128.Create(arr[0], arr[1], arr[2], arr[3]);
}only for floats. Not even mentioning that this technically violates the memory model since the arr must be 16b aligned so all elements are readed without possible torn values. But if we think we need this we can do it in LowerStoreCoalescing, but I am not sure this pattern is valuable |
|
/ba-g known CSharpMissingShebangInFileBasedProgram failure |
|
Seems good to me
Just for reference, this was namely supporting some of the older If there are real world scenarios (and they can't be handled with our other APIs we expose), then I agree that handling it in lowering is the better place. |
let's see if this transformation is worth the complexity it brings.