[PATCH] D44515: [IRCE] Change min value safety check

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:697
+  const SCEV *Limit = SE.getMinusSCEV(Max, StepMinusOne);
+  auto Predicate = Signed ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT;
+  return SE.isLoopBackedgeGuardedByCond(L, Predicate, BoundSCEV, Limit);
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > This is incorrect. You need to prove that `BoundSCEV < Limit`, so you should use SLT/ULT predicates, not SGT/ULT.
> SLT/ULT predicates, not SGT/UGT**
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.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:698
+  auto Predicate = Signed ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT;
+  return SE.isLoopBackedgeGuardedByCond(L, Predicate, BoundSCEV, Limit);
 }
----------------
mkazantsev wrote:
> 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`.
I think this is already handled on line 943.


https://reviews.llvm.org/D44515