-
Notifications
You must be signed in to change notification settings - Fork 5k
Bounds checks: make MergeEdgeAssertions more precise #113233
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
1772734
to
dc2cb16
Compare
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 reviewed 2 out of 2 changed files in this pull request and generated no comments.
PTAL @AndyAyersMS @jakobbotsch @dotnet/jit-contrib Diffs |
A bunch of hits on Regex: https://gist.github.com/MihuBot/f104c03eb53bf3c2f724d70a520636d1 |
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.
This method is getting pretty large/unwieldy, you might think about how it could be broken up.
I wonder if we have order dependencies elsewhere in AP, often we stop at the first assertion we find that tells us anything.
Would also be interesting to have a stress mode or something that just reverse the order of assertion iteration. If this is not zero diff then we have things to explore.
Yep, I plan a massive clean up (zero-diff) here and will inject stress modes. In this case diffs appeared because of the order e.g.: if X > 100 appears before "X > arrLen" we used to take "X > 100" and ignored "X > arrLen" Now we always prefer "X > arrLen" (but only if the 'preferredBound' was set) - we can play with this heuristic and e.g. accept 0/1 constants, but the diffs are mixed also, a few missing rules that I added |
This PR elides bounds checks in #113054 (closes #113054)
It turned out that MergeEdgeAssertions used to depend on order of assertions when it was tightening them so the behavior was different for tightening
BinOpArray
vsConstant
(whoever comes first). Now the logic is more sophisticated and doesn't depend on the order of assertions.