[PATCH] D110060: [LoopBoundSplit] Handle the case in which exiting block is loop header

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:48
   /// AddRec SCEV
-  const SCEV *AddRecSCEV;
+  const SCEVAddRecExpr *AddRecSCEV;
   /// Bound SCEV
----------------
Please split out SCEV -> SCEVAddRecExpr  refactoring in a separate NFC patch. It can go before the others.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:66
+    const SCEV *BoundSCEV = SE.getSCEV(Cond.BoundValue);
+    const SCEVAddRecExpr *LHSAddRecSCEV = dyn_cast<SCEVAddRecExpr>(AddRecSCEV);
+    const SCEVAddRecExpr *RHSAddRecSCEV = dyn_cast<SCEVAddRecExpr>(BoundSCEV);
----------------
Cast not needed.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:69
     // Locate AddRec in LHSSCEV and Bound in RHSSCEV.
-    if (isa<SCEVAddRecExpr>(Cond.BoundSCEV) &&
-        !isa<SCEVAddRecExpr>(Cond.AddRecSCEV)) {
+    if (!LHSAddRecSCEV && RHSAddRecSCEV) {
       std::swap(Cond.AddRecValue, Cond.BoundValue);
----------------
I guess this is impossible after you've ensured that `AddRecSCEV` is an addrec actually. Or can it be null?


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:390
+    if (ExitingCond.NonPHIAddRecValue ==
+        PN.getIncomingValueForBlock(L.getLoopLatch()))
+      ExitingCondLCSSAPhi = LCSSAPhi;
----------------
What happens if there is more than one phi that will fit?


================
Comment at: llvm/test/Transforms/LoopBoundSplit/bug51866.ll:39
+; CHECK-NEXT:    [[I_0_SPLIT:%.*]] = phi i16 [ [[INC_SPLIT:%.*]], [[FOR_INC_SPLIT:%.*]] ], [ 5, [[FOR_COND_SPLIT_PREHEADER]] ]
+; CHECK-NEXT:    [[I_1_SPLIT:%.*]] = phi i16 [ [[INC_SPLIT]], [[FOR_INC_SPLIT]] ], [ 10, [[FOR_COND_SPLIT_PREHEADER]] ]
+; CHECK-NEXT:    [[EXITCOND_NOT_SPLIT:%.*]] = icmp eq i16 [[I_0_SPLIT]], 10
----------------
jaykang10 wrote:
> mkazantsev wrote:
> > jaykang10 wrote:
> > > mkazantsev wrote:
> > > > Why is it 10 again?
> > > Ah, you are right! I made a mistake. The incoming value from preheader in post-loop should be new bound. Let me update it.
> > And why is it 5?.. It should be 15, no?
> um... The `%i.1` is not AddRecExpr...
> ```
> for.cond: 
>   %i.0 = phi i16 [ 0, %entry ], [ %inc, %for.inc ]
>   %i.1 = phi i16 [ 10, %entry ], [ %inc, %for.inc ]
> ...
> for.inc:                                          ; preds = %if.else, %if.then
>   %inc = add nuw nsw i16 %i.0, 1
> ```
> The value of `%i.1` will be 10 at first iteration and it will follow `%inc` at next iterations. In this case, the start value of `%i.1` in post loop is 5. If `%i.1` is AddRecExpr as below, the start value will be 15.
> ``` 
> for.cond: 
>   %i.0 = phi i16 [ 0, %entry ], [ %inc, %for.inc ]
>   %i.1 = phi i16 [ 10, %entry ], [ %inc.1, %for.inc ]
> ...
> for.inc:                                          ; preds = %if.else, %if.then
>   %inc = add nuw nsw i16 %i.0, 1
>   %inc.1 = add nuw nsw i16 %i.1, 1
> ```
> I think you mention the case where the phi is AddRecExpr and the code is missing the case. Let me check and update the code. Thanks for the detailed review.
Got it, I misread this IR and didn't notice the entry block was `%inc`. Thanks for clarification.


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

https://reviews.llvm.org/D110060