This is an archive of the discontinued LLVM Phabricator instance.

[IRCE] Change min value safety check
ClosedPublic

Authored by samparker on Mar 15 2018, 5:58 AM.

Details

Reviewers
sanjoy
mkazantsev
Summary

CanBeMin is currently used which will report true for any unknown values, but often a check is performed outside the loop which covers this situation:

for (int i = 0; i < N; ++i)
  ...

if (N > 0)
  for (int i = 0; i < N; ++i)
    ...

So I've add 'LoopGuardedAgainstMin' which reports whether the loop is guarded by a greater than the minimum check, which then allows loop with a variable count to be optimised. And also enable SGT to be converted to UGT, which has lead me to change one of the tests. Another one of the existing tests has been changed because I believe that the range metadata was preventing the test from performing as desired.

Diff Detail

Event Timeline

samparker created this revision.Mar 15 2018, 5:58 AM
samparker updated this revision to Diff 138539.Mar 15 2018, 6:03 AM

Added forgotten test.

mkazantsev added inline comments.Mar 16 2018, 5:04 AM
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
311

Please submit it as a separate patch.

705

Can we now remove this function and replace its usages with LoopGuardedAgainstMin? I don't mind if it goes as a follow-up change.

713

I would suggest calling it CannotBeMinInLoop and reorder args like S, L, Signed, SE.

878

Please submit it as a separate change.

908

Why do you need non-negativity here? In unsigned, "negative" only means that the first bit is 1 and the value is still seen as big positive.

951

Please submit it as a separate patch.

991

Do you plan to do the same for max value? I'm OK if it will be done in a separate patch.

1787

Please submit it as a separate patch.

1813

Same.

1847

Same.

test/Transforms/IRCE/eq_ne.ll
147

What is the reason of this change? If it demonstrates something new you can just create a new test identical to this one but without the range.

fhahn added a subscriber: fhahn.Mar 16 2018, 5:18 AM
fhahn added inline comments.
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
713

Maybe there's a more descriptive name for S, e.g. BoundSCEV or something.

samparker added inline comments.Mar 16 2018, 6:09 AM
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
705

I will have a look at the other uses.

908

Ah, guess I was trying to be overly cautious, FlagNUW will do.

991

Yes, I can do that.

test/Transforms/IRCE/eq_ne.ll
147

From the description, the existing test didn't seem like it was testing what it should have been. Given the range values, IRCE should be able to be performed and with this patch it does. So I wanted to change the test to match up with the description with IRCE being prevented.

samparker updated this revision to Diff 138693.Mar 16 2018, 6:55 AM

Thanks for the comments, I've made these changes:

  • CanBeMin has now been replaced with CannotBeMinInLoop.
  • The function arguments have been renamed and reordered.
  • KnownNonNegative check removed.
  • debug comments removed.
mkazantsev requested changes to this revision.Mar 18 2018, 10:04 PM
mkazantsev added inline comments.
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
940

This doesn't look correct. RightSCEV + Step - 1 = Max means that RightSCEV + Step exceeds Max. We should bail with SLT/UGT as well. Please correct me if I'm wrong.

And in any case, if you still think it should be done, it should go as a separate patch.

1474

{ } not needed here (yes, I hate this code style rule, too :)).

1502

Same.

1804

What was the reason of that?

test/Transforms/IRCE/eq_ne.ll
147

I would still prefer to not touch old tests for sake of this. Please create a new one, it doesn't hurt anyone and will help us to make sure that everything works OK if the range is there.

test/Transforms/IRCE/variable-loop-bounds.ll
14

Could you please rename it to %iv for better readability?

This revision now requires changes to proceed.Mar 18 2018, 10:04 PM
samparker updated this revision to Diff 138929.EditedMar 19 2018, 8:10 AM

I've replaced SumCanReachMax with SumCannotBeMaxInLoop, which uses similar logic to the new CannotBeMinInLoop, querying SCEV's LoopBackedgeGuardedBy. These two updated checks are need together to get this to work. Other changes:

  • Removed whitespace.
  • re-inserted the range metadata into test.
  • bracket style changes
mkazantsev requested changes to this revision.Mar 19 2018, 10:38 PM
mkazantsev added inline comments.
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
690

You are making this function for SCEVs regardless of their semantics, please rename BoundSCEV and StepMinusOne into something more general.

697

This is incorrect. You need to prove that BoundSCEV < Limit, so you should use SLT/ULT predicates, not SGT/ULT.

