[PATCH] D109354: [LoopBoundSplit] Check the split condition with its AddRec start value

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:273
+    const SCEV *SplitAddRecStartSCEV =
+        dyn_cast<SCEVAddRecExpr>(SplitCandidateCond.AddRecSCEV)->getStart();
+    if (SE.isKnownPredicate(
----------------
mkazantsev wrote:
> This should either be `cast` instead of `dyn_cast` (if it never fails), or there should be a null check (if it may fail).
Sorry, let me update it with cast.


================
Comment at: llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp:274
+        dyn_cast<SCEVAddRecExpr>(SplitCandidateCond.AddRecSCEV)->getStart();
+    if (SE.isKnownPredicate(
+            ICmpInst::getInversePredicate(SplitCandidateCond.Pred),
----------------
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.


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

https://reviews.llvm.org/D109354