-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[release/8.0-staging] JIT: Track sideness of arrOp in GetCheckedBoundArithInfo #100968
[release/8.0-staging] JIT: Track sideness of arrOp in GetCheckedBoundArithInfo #100968
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
@@ -3066,8 +3066,11 @@ class CSE_Heuristic | |||
|
|||
assert(vnStore->IsVNCompareCheckedBoundArith(oldCmpVN)); | |||
vnStore->GetCompareCheckedBoundArithInfo(oldCmpVN, &info); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the other uses of this function? Do they need to be changed too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jakobbotsch I've inspected all uses and didn't find places where we might make wrong decisions due to order of operands (the only use of arrOp
is this CSE where we compose a new VN and in rangecheck)
PTAL @AndyAyersMS @jakobbotsch (backport of #100848) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. we will take for consideration in 8.0.x
Fixes #100809
Backport of #100848 to release/8.0-staging
Customer Impact
Reported in #100809, there is a very specific pattern that is incorrectly lowered by JIT into an invalid code. Unfortunately, there is no good workaround for it we can recommend + the pattern in question lives in the BCL so users can't change it. The minimal repro to hit the bug is:
Prints
True
in Release andFalse
in Debug. The actual bug was introduced many releases ago, but it started to manifest only since .NET 8.0 because of an unrelated optimization (#81998) that exposed it.Regression
Regressed in .NET 8.0
Testing
Added a test
Risk
Low: it requires a very specific code pattern to hit