When peeling kicks in, it updates the loop preheader.
Later, a successful full unroll of the loop needs to update a PHI which i-th argument comes from the loop preheader, so it'd better look at the correct block.
Fixes https://bugs.llvm.org/show_bug.cgi?id=33437
Details
Details
Diff Detail
Diff Detail
Event Timeline
| lib/Transforms/Utils/LoopUnroll.cpp | ||
|---|---|---|
| 407 | Do we need to recompute TripCount and TripMultiple here as well? | |
| lib/Transforms/Utils/LoopUnroll.cpp | ||
|---|---|---|
| 407 | Yes, probably. Let me update it. | |
Comment Actions
Hi,
The change looks good to me, but I'd like to see a more simplified test. We probably don't need 90% of its instructions, and also it doesn't need to be SystemZ-specific.
Michael
Comment Actions
FWIW, this only triggers with SystemZ as triple (probably because it sets peeling parameters differently?). It might apply to other triples as well, but it doesn't seem to crash on Linux x86-64 (or FreeBSD x86-64), which are the only two other triples I have available.
I concur with you this is a more general problem, although I can't trigger this in a different way :)
On your other point, let me try to reduce this further :)
Comment Actions
FWIW, this only triggers with SystemZ as triple (probably because it sets peeling parameters differently?).
Yeah, that's probably the reason. I suggest to (find and) pass the necessary parameters explicitly so we don't depend on target preferences at all.
Michael
Comment Actions
The issue was indeed that peeling was forced only on systemz.
I added a cl::opt and I was able to reproduce the bug on my machine (x86-64 Linux), and I was able to reduce the testcase to something much smaller:
declare zeroext i8 @patatino()
define fastcc void @tinky() {
entry:
br label %for.cond93
for.cond93.loopexit:
ret void
for.cond93:
br label %for.body198
for.body198:
%l_249.12 = phi i8 [ undef, %for.cond93 ], [ %call593, %for.body198 ]
%l_522.01 = phi i32 [ 0, %for.cond93 ], [ 1, %for.body198 ]
%call593 = tail call zeroext i8 @patatino()
br i1 false, label %for.body198, label %for.cond93.loopexit
}Maybe we can squeeze out another instruction or tow, but that doesn't seem to be worth our time.
Comment Actions
I added a cl::opt...
You seemed to forget to use it in the test though :)
I would also give basic blocks and variables more meaningful names (specifically, I'd rename header and preheader blocks to show clearer, what happens in the test).
Michael
| llvm/trunk/test/Transforms/LoopUnroll/pr33437.ll | ||
|---|---|---|
| 2 ↗ | (On Diff #112953) | Won't opt %s -S overwrite the input file with the output? |
Comment Actions
Sorry, let me fix this right now.
| llvm/trunk/test/Transforms/LoopUnroll/pr33437.ll | ||
|---|---|---|
| 2 ↗ | (On Diff #112953) | No, I don't think so. |
| llvm/trunk/test/Transforms/LoopUnroll/pr33437.ll | ||
|---|---|---|
| 2 ↗ | (On Diff #112953) | The main reason to avoid opt -S %s (and prefer opt -S < %s) is that opt -S %s prints out the file name which can cause mismatches (tests can pass or fail spuriously). I don't know if any OS allows this, but e.g. you could have @tinky( in the file path somewhere and the CHECK-NEXT on line 8 would fail. |
Do we need to recompute TripCount and TripMultiple here as well?