[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:
> > 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.
> 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.
Fine for me!


https://reviews.llvm.org/D44515