[PATCH] D109354: [LoopBoundSplit] Check the start value of split cond AddRec

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:274
+        dyn_cast<SCEVAddRecExpr>(SplitCandidateCond.AddRecSCEV)->getStart();
+    if (SE.isKnownPredicate(
+            ICmpInst::getInversePredicate(SplitCandidateCond.Pred),
----------------
mkazantsev wrote:
> jaykang10 wrote:
> > mkazantsev wrote:
> > > And what if both direct and inverted predicates are not known? It may be false, but SCEV is just not smart enough to prove it.
> > > 
> > > I think the correct (conservative) check should be like
> > > 
> > > ```
> > > if (!isKnownPredicate(SplitCandidateCond.Pred, SplitAddRecStartSCEV, SplitCandidateCond.BoundSCEV))
> > >   continue;
> > > ```
> > um... after using `SplitCandidateCond.Pred` directly instead of `getInversePredicate`, the existing tests are failed... and I feel this patch's idea is wrong... Let's see an example again.
> > ```
> > define void @split_loop_bound_inc_with_sgt(i64 %a, i64* noalias %src, i64* noalias %dst, i64 %n) {
> > loop.ph:
> >   br label %loop
> > 
> > loop:
> >   %iv = phi i64 [ %inc, %for.inc ], [ 0, %loop.ph ]
> >   %cmp = icmp slt i64 %iv, %a
> >   br i1 %cmp, label %if.then, label %if.else
> > 
> > if.then:
> >   %src.arrayidx = getelementptr inbounds i64, i64* %src, i64 %iv 
> >   %val = load i64, i64* %src.arrayidx
> >   %dst.arrayidx = getelementptr inbounds i64, i64* %dst, i64 %iv 
> >   store i64 %val, i64* %dst.arrayidx
> >   br label %for.inc
> > 
> > if.else:
> >   br label %for.inc
> > 
> > for.inc:
> >   %inc = add nuw nsw i64 %iv, 1
> >   %cond = icmp sgt i64 %inc, %n
> >   br i1 %cond, label %exit, label %loop
> > 
> > exit:
> >   ret void
> > }
> > ```
> > The split cond's AddRec is `{0,+,1}<nuw><nsw><%loop>` and the exiting cond's AddRec is `{1,+,1}<nuw><nsw><%loop>`. The %loop's iteration follows exiting cond's AddRec. It means that the start value of loop is `1`. After transformation, we assume the split condition of the pre-loop is always `true`. In order to guarantee it, we need to check the start value of the split cond AddRec is less than the exiting cond one. For example, let's say the start value of the split cond AddRec is `3` and the exiting cond one is `1`. In this case, the split condition is false at iteration 1 and 2 of pre-loop.
> > 
> > I think it is a correct way to fix the bug. I am sorry for wrong patch... Let me update patch with this logic again. If you feel something wrong, please let me know.
> I don't quite get it, why is it ULE now regardless of the original predicate?
Sorry... It was wrong. I am trying to update code following my last comment.


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

https://reviews.llvm.org/D109354