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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

Looking much better now. Few more nits & a question regarding `ne`, which is a potential correctness concern.



================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:28
+
+using namespace llvm;
+using namespace llvm::PatternMatch;
----------------
namespace llvm {


================
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 =
----------------
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.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:108
+    }
+  }
+  return false;
----------------
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).


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:131
+  // Allowed constant step.
+  if (!isa<SCEVConstant>(StepRecSCEV))
+    return false;
----------------
```
ConstantInt *StepCI = dyn_cast<SCEVConstant>(StepRecSCEV)->getValue();
if (!StepCI || !StepCI->isPositive())
  return false;
...
```



================
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)),
----------------
Could declare as `Value *LHS, *RHS;` Just an idea.


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


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:166
+
+static bool CanSplitLoopBound(Loop &L, LoopInfo &LI, DominatorTree &DT,
+                              ScalarEvolution &SE, ConditionInfo &Cond) {
----------------
canSplit...


================
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.
----------------
Consider making params `const` where suitable.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:217
+  if (BI->getParent() != L.getHeader())
+    return false;
+
----------------
Why is that needed?


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:221
+  BasicBlock *Succ0 = BI->getSuccessor(0);
+  BasicBlock *Succ1 = BI->getSuccessor(1);
+
----------------
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.


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


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

https://reviews.llvm.org/D102234