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

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added a comment.

@reames I appreciate your suggestion.

It looks your suggestion with existing SCEV logic is more general. Once I update this patch with your suggestion, I will let you and @mkazantsev know.

P.S I am feeling unwell after getting covid vaccination jab so it could take some time to implement your suggestion. Please understand it.



================
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);
----------------
mkazantsev wrote:
> 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.
Ah, Thanks for letting me know. I will update it.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:239
+  // Assumed same signedness.
+  if (ExitingCond.HasNoSignedWrap != SplitCandidateCond.HasNoSignedWrap)
+    return false;
----------------
mkazantsev wrote:
> 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. :) 
Yep, I will update it.


================
Comment at: llvm/lib/Transforms/Scalar/SimpleLoopBoundSplit.cpp:263
+      for (auto &Inst : *Succ) {
+        if (isa<LoadInst>(Inst) || isa<StoreInst>(Inst))
+          return true;
----------------
mkazantsev wrote:
> 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.
Ah, Thanks for sharing the info.


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

https://reviews.llvm.org/D102234