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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev 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);
----------------
samparker wrote:
> 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.
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.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:698
+  auto Predicate = Signed ? ICmpInst::ICMP_SGT : ICmpInst::ICMP_UGT;
+  return SE.isLoopBackedgeGuardedByCond(L, Predicate, BoundSCEV, Limit);
 }
----------------
samparker wrote:
> 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.
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.



https://reviews.llvm.org/D44515