This is an archive of the discontinued LLVM Phabricator instance.

[LoopBoundSplit] Check the start value of split cond AddRec
ClosedPublic

Authored by jaykang10 on Sep 7 2021, 5:00 AM.

Details

Summary

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 satisfies the split condition.

It is related to https://bugs.llvm.org/show_bug.cgi?id=51766

Diff Detail

Event Timeline

jaykang10 created this revision.Sep 7 2021, 5:00 AM
jaykang10 requested review of this revision.Sep 7 2021, 5:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 7 2021, 5:00 AM

Please precommit test to show impact of your patch.

jaykang10 updated this revision to Diff 371085.Sep 7 2021, 8:19 AM

Updated the test output against pre-committed test.

jaykang10 added a comment.EditedSep 7 2021, 8:20 AM

Please precommit test to show impact of your patch..

Yep, Done.

mkazantsev requested changes to this revision.Sep 7 2021, 11:45 PM
mkazantsev added inline comments.
llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp
273

This should either be cast instead of dyn_cast (if it never fails), or there should be a null check (if it may fail).

274

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;
This revision now requires changes to proceed.Sep 7 2021, 11:45 PM
jaykang10 added inline comments.Sep 8 2021, 4:04 AM
llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp
273

Sorry, let me update it with cast.

274

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.

jaykang10 updated this revision to Diff 371313.Sep 8 2021, 4:15 AM
jaykang10 retitled this revision from [LoopBoundSplit] Check the split condition with its AddRec start value to [LoopBoundSplit] Check the start value of split cond AddRec.
jaykang10 edited the summary of this revision. (Show Details)

Sorry... I was wrong again...

From the attached test to this patch, the AddRecSCEV of the split condition is {%qqq,+,2}<nw><%for.body> and the BoundSCEV is %qqq. The split condition %.not9 = icmp ult i16 %t2, %qqq is false at all iterations. In this case, we should not generate the pre-loop which has true split condition...

I am not sure there are passes to remove this conditional branch which has constant condition potentially at all iterations. For now, I will try to recognize this case and skip the transformation.

I have thought of the problem too complicated... I am sorry for wrong patch again... If you feel something wrong, please let me know.

mkazantsev added inline comments.Sep 8 2021, 10:14 PM
llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp
274

I don't quite get it, why is it ULE now regardless of the original predicate?

jaykang10 added inline comments.Sep 9 2021, 12:36 AM
llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp
274

Sorry... It was wrong. I am trying to update code following my last comment.

jaykang10 updated this revision to Diff 371528.Sep 9 2021, 3:03 AM
jaykang10 edited the summary of this revision. (Show Details)

The expected outputs of the existing tests were wrong. Updated them correctly and added more tests.

If it's still a work in progess change, you can as well use "Plan changes" to withdraw it from review list.

mkazantsev added inline comments.Sep 9 2021, 9:24 PM
llvm/test/Transforms/LoopBoundSplit/loop-bound-split.ll
6

This test's name is confusing. There is no sgt inside (and same with the next one).

If it's still a work in progess change, you can as well use "Plan changes" to withdraw it from review list.

I am sorry for changing patch frequently. If possible, I would like to get review with current patch.

llvm/test/Transforms/LoopBoundSplit/loop-bound-split.ll
6

Ah, sorry... The sgt is for loop exit condition. Let me update the test names.

jaykang10 updated this revision to Diff 371815.Sep 10 2021, 1:19 AM

Updated test names.

mkazantsev accepted this revision.Sep 12 2021, 8:11 PM

LGTM, thanks!

llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp
272

As a follow-up (no need to do it in this patch): can AddRecSCEV have the type of SCEVAddRecExpr so that this cast is not needed?

273

As a follow-up: consider using isLoopEntryGuardedByCond. Maybe the 1st iter condition can be inferred from a guarding check. Can be a separate patch.

This revision is now accepted and ready to land.Sep 12 2021, 8:11 PM

LGTM, thanks!

Thanks for review @mkazantsev! Let me push this patch and create a new patch following your comments.

llvm/lib/Transforms/Scalar/LoopBoundSplit.cpp
272

Yep, let me update it in new patch.

273

Yep, let me try to use the isLoopEntryGuardedByCond in new patch.