-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fix range check upper limit. #198
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
|
PTAL @dotnet/jit-contrib |
`monIncreasing` can be used only to liberally assume the lower bound.
| // | ||
| // **Step 5. If monotonic is true, then perform a widening step, where we assume, the | ||
| // SSA variables that are "dependent" get their values from the definitions in the | ||
| // **Step 5. If monotonic increasing is true, then perform a widening step, where we assume, the |
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.
Nit from pre-existing comment: I think this really should be called a "narrowing step" as (dep,dep) abstractly describes a wider set of values than (0,dep).
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.
In classic compiler books that step is called "widening" (or widening operator) because it also sets the upper limit to +INF.
I do not see any immediate benefits if we start replacing (dep) with (inf) right now. However, I think it is better to keep the preexisting name so people can find the articles that the algorithm was based on.
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.
so people can find the articles that the algorithm was based on.
Perhaps we could mention those articles in a comment? I am curious to learn if range check is actually based on any sort of analysis formalism.
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.
It is hard for me to find the first article that describes that part of the interval analysis. A good example that is available in preview is in "Compiler Design: Analysis and Transformation, Helmut Seidl, Reinhard Wilhelm, Sebastian Hack" . There are earlier articles that have more mathematical formalism but they are far from our implementation.
I have used a wrong exception type when copied the test. I have checked that it fails without the fix on x86 and passes with it.
|
@erozenfeld @AndyAyersMS could you please take another look? I would like to merge it, so I could delete the fork that it uses and start to publish new PRs. |
erozenfeld
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.
LGTM
There was an issue with range calculations in a rare loop form that was exposed by
complus_JitStressBBProf=1in https://github.com/dotnet/coreclr/issues/27805.I have added a test that exposes the issue without any stress mode but it relies on the assertion prop optimization that I have added in #160. New test failures.
In the new test case,
fgOptimizeBranchis blocked by different try regions and we can't copy loop condition as we usually do.So we keep our original order:
Note: usually we tranform it into:
so with the original order when we try to optimize bounds check for
arr[i]we have two phi nodes, one isi0=(0, 0)that is the pre loop value and another isi1=(dependent, dependent)that is loop variable:so
i1asks for a range ofi1andi2. When it asks fori2it tries to use assertion propagation info, but there is no info abouti2there. So we end up withi0=(0, 0), i2=(dependent, dependent)and the old code was merging it intoi1=(0,0)that allowed the compiler to incorrectly remove the bounds check.In the case where we don't have different try regions and create a loop with postcondition assertion propagation has information about
i2and uses it to modifyi2range as(dependent, 100)soi1 = (0, 100).complus_JitStressBBProf=1was blocking that cloning and exposed the issue on many test cases in that file.The fix is to use
increasingMonotoningflag only to set the lower bound.Note: We can add the same optimization for decreasing iterators later, but they are probably very rare.
Fixes https://github.com/dotnet/coreclr/issues/27805.
Commits:
1860741: add a new test case to osr015.
4239890: Improve JitDump output for range checks.
9ef0ca3: Rename
monotonictomonIncreasing.396198f: Fix the bug.
monIncreasingcan be used only to liberally assume the lower bound.