This is an archive of the discontinued LLVM Phabricator instance.

[IRCE] Enable decreasing loops of variable bounds
ClosedPublic

Authored by samparker on Mar 22 2018, 4:00 AM.

Details

Summary

As a part 2 to my previous patch, this updates the logic for the decreasing safety checks in a similar manner:

  • CanBeMax is replaced by CannotBeMaxInLoop which queries isLoopEntryGuardedByCond on the maximum value.
  • SumCanReachMin is replaced by isSafeDecreasingBound which includes some logic from parseLoopStructure and, again, has been updated to use isLoopEntryGuardedByCond on the given bounds.

Diff Detail

Repository
rL LLVM

Event Timeline

samparker created this revision.Mar 22 2018, 4:00 AM

Under variable bounds, do you mean somethign that varies in the loop or just a non-constant?

mkazantsev added inline comments.Mar 23 2018, 12:31 AM
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
697 ↗(On Diff #139425)

BTW, this (and same for min) look general enough to be moved to Scalar Evolution analysis. Please consider making this in future, maybe someone else will need them. For now, I'm OK with what we have here.

716 ↗(On Diff #139425)

Just a general observation (this problem seems to exist in old code as well): if the predicate is unsigned, the loop cannot be decreasing. In unsigned, all values are seen as non-negative. So this logic makes no sense. Could you please check that we only come here with signed predicates? If so, please add an assert on that.

718 ↗(On Diff #139425)

Assert that step is known negative?

samparker added inline comments.Mar 26 2018, 3:37 AM
lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
716 ↗(On Diff #139425)

But if we know that the limit is a non negative, why couldn't an unsigned comparison be used? test_02 in decrementing-loop.ll does just this. If sub can legally handle unsigned values, I can't see why we wouldn't be able to use unsigned compares in this case.

mkazantsev accepted this revision.Mar 26 2018, 10:31 PM

LGTM

lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
716 ↗(On Diff #139425)

Ok, it makes sense (yet at the first glance this confused me slightly :) ). We can always legally replace sgt with ugt if we prove non-negativity of both, so I'm fine with that.

This revision is now accepted and ready to land.Mar 26 2018, 10:31 PM
This revision was automatically updated to reflect the committed changes.