JIT: remove unsound NOT(relop) VN simplification#129097
Merged
Merged
Conversation
VNForFunc was rewriting NOT(relop(x,y)) to Reverse(relop)(x,y). GT_NOT is bitwise complement, not logical negation: for a 0/1 relop result, `~v` produces -1 or -2, not the 1 or 0 that the reversed relop would, so downstream arithmetic uses of the complemented value folded the wrong way. Fixes dotnet#129076.
github-actions
Bot
added
the
area-CodeGen-coreclr
Contributor
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Member
Author
|
@dotnet/jit-contrib PTAL |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes a value-numbering (VN) simplification in CoreCLR JIT where NOT(relop(x,y)) could be rewritten to the reversed relational operator, and adds a JIT regression test intended to cover the resulting miscompile scenario.
Changes:
- Remove the
VNF_NOT+VNFuncIsComparisonrewrite inValueNumStore::VNForFunc. - Add a new JitBlue regression test (
Runtime_129076) exercising~(relop)used in downstream arithmetic comparisons. - Add a minimal test
.csprojto build the new regression.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/valuenum.cpp | Removes the NOT(relop) -> Reverse(relop) VN fold while keeping NOT(NOT(x)) -> x. |
| src/tests/JIT/Regression/JitBlue/Runtime_129076/Runtime_129076.csproj | New test project for the regression. |
| src/tests/JIT/Regression/JitBlue/Runtime_129076/Runtime_129076.cs | New regression test reproducer validating correct behavior. |
jakobbotsch
reviewed
Jun 7, 2026
Drop the standalone .csproj and add the .cs to Regression_ro_2.csproj following the pattern of recent JitBlue tests (Runtime_128631, Runtime_128801). Standalone csprojs are only needed when the test requires its own process via CLRTestEnvironmentVariable; ours doesn't. Verified: bash Regression_ro_2.sh invokes Runtime_129076.TestEntryPoint (passes on the fixed JIT, fails on the regressed JIT). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jakobbotsch
approved these changes
Jun 7, 2026
Member
Author
Yes, it was from #127181 (so ~6 weeks old) |
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.
Note
This PR was authored by GitHub Copilot CLI on @AndyAyersMS's machine.
VNForFunc was rewriting
NOT(relop(x,y))toReverse(relop)(x,y).GT_NOTis bitwise complement, not logical negation: for a 0/1 relop result,~vproduces -1 or -2, whileReverse(relop)produces 0 or 1, so downstream arithmetic uses of the complemented value (here~v3 >= -1in the repro) folded the wrong way.Removes the case; keeps
NOT(NOT(x)) ==> xwhich is correct.SuperPMI asmdiffs across all 10 osx-arm64 Checked collections (2,302,269 contexts, 871K MinOpts + 1.4M FullOpts): 0 diffs. The peephole never fires on real-world code.
Fixes #129076.