[PATCH] D100566: [SCEV] Add a ad-hoc pattern on isImpliedCondBalancedTypes
Nikita Popov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 28 13:41:13 PDT 2021
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
More information about the llvm-commits
mailing list