Fix a couple issues with Vector.WithElement #115648
Open
+164
−15
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues with the Vector.WithElement intrinsic functionality and addresses related JIT failures by updating test cases and modifying the lowering and code generation logic.
- Updates test projects for runtime verification of the intrinsic changes.
- Revises the index calculation and bounds handling in the JIT lowering logic.
- Adjusts the emitter call in the hardware intrinsic code generation.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/tests/JIT/Regression/JitBlue/Runtime_115532/Runtime_115532.csproj | New project file for test verification |
| src/tests/JIT/Regression/JitBlue/Runtime_115532/Runtime_115532.cs | Test verifying the Vector.WithElement behavior |
| src/tests/JIT/Regression/JitBlue/Runtime_115487/Runtime_115487.csproj | New project file for test verification |
| src/tests/JIT/Regression/JitBlue/Runtime_115487/Runtime_115487.cs | Test demonstrating proper intrinsic handling and JIT behavior |
| src/coreclr/jit/lowerxarch.cpp | Refactored index handling with modulo masking and refined assert condition |
| src/coreclr/jit/hwintrinsiccodegenxarch.cpp | Changed emitter instruction from ins_Move_Extend to ins_Store to reflect desired semantics |
Comments suppressed due to low confidence (2)
src/coreclr/jit/lowerxarch.cpp:5676
- [nitpick] Using modulo to constrain 'imm8' limits the index into the SIMD vector, which may hide unexpected negative values from upstream logic. Please confirm that this behavior is intended and consider adding a comment that documents the rationale behind the modulo operation.
ssize_t imm8 = static_cast<uint8_t>(op2->AsIntCon()->IconValue()) % count;
src/coreclr/jit/hwintrinsiccodegenxarch.cpp:2008
- The instruction call has been updated from ins_Move_Extend to ins_Store. Please verify that this change is aligned with the intended store semantics and add documentation if necessary to clarify the rationale for future maintainers.
GetEmitter()->emitIns_ARX_R(ins_Store(op3->TypeGet()), // Store
|
Alternative to #115645? |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Yes. One that fixes the issues as they were relatively small/simple |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
tannergooding
commented
May 16, 2025
src/tests/JIT/Regression/JitBlue/Runtime_115532/Runtime_115532.cs
Outdated
Show resolved
Hide resolved
tannergooding
commented
May 16, 2025
src/tests/JIT/Regression/JitBlue/Runtime_115487/Runtime_115487.cs
Outdated
Show resolved
Hide resolved
src/tests/JIT/Regression/JitBlue/Runtime_115532/Runtime_115532.cs
Outdated
Show resolved
Hide resolved
filipnavara
reviewed
May 16, 2025
Co-authored-by: Filip Navara <filip.navara@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
tannergooding
commented
May 17, 2025
Comment on lines
+28
to
+29
| vdec = vdec.WithElement(2, 0.0) | ||
| .WithElement(3, 0.0); |
There was a problem hiding this comment.
Looks like Mono currently asserts for these:
16:52:47.954 Running test: JIT/Regression/JitBlue/Runtime_115532/Runtime_115532/Runtime_115532.dll
* Assertion at /Users/runner/work/1/s/src/mono/mono/mini/mini-amd64.c:7783, condition `ins->inst_c0 == 1' not met
This should be a relatively simple thing to handle and I'll take a look. CC. @lewing since you've done several of the recent Mono SIMD PRs
|
/azp run runtime-coreclr jitstress-isas-x86, Fuzzlyn, Antigen, runtime-coreclr jitstress, runtime-coreclr jitstressregs |
|
Azure Pipelines successfully started running 5 pipeline(s). |
This was referenced May 17, 2025
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolves #115532
Resolves #115487
Resolves #115459
Resolves #115644
These were introduced with #115348