-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Remove bounds checks for a[i+c1] followed by a[i+c2] #112532
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
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
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.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (2)
- src/coreclr/jit/assertionprop.cpp: Language not supported
- src/coreclr/jit/valuenum.cpp: Language not supported
PTAL @jakobbotsch @AndyAyersMS @dotnet/jit-contrib Diffs - mostly just removed bounds checks. |
// Order arg0 arg1 by numerical VN value. | ||
if (arg0VN > arg1VN) | ||
// Order arg0 arg1 by numerical VN value, but keep constant non-handle VNs on the right. | ||
if ((arg0VN > arg1VN) || IsVNConstantNonHandle(arg0VN)) |
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.
Previous code that was swapping VN operands in a commutative binary func could move constants to the left side which is not desired. Not a correctness issue, but a bit annoying for downstream phases
if ((currentIdx == assertedFuncApp.m_args[0]) && (currentOffset >= 0) && | ||
(assertedOffset >= currentOffset)) | ||
{ | ||
isRedundant = true; | ||
} |
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.
Is there anything to worry about with regards to overflow here? E.g. if i + cns1
does not overflow yet i + cns2
does
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.
Or, perhaps something like i = -5, cns1 = 5, cns2 = 4
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 ah, we need to also check the index for being non-negative (via assertions), the diffs are a lot smaller if I check that. I guess #112595 will handle this
Not entirely a proper fix for #112524, but still helps. Basically, if we have an assertion that "(i + CNS1)" is within bounds, then any "(i + CNS2)" is within the same bounds if CNS2 is in [0..CNS1] range.
Was:
Now: