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

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


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


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


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


================
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))))
----------------
mkazantsev wrote:
> How about checking `isSCEVableType` for comparison arguments right here?
Yep, I will add checks with `isSCEVable(Type)` for the comparison arguments.


================
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()))
----------------
mkazantsev wrote:
> 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`.
Yep, I will update it.


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


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

https://reviews.llvm.org/D102234