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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a comment.

Thanks for the update, I will try to find some time to wrap my head about it within next few days.



================
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);
----------------
jaykang10 wrote:
> mkazantsev wrote:
> > I think this is a natural place where you could apply PatternMatch.
> Sorry... I am not sure how I can use PatternMatch to extract information from ICmp instruction... 
It will be something like
```
  ICmpInst::Predicate Pred;
  Value *LHS, RHS;
  match(condition, m_ICmp(Pred, m_Value(LHS), m_Value(RHS)))
```
You can grep examples of this in many places, just grep by "m_ICmp". It just simplifies code a lot.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:139
+        // by CalculateUpperBoundWithLT.
+        if (Cond.Pred != ICmpInst::ICMP_ULT && Cond.Pred != ICmpInst::ICMP_SLT)
+          return false;
----------------
jaykang10 wrote:
> mkazantsev wrote:
> > Check this before you construct SCEVs to save compile time.
> um... I think we need to check it after `CalculateUpperBoundWithLT` in order to check only `LT` condition... We could add more conversions of conditions like 'EQ` --> `LT', `LE' --> `LT` later. If I missed something, please let me know.
Ah ok, you're right.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:239
+  // Assumed same signedness.
+  if (ExitingCond.HasNoSignedWrap != SplitCandidateCond.HasNoSignedWrap)
+    return false;
----------------
jaykang10 wrote:
> mkazantsev wrote:
> > 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.
> You are right. It is supposed to be same. However, we can not compare the AddRec directly because the AddRec of exit cond is different with split candidate's one because `inc++` is followed by exit cond. The start value of AddRec is different as below.
> ```
> ExitCond AddRec
> {1,+,1}<nuw><nsw><%loop>
> 
> SplitCandidateCond AddRec
> {0,+,1}<nuw><nsw><%loop>
> ```
> I think we have checked the step of AddRec and need to check its start value and signedness more to say they are same AddRec. I forgot to check the start value. I will add the check. If I missed something or you feel something wrong, please let me know. 
Maybe we should assert the preconditions here to make sure we are doing the right thing. :) 


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:263
+      for (auto &Inst : *Succ) {
+        if (isa<LoadInst>(Inst) || isa<StoreInst>(Inst))
+          return true;
----------------
jaykang10 wrote:
> mkazantsev wrote:
> > `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.
> > `any_of`?
> > I believe we don't vectorize volatile load/stores (or do we?) Maybe check at least this.
> 
> Yep, I will update it.
> 
> > 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.
> 
> um... if the condition of the side exit is related to induction variable, we could handle it.
It's never possible to say. For example, it may be some call to a method that has its own iteration counter (not related to the IV) and still have very subtle dependence on it that is impossible to fugire out.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:273
+
+  // Now, we have a split candidate. Let's split loop bound with it as below.
+  //    +--------------------+
----------------
jaykang10 wrote:
> mkazantsev wrote:
> > It looks like it can be an utility function usable by other parts of LLVM. Consider factoring it out.
> Once the basic version of this pass is stabilized, I would consider it.
Fair enough!


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

https://reviews.llvm.org/D102234