Validate store-coalescing constants before modifying IR - #133976
Merged
tannergooding merged 2 commits intoSep 21, 2026
Merged
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
github-actions
Bot
added
the
area-CodeGen-coreclr
|
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. |
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Add the required SDK configuration so the no-HW regression builds and runs.
Get a fresh assessment by requesting another Copilot review.
Review tier: Lite
Findings: 1
Open (1)
What changed in this PR
Fixes JIT store-coalescing corruption by validating constants before mutating IR.
Changes:
- Moves scalar and SIMD validation ahead of store rewriting.
- Adds
Vector2andVector64.CreateScalarregression coverage. - Adds a hardware-intrinsics-disabled test variant.
| File | Summary |
|---|---|
src/tests/JIT/Regression_2/Runtime_133746/Runtime_133746.csproj |
Configures the primary regression test. |
src/tests/JIT/Regression_2/Runtime_133746/Runtime_133746.cs |
Adds regression coverage for affected vector cases. |
src/tests/JIT/Regression_2/Runtime_133746/Runtime_133746_nohw.csproj |
Critical: lacks the root SDK configuration, so the no-HW regression cannot run. |
src/coreclr/jit/lower.cpp |
Validates constants before store removal and widening. |
This was referenced Sep 15, 2026
Closed
dhartglassMSFT
approved these changes
Sep 18, 2026
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
dhartglassMSFT
approved these changes
Sep 18, 2026
Member
Author
|
/backport to release/11.0 Note This backport request was posted by GitHub Copilot on behalf of @tannergooding. |
Contributor
|
Started backporting to |
4 tasks
tannergooding
added a commit
that referenced
this pull request
Sep 21, 2026
#134344) Backport of #133976 to `release/11.0`. Validates store-coalescing constants before removing the previous store or widening the current store. Unsupported combinations now retain their original stores instead of leaving partially modified IR. ## Customer Impact - [ ] Customer reported - [x] Found internally Incorrect JIT code generation can lose previously stored bytes or overwrite adjacent bytes with a sign-extended narrow constant. In #133746, `Vector64.CreateScalar((sbyte)-1)` produces `<-1, -1, ...>` instead of `<-1, 0, ...>` with `DOTNET_EnableHWIntrinsic=0` and `DOTNET_TieredCompilation=0`. The same faulty rejection path also affects a narrow write over a `Vector2` constant. Not taking the fix leaves silent incorrect results in affected optimized code. ## Regression - [x] Yes - [ ] No Introduced during .NET 11 development by #126562 (commit `2a03681857007feda3c1f82d946ba8848c7350e2`, merged April 27, 2026). That change added local-field store coalescing and fallible constant validation after IR mutation. The .NET 10 implementation does not have this local-field coalescing path or the post-mutation rejection checks; this is not a .NET 10 regression. ## Testing The original PR reports validation with a checked Windows x64 CoreCLR build, fixed JIT and cross-target JIT builds, both new regression variants, AVX2-disabled execution, and 16 existing struct-promotion tests. The `Vector2` regression fails against the baseline JIT and passes with the fix. Coverage also checks negative and boundary signed values for `Vector64.CreateScalar`, with normal and hardware-intrinsics-disabled configurations. These tests exercise the previously uncovered constant-rejection path that could leave a store transformation partially applied. Four existing coalescing-related methods had identical normalized disassembly before and after the fix (27, 55, 19, and 21 bytes). JIT formatting passed on the original change. The exact reported `CreateScalar` failure did not reproduce on the original validation host; ARM64 execution was not performed. This backport matches the original PR's patches, apart from JIT hunk line numbers. No additional local build or execution was performed for the release branch; backport CI is pending. ## Risk Low. The product change moves existing scalar constant extraction and SIMD validation ahead of destructive IR mutation, without adding a new optimization or changing the existing packing and atomicity logic. Unsupported combinations preserve the original stores. Regression coverage verifies the failure mode, and the original PR's code-generation comparisons found no changes in the four sampled supported cases. The remaining validation limitation is the lack of ARM64 execution; release-branch CI still needs to complete. **IMPORTANT**: If this backport is for a servicing release, please verify that: - For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`, not `release/X.0`. - For .NET 10+: The PR target branch is `release/X.0` (no `-staging` suffix). Verified target: `dotnet/runtime`, `release/11.0`. ## Package authoring no longer needed in .NET 9 **IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version. Keep in mind that we still need package authoring in .NET 8 and older versions. Resolves #133746 > [!NOTE] > This backport description was drafted by GitHub Copilot. Co-authored-by: Tanner Gooding <tagoo@outlook.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
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
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.
Fixes #133746.
LowerStoreCoalescingremoved the previous store and widened the current store before checking whether their constants could be combined. If constant extraction failed, lowering returned with the transformation only partially applied: the earlier bytes were lost and a narrow constant could be stored at the widened size without masking.Move scalar constant extraction and SIMD constant validation ahead of IR mutation. Unsupported combinations retain their original stores; supported combinations retain the existing packing and atomicity handling. This fixes the rejection path rather than special-casing
CreateScalaror disabling coalescing.Add regressions for a narrow write over a
Vector2constant and signedVector64.CreateScalarvalues, with normal and hardware-intrinsics-disabled configurations. On Windows x64, theVector2case reproduces the same invalid byte-to-long widening and fails against the baseline JIT; both variants pass with the fix. The exact reportedCreateScalarfailure did not reproduce on this host.Validation: checked Windows x64 CoreCLR, fixed JIT and cross-target JIT builds; both regression variants, AVX2-disabled execution, and 16 existing struct-promotion tests. Four existing coalescing-related methods have identical normalized disassembly before and after the fix (27, 55, 19, and 21 bytes respectively). JIT formatting passed. ARM64 execution was not performed.
Note
This PR description was drafted by GitHub Copilot.