-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Use TryGetRangeFromAssertions to fold more relops in global assert prop #112824
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
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 4 out of 4 changed files in this pull request and generated no comments.
|
/azp run runtime-coreclr libraries-pgo, runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
/azp run runtime-coreclr libraries-pgo, runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn |
|
Azure Pipelines successfully started running 4 pipeline(s). |
|
PTAL @jakobbotsch @dotnet/jit-contrib diffs I ended up removing SSA-based for now for two reasons:
I'll try again separately, SSA-based one increases diffs significantly (up to -250k for win-x64) Fuzzlyn's failures repro on Main too, although, looks like some of them are my fault (previous PRs) - I'm looking now |
AndyAyersMS
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.
Can you update the description to describe what this does now?
src/coreclr/jit/assertionprop.cpp
Outdated
| if (tree->OperIsCmpCompare() && | ||
| // We can also use a more powerful SSA-based TryGetRange, but it's too expensive to call for every relop. | ||
| // Hence, the following checks are driven by TP/CQ balance: | ||
| op1->TypeIs(TYP_INT) && op2->IsIntCnsFitsInI32() && !op2->IsIntegralConst(0)) |
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.
Could this use op2's VN instead relying on it being a literal constant?
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.
Sure, I just assumed that we always propagate constants by value before global assertprop so while we generally should rely on VNs, for constants it's more or less fine 🙂 but in the sake of consistency I guess I can
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.
@AndyAyersMS I've addressed your feedback in 877112c and updated the issue description
This PR does:
optAssertionPropGlobal_RelOpwith help ofRangeCheck::TryGetRangeFromAssertions(it's not too expensive). For now, it only handles "Op1 relop IntCns". For that, I introducedEvalRelopthat can answer questions about two ranges (e.g.range1 is >= range2, etc)MergeEdgeAssertionsa bit:NO_THROWassertions we can also check if our vn equals lenVN - this gives us a hint that lenVN is >= indexVN (in case of constant)DoesOverflowto be less conservative for full constant rangesOne note that
RangeCheck::TryGetRangeFromAssertionsis more or less cheap - it relies only on assertions. There is alsoRangeCheck::TryGetRangethat is SSA-based - it finds a lot more places to optimize, with it, I see -250k diffs. Unfortunately, we have to pay with up to +1% TP for them. I want to invest some time in improving TP in RangeCheck to see if I can get 80% of th e diffs for a small priceExample of an improved pattern:
Diffs