[PATCH] D90926: [SCEV] Don't use not expressions for implied conditions

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10135
+                                   Context);
+    if (!isa<SCEVConstant>(FoundRHS) && !isa<SCEVAddRecExpr>(FoundLHS))
       return isImpliedCondOperands(Pred, LHS, RHS, FoundRHS, FoundLHS, Context);
----------------
mkazantsev wrote:
> I wonder, is AddRec the only case where we expect constant to be on right hand side? I'm pretty sure we want it for most arithmetics we process. Do we regress if `SCEVAddRecExpr` is removed? This should give even better compile time effect.
We canonicalize addrec to left and constant to right, so this only does the swap if *neither* is present.


================
Comment at: llvm/unittests/Analysis/ScalarEvolutionTest.cpp:1405
+    const SCEV *AddRec_0_1 = SE.getSCEV(IV);
+    // {0,+,-1}<nw>
+    const SCEV *AddRec_0_N1 = SE.getNegativeSCEV(AddRec_0_1);
----------------
mkazantsev wrote:
> Nit: this thing should have `<nsw>`, and the fact being proved is actually not true without this flag.
The `<nw>` here is based on the flags present in `dump()`. I think it doesn't actually matter because the implication we're effectively proving is that `{0,+,1}<nuw><nsw> > 0` implies `{-1,+,1} > -1`. And for this part, just the nowrap flag on the left addrec are sufficient, the ones on the rhs are implied.


================
Comment at: llvm/unittests/Analysis/ScalarEvolutionTest.cpp:1411
+                                  ICmpInst::ICMP_SGT, AddRec_0_1, Zero));
+    // {0,+,-1}<nw> < -1  =>  {0,+,1}<nuw><nsw> > 0
+    EXPECT_TRUE(isImpliedCond(SE, ICmpInst::ICMP_SGT, AddRec_0_1, Zero,
----------------
mkazantsev wrote:
> I think it can be strengthened to `{0,+,1}<nuw><nsw> > 1`, no?
We should be able to prove that, but don't. After negation, we'll try both `{0,+,-1}<nw> < -1 ->  {-1,+,-1}<nw> < -2` and `{-1,+,1}<nsw> > 0  ->  {0,+,1}<nuw><nsw> > 1`, but can't prove either.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90926/new/

https://reviews.llvm.org/D90926