698

It is also potentially incorrect. You need to check this condition not on loop backedge but on loop entry. Otherwise it can be in theory wrong on 1st iteration. It is checked with isLoopEntryGuardedByCond.

This revision now requires changes to proceed.Mar 19 2018, 10:38 PM
mkazantsev added inline comments.Mar 19 2018, 10:39 PM
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
697

SLT/ULT predicates, not SGT/UGT**

samparker added inline comments.Mar 20 2018, 3:38 AM
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
697

We do, but as far as I can tell, this is called from where the exit successor is the 'true' successor, whereas we want to confirm the condition of the header being the 'true' successor.

For example, SumCannotBeMax could produce:
ICMP_UGT {-1, + %N}, -1

The original predicate would have been EQ and SCEV will inverse and simplify the predicate to target the header into:
ICMP_NE {1, +1}, %N

Which gets further simplified into:
ICMP_ULT {0, +1}, (-1 + %N)

Which is what you're suggesting. I find the logic in the parseLoopStructure hard to follow, so please tell me if I'm missing something here.

698

I think this is already handled on line 943.

mkazantsev added inline comments.Mar 20 2018, 4:46 AM
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
697

My point is that you are making a separate static function which can later be moved to some Utils file. It must not rely something that is true in that particular place where it is invoked now.

What I can read from this function's name is that it is a function that takes two SCEVs and returns true if it proves that sum of these two SCEVs does not reach signed/unsigned max. And this is not in agreement with what is happening inside. If that is *not* what this function is supposed to do, then maybe it needs a better name.

698

Likewise, if you rely on something from outside your function then you should add an assert on it. If this function goes to utils and someone reuses it, it can be not obvious for them.

mkazantsev added inline comments.Mar 20 2018, 4:48 AM
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
698

And clearly, I don't see why you should check guard on backedge and not on entry. Any arguments in favour of that?

samparker added inline comments.Mar 20 2018, 4:53 AM
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
697

Okay, I see your point. I will have a go at extracting some functionality from parseLoopStructure and into a combined helper function that could be used in a standalone fashion.

mkazantsev added inline comments.Mar 20 2018, 4:54 AM
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
697

Fine for me!

samparker updated this revision to Diff 139126.Mar 20 2018, 8:03 AM

Thanks Max, I've now:

  • corrected the loop guard logic, including using the entry instead of backedge guard.
  • extracted the bound checking into one function, merging the Max check into it.
samparker updated this revision to Diff 139143.Mar 20 2018, 9:27 AM

Slight format change and added AvailableAtLoopEntry check in CannotBeMinInLoop.

Code looks fine with minor nits inlined. Please add more tests, I want to be adamant that it also works for other latch predicates.

lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
688–689

I think I got what this function does, but do you mind adding an explanatory comment since it is not completely trivial?

690

Please rename S1 to something more sound, it is unclear how it is connected to Start and Step. I propose Bound or something of this nature.

708–709

Assert that LatchBrExitIdx == 0 at this point.

713

Please clang-format this.

716

Availabllity of S1 can be checked in very beginning since you check it for both LatchBrExitIdx == 0 and 1. I also think that this can actually be an assert, but not sure about the latter.

739

This maybe also can be an assert, but not 100% sure about it. It's up to you.

test/Transforms/IRCE/variable-loop-bounds.ll
38

Do you mind adding similar tests with equivalent exit conditions defined by different predicates? (i.e. ne, slt, ult etc)

samparker updated this revision to Diff 139267.Mar 21 2018, 3:12 AM

Hi Max,

I've left the AvailableAtLoopEntry as conditionals and not asserts, as it seems safer at the moment. Things done:

  • added more tests
  • renamed the argument
  • added an assert for LatchBrExit == 0
  • exit early, only checking once, that AvailableAtLoopEntry is true
  • added debug messages into the new function
mkazantsev accepted this revision.Mar 21 2018, 10:32 PM

LGTM with nit inlined.

lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
704

This is usually done as DEBUG(dbgs() << "irce: Start: " << *Start << '\n');

This revision is now accepted and ready to land.Mar 21 2018, 10:32 PM
mkazantsev added inline comments.Mar 21 2018, 10:39 PM
test/Transforms/IRCE/eq_ne.ll
140

Please add checks for positive test just like in test_03. Also do the same for your new tests.

Thanks for the review and your help. I'll make the changes and commit at the start of next week. I'll also prepare the patch for the decreasing case.