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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10124
+    // We can write the implication
+    // 0.  LHS Pred      RHS  <=   FoundLHS SwapPred  FoundRHS
+    // using one of the following ways:
----------------
nit: please replace `<=` with `<-` to make it crystal clear that it's implication and not `le` :)


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10135
+                                   Context);
+    if (!isa<SCEVConstant>(FoundRHS) && !isa<SCEVAddRecExpr>(FoundLHS))
       return isImpliedCondOperands(Pred, LHS, RHS, FoundRHS, FoundLHS, Context);
----------------
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.


================
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);
----------------
Nit: this thing should have `<nsw>`, and the fact being proved is actually not true without this flag.


================
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,
----------------
I think it can be strengthened to `{0,+,1}<nuw><nsw> > 1`, no?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90926