-
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
Improve branch optimizer around implied relops #95234
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsShould help with cases like: void Test(int x)
{
if (x > 100)
if (x > 10) // always true
Console.WriteLine();
} Ideally, it should be done in "redundant branch opt" phase, but it seems to be way more complicated so at least let's have it in assertprop for now.
|
ffa7d1a
to
3f82e12
Compare
/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, Fuzzlyn |
Azure Pipelines successfully started running 3 pipeline(s). |
PTAL @AndyAyersMS @dotnet/jit-contrib Diffs (not too big but I have some follow-up ideas how to extend them) |
// BB4: | ||
// return; | ||
|
||
// Check whether the dominating compare being "false" implies the dominated compare is known |
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 reversal/negation here is a bit tricky, but I think I follow it.
Seems like there's a complementary case where you don't negate the upper relop and then may instead have canInferFromTrue
abilities?
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.
Yeah, basically there are two cases:
canInferFromFalse
:
if (x > 100)
{
if (x > 10)
{
...
}
}
canInferFromTrue
:
if (x > 100)
{
...
}
if (x > 10)
{
...
}
I'll check separately the 2nd case (last I tried it added very little diffs)
How many of those diffs represent places code itself should be cleaned up? Like your example. |
I inspected a few diffs and they were inlining-related so I wasn't able to find an obvious case that can be cleaned up in C# |
Hm.. arm failures look related and they're floating point related... soft floats? |
…1 into implied-relop-assertprop
Since the last review I found a small bug - I replaced |
Should help with cases like:
Ideally, it should be done in "redundant branch opt" phase, but it seems to be way more complicated so at least let's have it in assertprop for now: EgorBo@cdbaac0