This is an archive of the discontinued LLVM Phabricator instance.

[LoopUnroll] Properly update loop structure in case of successful peeling
ClosedPublic

Authored by davide on Aug 25 2017, 10:25 AM.

Details

Summary

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

Diff Detail

Event Timeline

davide created this revision.Aug 25 2017, 10:25 AM
efriedma added inline comments.Aug 25 2017, 12:35 PM
lib/Transforms/Utils/LoopUnroll.cpp
407

Do we need to recompute TripCount and TripMultiple here as well?

davide added inline comments.Aug 25 2017, 12:45 PM
lib/Transforms/Utils/LoopUnroll.cpp
407

Yes, probably. Let me update it.

davide updated this revision to Diff 112735.Aug 25 2017, 1:28 PM

Update Trip count/tripmultiple asking them to ScalarEvolution, as suggested by Eli.

davide retitled this revision from [LoopUnroll] Properly update the loop preheader in case of successful peeling to [LoopUnroll] Properly update loop structure in case of successful peeling.Aug 25 2017, 5:01 PM
mzolotukhin edited edge metadata.Aug 28 2017, 11:32 AM

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

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

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 :)

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

I think I need a new cl::opt to force peeling count from loop unrolling.

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.

This revision was automatically updated to reflect the committed changes.

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?

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

Sorry, let me fix this right now.

llvm/trunk/test/Transforms/LoopUnroll/pr33437.ll
2 ↗(On Diff #112953)

No, I don't think so.
If it does, then I'll need to change the other hundreds of tests I've written :)

sanjoy added inline comments.Aug 28 2017, 8:19 PM
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.

All possibly addressed in r312015. Thanks for the comments, guys.