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

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10483
+    } else if (Pred == ICmpInst::ICMP_SLT && isa<SCEVAddRecExpr>(FoundLHS))
+      FoundCandidate = true;
+
----------------
nikic wrote:
> 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?
> 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)) {
> ```

You are right.
 
> 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?

It is just conservative check. If AddRrecs is always canonicalized to the left-hand side, we do not need check FoundRHS. From below unit tests, when I call the `isImpliedCond` directly, I was able to see the FoundRHS is AddRec. If it is not problem, I will remove above code.


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10495
+            return true;
+          return false;
+        };
----------------
nikic wrote:
> You can just return the isImpliedCondBalancedTypes result here, no need for separate return true / return false.
You are right! I will update it.


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


================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:10470
+             !StepCst->getValue()->isNegative()) ||
+            AddRec->getNoWrapFlags(SCEV::FlagNUW)) {
+          const auto *NewFoundLHS = cast<SCEVAddRecExpr>(FoundLHS)->getStart();
----------------
nikic wrote:
> 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.
Yep, I will add a unit test which is named with `ImpliedCondWithAddRecNUW`. You can see it on the unit test. If the unit test is wrong for unsigned, please let me know. 


================
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
----------------
nikic wrote:
> 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.
Yep, you are right. Next time, I will be careful. This test aimed to show people the impact of IRCE pass with this SCEV change. I have started the discussion of IRCE pass so I would like to remove this test from this SCEV change now. I am sorry for inconvenience.


================
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 "
----------------
nikic wrote:
> We should also have a negative test without nsw flag.
Yep, I will add it.


================
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));
----------------
nikic wrote:
> More typically spelled `ICmp`...
Yep, I will update it.


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

https://reviews.llvm.org/D100566