[PATCH] D109354: [LoopBoundSplit] Check the split condition with its AddRec start value
JinGu Kang via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Sep 8 04:04:55 PDT 2021
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
More information about the llvm-commits
mailing list