Page MenuHomePhabricator

[llvm] Loop Vectorization fault in collectLoopUni
ClosedPublic

Authored by david2050 on Jul 27 2016, 2:50 PM.

Details

Summary

In a large LTO build, the assert inside this loop body is triggered because Worklist is empty. This change avoids the problem and appears to successfully compile. The code appears to strongly expect the Worklist to be non-empty.

I will try to find a test case.... but any suggestions along the way would be appreciated.

Diff Detail

Repository
rL LLVM

Event Timeline

david2050 updated this revision to Diff 65808.Jul 27 2016, 2:50 PM
david2050 retitled this revision from to [llvm] Loop Vectorization fault in collectLoopUni.
david2050 updated this object.
david2050 added a reviewer: wmi.
david2050 added subscribers: freik, nadav.
anemet added a subscriber: anemet.Jul 27 2016, 3:36 PM

Hi David,

You forgot to CC llvm-commits.

Adam

Gerolf added a subscriber: Gerolf.Jul 27 2016, 3:58 PM
Gerolf added inline comments.
lib/Transforms/Vectorize/LoopVectorize.cpp
4862 ↗(On Diff #65808)

I think it would be better to return when the worklist is empty. At this point there is nothing left to do and the compiler does not have to execute the case. We ran into the same problem and I isolated function and loop, but don't have a small test case yet.

wmi edited edge metadata.Jul 27 2016, 3:59 PM
wmi added a subscriber: llvm-commits.

Thanks for helping me fixing the bug. I think most of the cases worklist is not empty because loop condition is usually defined inside the loop, but it is possible that the loop condition is defined outside of loop. I think your fix is good.

Here is a testcase reproducing the problem:

~/workarea/llvm-r275818/dbuild/bin/opt -loop-vectorize -S < a.ll

a.ll:

define void @foo() local_unnamed_addr {
entry:
  %exitcond = icmp eq i64 3, 3
  br label %for.body

for.body:                                         ; preds = %entry
  %i.05 = phi i64 [ %inc, %for.body ], [ 0, %entry ]
  %total1 = add nsw i64 %i.05, 3
  %inc = add nuw nsw i64 %i.05, 1
  br i1 %exitcond, label %for.end, label %for.body, !llvm.loop !0

for.end:                                          ; preds = %for.body
  ret void
}

!0 = distinct !{!0, !1}
!1 = !{!"llvm.loop.vectorize.enable", i1 true}
wmi added inline comments.Jul 27 2016, 4:03 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
4862 ↗(On Diff #65808)

Even if the Worklist is empty, it is still possible that induction variable could be added into the uniform set in the following code, so I think the existing fix is better.

wmi accepted this revision.Jul 27 2016, 4:05 PM
wmi edited edge metadata.

LGTM with the test added.

This revision is now accepted and ready to land.Jul 27 2016, 4:05 PM
david2050 updated this revision to Diff 65833.Jul 27 2016, 4:27 PM
david2050 edited edge metadata.

Added test case

Wei would you commit, I don't have permissions

This revision was automatically updated to reflect the committed changes.

Hm, FWIW this test doesn't reproduce with our compiler that exposes the problem at stage 2. And I still think when there are no uniforms within the loop there is no need to traverser the PHI nodes later.
Since the patch clearly fixes an obvious problem I'm OK with a commit even w/o a test case - LGTM.

-Gerolf

anemet added inline comments.Jul 27 2016, 9:37 PM
llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp
4839–4844

Hi Wei,

AFAICT, you introduced this code in D21755. It would be great if you could explain the rationale/add a comment for the isOutOfScope part (especially that it has led to this bug).

Also, was there a test added for the case of an out-of-scope condition in D21755?

Thank you,
Adam

anemet added inline comments.Jul 29 2016, 10:28 AM
llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp
4839–4844

@wmi, did you see this request? It's not urgent; I just wanted to make sure that it does not get lost.