- Notifications
You must be signed in to change notification settings - Fork5.2k
Jit: Remove bounds checks with tests against length.#40180
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
sandreenko commentedJul 31, 2020
Hi@nathan-moore, thanks for the change.
The master branch is currently blocked for feature changes but it won't be long (~few weeks) until we open in for post-5.0 PRs, so you could keep it as an open PR, I have marked it as 6.0 so we don't mix it with 5.0 fixes. |
556da3f to9c4c520CompareUh oh!
There was an error while loading.Please reload this page.
nathan-moore commentedSep 4, 2020
@sandreenko PTAL |
sandreenko commentedSep 9, 2020
PTAL @dotnet/jit-contrib |
sandreenko left a comment
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.
The change makes sense, thanks@nathan-moore , but I do not have enough expertise in this area to approve,@briansull could you please take a look?
I have left a few requests for comments for unclear parts, also expect outerloop/stress/jitstress testing for this change before the merge.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
briansull commentedSep 14, 2020
Taking a look at this |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
briansull left a comment
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.
I would like to see some small updates then I will approve.
Uh oh!
There was an error while loading.Please reload this page.
| limit =Limit(Limit::keConstant, info.constVal); | ||
| cmpOper = (genTreeOps)info.cmpOper; | ||
| } | ||
| // Current assertion is of the form i == 100 |
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.
Comment should read:
// Is the current assertion a valid constant int32 assertion?
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.
This is the pre-existing style for this else if chain. I can change them over, but I personally like having the form there.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
briansull left a comment
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.
Looks Good
am11 commentedOct 7, 2020
@briansull,@nathan-moore fyi: his keyword has closed the issue when PR was merged but the original repro in the issue persists (bound check elides with |
danmoseley commentedOct 16, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@briansull I happened to notice the question above wasn't answered. |
briansull commentedOct 16, 2020
We closed the issue#11623 when PR was merged I will reopen it. |
Uh oh!
There was an error while loading.Please reload this page.
Look through assertions when trying to get the array length if we don't have a better way.
This removes bounds checks in this form:
fixes#11623