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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:47
+  /// Has been swapped LHS and RHS for AddRec and Bound
+  bool OpsSwapped;
+
----------------
Instead of this, consider flipping the predicate.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:76
+
+  auto CalculateUpperBound = [&L, &SE](ConditionInfo &Cond, bool IsExitCond) {
+    if (IsExitCond) {
----------------
General notion: all this code is very hard of read because of all lambdae inlined. Can they be separate functions?


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:97
+        unsigned BitWidth = BoundSCEVIntType->getBitWidth();
+        const SCEV *BoundPlusOneSCEV =
+            SE.getAddExpr(Cond.BoundSCEV, SE.getOne(BoundSCEVIntType));
----------------
You can sink this computation under the condition `bound < max`.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:157
+    BasicBlock *FalseSucc = nullptr;
+    if (!match(BI, m_Br(m_ICmp(Pred, m_Value(), m_Value()),
+                        m_BasicBlock(TrueSucc), m_BasicBlock(FalseSucc))))
----------------
How about checking `isSCEVableType` for comparison arguments right here?


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:238
+    // Allowed ICmp.
+    ICmpInst *ICmp = dyn_cast<ICmpInst>(BI->getCondition());
+    if (!ICmp || !isa<IntegerType>(ICmp->getOperand(0)->getType()))
----------------
1. It is already checked by `IsProcessableCondBI `, you can use `cast` instead of `dyn_cast`.
2. `isa<IntegerType>` --> `SE.isSCEVableType` and move it inside of `IsProcessableCondBI`.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:252
+    SplitCandidateCond.BI = BI;
+    FoundCandidate = true;
+    break;
----------------
That would be more natural to just return `BI` or `nullptr` from it (might also require function renaming).


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:278
+    BasicBlock *Succ1Succ = Succ1->getSingleSuccessor();
+    if (Succ0Succ != Succ1Succ)
+      return false;
----------------
What if both of them are nullptr?


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

https://reviews.llvm.org/D102234