[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:28
+
+using namespace llvm;
+using namespace llvm::PatternMatch;
----------------
mkazantsev wrote:
> namespace llvm {
Yep, I will update it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:88
+  // AddRec <= Bound  -->  AddRec < Bound + 1
+  if (Cond.Pred == ICmpInst::ICMP_ULE || Cond.Pred == ICmpInst::ICMP_SLE) {
+    if (IntegerType *BoundSCEVIntType =
----------------
mkazantsev wrote:
> nit: I'd suggest resturure it as 
> ```
> if (pred != ULE && pred != SLE)
>   return false;
> 
> ```
> 
> this will reduce the nest for the biggest code piece. Just an idea.
Yep, I will update it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:108
+    }
+  }
+  return false;
----------------
mkazantsev wrote:
> Please add a TODO to handle ICMP_NE/EQ (I guess it was in the earlier versions of this patch and still good to support, but not necessarily in this revision).
Yep, I will add ToDo for it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:131
+  // Allowed constant step.
+  if (!isa<SCEVConstant>(StepRecSCEV))
+    return false;
----------------
mkazantsev wrote:
> ```
> ConstantInt *StepCI = dyn_cast<SCEVConstant>(StepRecSCEV)->getValue();
> if (!StepCI || !StepCI->isPositive())
>   return false;
> ...
> ```
> 
It looked there is no `isPositive` in ConstantInt. I will update it with isNegative() and isZero(),


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:152
+  Value *LHS = nullptr;
+  Value *RHS = nullptr;
+  if (!match(BI, m_Br(m_ICmp(Pred, m_Value(LHS), m_Value(RHS)),
----------------
mkazantsev wrote:
> Could declare as `Value *LHS, *RHS;` Just an idea.
Yep, I will update it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:157
+
+  if (!SE.isSCEVable(LHS->getType()) || !SE.isSCEVable(RHS->getType()))
+    return false;
----------------
mkazantsev wrote:
> It's enough to check LHS type and assert on RHS type.
Yep, I will update it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:166
+
+static bool CanSplitLoopBound(Loop &L, LoopInfo &LI, DominatorTree &DT,
+                              ScalarEvolution &SE, ConditionInfo &Cond) {
----------------
mkazantsev wrote:
> canSplit...
sorry for inconvenient. I will update it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:167
+static bool CanSplitLoopBound(Loop &L, LoopInfo &LI, DominatorTree &DT,
+                              ScalarEvolution &SE, ConditionInfo &Cond) {
+  // Skip function with optsize.
----------------
mkazantsev wrote:
> Consider making params `const` where suitable.
Yep, let me try to add it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:217
+  if (BI->getParent() != L.getHeader())
+    return false;
+
----------------
mkazantsev wrote:
> Why is that needed?
Ah, I have tried to follow the below comment from @reames.

> For a profitability check, I'd start specifically with the case where the condition precisely splits a loop into two halves. e.g. for ... { if (C) { body1 } else { body2 }. This is the easiest to believe is generally profitable, and we can generalize the heuristic selection later.

For the condition precisely splits a loop into two halves, I have checked that the conditional branch is in header and the join point is latch. Let me remove them.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:221
+  BasicBlock *Succ0 = BI->getSuccessor(0);
+  BasicBlock *Succ1 = BI->getSuccessor(1);
+
----------------
mkazantsev wrote:
> This is still a very strict limitation I think. I can always split critical edges if you need it. I'm fine if you just add a TODO to consider it in the future.
Yep, I will remove the checks with single predecessor.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:244
+                                      ConditionInfo &ExitingCond,
+                                      ConditionInfo &SplitCandidateCond) {
+  for (auto *BB : L.blocks()) {
----------------
mkazantsev wrote:
> Consider marking params as `const` where suitable.
Yep, I will update it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:355
+  Instruction *OrigBI = PostLoopPreHeader->getTerminator();
+  ICmpInst::Predicate Pred = ICmpInst::ICMP_NE;
+  Value *Cond =
----------------
mkazantsev wrote:
> Two points here:
> 1. Functional concern. Will NE work ok for step other than 1?
> 2. `lt` generally gives more info to the opt than `ne` (at least because `lt` implies `ne`). Any reason for `ne` here?
I have tried to follow below comment from @reames.

> The "if (i != N)" is a loop guard and can be identified by getLoopGuardBranch.

I was also not sure whether `ne` is better than `lt` here...


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

https://reviews.llvm.org/D102234