[PATCH] D102234: [SimpleLoopBoundSplit] Split Bound of Loop which has conditional branch with IV

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added a comment.
This revision now requires changes to proceed.

That's amazing that this work has started after so many years talking about this! :) I've found some problems/bugs, but generally looks cool.



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:46
+  /// Signedness info
+  bool HasNoSignedWrap;
+
----------------
Why signed only? TODO unsigned?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:62
+    Cond.AddRecValue = ICmp->getOperand(0);
+    Cond.BoundValue = ICmp->getOperand(1);
+    Cond.AddRecSCEV = SE.getSCEV(Cond.AddRecValue);
----------------
I think this is a natural place where you could apply PatternMatch.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:72
+      Cond.OpsSwapped = true;
+    }
+  };
----------------
Assert that one of them is an addrec and another is available at loop entry?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:75
+
+  auto CalculateUpperBoundWithLT = [&SE](ICmpInst::Predicate &Pred,
+                                         const SCEVAddRecExpr *&AddRecSCEV,
----------------
It looks natural to encapsulate Pred, AddRec, Bound and nowrap flags into a structure (e.g. some `BoundInfo`) and pass `const BoundInfo &` here. WDYT?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:92
+    if (IsExitCond && Pred == ICmpInst::ICMP_EQ) {
+      Pred = AddRecSCEV->hasNoSignedWrap() ? ICmpInst::ICMP_SLT
+                                           : ICmpInst::ICMP_ULT;
----------------
1. Why do you prefer signed predicates over unsigned? Other parts of LLVM (e.g. indvars) tend to canonicalize everything as unsigned if they can prove it.
2. Isn't `AddRecSCEV->hasNoSignedWrap()` a same thing as `HasNoSignedWrap`? If so, remove one of them.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:102
+    //      Range                 Range
+    // AddRec <= Bound  -->  AddRec < Bound - 1
+    if (Pred == ICmpInst::ICMP_ULE || Pred == ICmpInst::ICMP_SLE) {
----------------
1. This is just wrong, it should be `AddRec < Bound + 1`.
2. It is only correct if you can prove that `Bound + 1` does not overflow. Simple exapmle is `X <= SINT_MAX` --> `X < SINT_MIN` is wrong.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:120
+
+        const SCEV *StepRecSCEV = AddRecSCEV->getStepRecurrence(SE);
+        // Allowed constant step.
----------------
Check `isAffine` before it to save compile time in case of really complex `AddRecs`.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:127
+        // Allowed positive step.
+        if (StepCI->isNegative())
+          return false;
----------------
I understand that zero step is impossible, but still, being negative and being non-positive is't the same thing is general. :)


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:131
+        // Allowed step is one.
+        if (!StepCI->isOne())
+          return false;
----------------
1. This check makes the check above meaningless.
2. Reading the code, I think the only place where it matters is  replacing `Eq` condition with `Lt`. Do we beed this for something else?
3. Please add a TODO to add negative step support. We've stuggled a lot with it in IndVars, and I see this pain coming again.



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:139
+        // by CalculateUpperBoundWithLT.
+        if (Cond.Pred != ICmpInst::ICMP_ULT && Cond.Pred != ICmpInst::ICMP_SLT)
+          return false;
----------------
Check this before you construct SCEVs to save compile time.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:160
+    // Check loop is in LCSSA form.
+    if (!L.isRecursivelyLCSSAForm(DT, LI))
+      return false;
----------------
It's innermost loop, so I thing "recursive" check is an overkill. Or do we plan to support outer loops for it?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:168
+    BasicBlock *ExitingBB = L.getExitingBlock();
+    // Assumed only one exiting block.
+    if (!ExitingBB)
----------------
This is a very restrictive thing. Why is it required?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:174
+    // Allowed only conditional branch.
+    if (!ExitingBI || !ExitingBI->isConditional() ||
+        isa<Constant>(ExitingBI->getCondition()) ||
----------------
Use pattern match.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:185
+    // Check the condition is processable.
+    if (!HasProcessableCondition(ICmp, Cond, true))
+      return false;
----------------
Pls add `/*IsExitCond*/`


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:220
+    if (FoundCandidate)
+      return false;
+
----------------
Logicaly, this check should go last. You may discard the candidate by conditions below.
Anyways, why is that a problem at all? If we have at least one candidate we can get rid of it and then proceed with the other ones.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:229
+    if (!HasProcessableCondition(ICmp, SplitCandidateCond, false))
+      return false;
+
----------------
Why not `continue`?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:232
+    SplitCandidateCond.BI = BI;
+    FoundCandidate = true;
+  }
----------------
Break?


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:239
+  // Assumed same signedness.
+  if (ExitingCond.HasNoSignedWrap != SplitCandidateCond.HasNoSignedWrap)
+    return false;
----------------
Do we ever check they are against the same AddRec?
If it's supposed to be same, I think we can just assert on it.
If not, I'm not catching how the rest of logic is going to work.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:244
+    // This transformation aims mainly to help loop vectorizer. If we can not
+    // vectorize the loop, it is not profitable.
+    if (!LAI.canVectorizeMemory())
----------------
I think it's a too strong statement. Getting rid of a branch in a critical loop may be useful even without vectorization. If you want to be that restrictive, place add an option to ignore this check.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:263
+      for (auto &Inst : *Succ) {
+        if (isa<LoadInst>(Inst) || isa<StoreInst>(Inst))
+          return true;
----------------
`any_of`?
I believe we don't vectorize volatile load/stores (or do we?) Maybe check at least this.
It looks like the transform is also profitable if one of these instructions is side exiting. We don't vectorize them so far, but if you get rid of it, the loop may become vectorizable.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:273
+
+  // Now, we have a split candidate. Let's split loop bound with it as below.
+  //    +--------------------+
----------------
It looks like it can be an utility function usable by other parts of LLVM. Consider factoring it out.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:337
+  const SCEV *NewBound1 =
+      SplitCandidateCond.HasNoSignedWrap
+          ? SE.getSMinExpr(ExitingCond.BoundSCEV, SplitCandidateCond.BoundSCEV)
----------------
Same question about preference of signed over unsigned. LLVM does otherwise in other parts. Let's not create more self-contradictions.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:411
+  simplifyLoop(&L, &DT, &LI, &SE, nullptr, nullptr, true);
+  formLCSSARecursively(*SecondLoop, DT, &LI, &SE);
+  simplifyLoop(SecondLoop, &DT, &LI, &SE, nullptr, nullptr, true);
----------------
This can be expensive. Can't we do a more surgical update?

I'm fine if it's a TODO with follow-up fix.



================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:435
+
+  assert(AR.DT.verify(DominatorTree::VerificationLevel::Fast));
+
----------------
Please verify loop info too.


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

https://reviews.llvm.org/D102234