[PATCH] D100566: [SCEV] Add a ad-hoc pattern on isImpliedCondBalancedTypes

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10483
+    } else if (Pred == ICmpInst::ICMP_SLT && isa<SCEVAddRecExpr>(FoundLHS))
+      FoundCandidate = true;
+
----------------
As far as I can tell, the actual Pred is not relevant -- this should be correct independently of the used Pred, right? If so, I think it would be better to drop those checks and do something like this:

```
    if (isa<SCEVAddRecExpr>(FoundRHS) && !isa<SCEVAddRecExpr>(FoundLHS)) {
      std::swap(FoundLHS, FoundRHS);
      std::swap(LHS, RHS);
      Pred = ICmpInst::getSwappedPredicate(Pred);
    }

    if (const auto *AddRec = dyn_cast<SCEVAddRecExpr>(FoundLHS)) {
```

Though I'm also wondering why we need to handle the case of AddRec in FoundRHS -- normally, AddRecs are canonicalized to the left-hand side. What is breaking the canonicalization here?


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10495
+            return true;
+          return false;
+        };
----------------
You can just return the isImpliedCondBalancedTypes result here, no need for separate return true / return false.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10497
+        };
+        if ((AddRec->getNoWrapFlags(SCEV::FlagNSW) &&
+             !StepCst->getValue()->isNegative())) {
----------------
`AddRec->hasNoSignedWrap()` would be more idiomatic.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10470
+             !StepCst->getValue()->isNegative()) ||
+            AddRec->getNoWrapFlags(SCEV::FlagNUW)) {
+          const auto *NewFoundLHS = cast<SCEVAddRecExpr>(FoundLHS)->getStart();
----------------
jaykang10 wrote:
> nikic wrote:
> > This still isn't right: the required nowrap flag depends on the predicate. If you have `NewFoundPred = ICMP_SLE`, then you need nsw. If you have `NewFoundPred = ICMP_ULE`, then you need nuw.
> Oops! sorry, you are right! It looks the unsigned predicate with constant is transformed with `SimplifyICmpOperands` earlier. Let's just handle signed case. 
Can you please add a test for the unsigned case? Looking at SimplifyICmpOperands, it's not really clear to me why it would already be handled.


================
Comment at: llvm/test/Transforms/IRCE/sibling_loops.ll:10
+define dso_local void @test() local_unnamed_addr #0 {
+  %1 = zext i32 undef to i64
+  br label %2
----------------
This test is over-reduced. It's best to avoid undef values, as it's very easy for tests to get broken by unrelated changes.


================
Comment at: llvm/unittests/Analysis/ScalarEvolutionTest.cpp:1426
+                          "  %iv = phi i32 [ 1, %entry], [%iv.next, %loop] "
+                          "  %iv.next = add nsw i32 %iv, 1 "
+                          "  %cmp = icmp eq i32 %iv, %len "
----------------
We should also have a negative test without nsw flag.


================
Comment at: llvm/unittests/Analysis/ScalarEvolutionTest.cpp:1473
+  runWithSE(*M, "foo", [](Function &F, LoopInfo &LI, ScalarEvolution &SE) {
+    ICmpInst *ICMP = cast<ICmpInst>(getInstructionByName(F, "cmp"));
+    const SCEV *FoundLHS = SE.getSCEV(ICMP->getOperand(0));
----------------
More typically spelled `ICmp`...


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

https://reviews.llvm.org/D100566