-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Jit: Remove bounds checks with tests against length. #40180
Conversation
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
to
9c4c520
Compare
@sandreenko PTAL |
PTAL @dotnet/jit-contrib |
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.
src/tests/JIT/opt/AssertionPropagation/ArrBoundMinLength.csproj
Outdated
Show resolved
Hide resolved
Taking a look at this |
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.
@@ -627,6 +670,31 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP | |||
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.
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
@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 |
@briansull I happened to notice the question above wasn't answered. |
We closed the issue #11623 when PR was merged I will reopen it. |
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