[LoopUnroll] Don't peel loops where the latch isn't the exiting block
ClosedPublic
Actions

Authored by mkuper on Mar 8 2017, 2:23 PM.

Details

Reviewers

mzolotukhin
danielcdh
davide

Commits

rG2da2bfa088e9: [LoopUnroll] Don't peel loops where the latch isn't the exiting block
rL297993: [LoopUnroll] Don't peel loops where the latch isn't the exiting block

Summary

Peeling assumed this doesn't happen, but didn't check it.

This fixes PR32178.

Diff Detail

Repository: rL LLVM

Event Timeline

mkuper created this revision.Mar 8 2017, 2:23 PM

ping

Looks good to me, but, as you noticed in the PR - do we want to work with such loops at all? Did you check what happens if we just bail out on such loops?

Michael

This revision is now accepted and ready to land.Mar 15 2017, 4:38 PM

In D30757#702345, @mzolotukhin wrote:

Looks good to me, but, as you noticed in the PR - do we want to work with such loops at all? Did you check what happens if we just bail out on such loops?

Michael

No, but I'm tempted to, because I think we have more bugs related to irreducible control flow that this patch doesn't fix.

Do you know if we already have a utility somewhere that detects this? There's no way to get this information out of LoopInfo that I'm aware of.
LV rolls its own ( hasCyclesInLoopBody() ), which would be fine for my original use case, but r296898 started handling non-innermost loops.

Do you know if we already have a utility somewhere that detects this?

Can we require the loop to be in a simplified form and its latch to be exiting? Will it suffice?

In D30757#702406, @mzolotukhin wrote:

Do you know if we already have a utility somewhere that detects this?

Can we require the loop to be in a simplified form and its latch to be exiting? Will it suffice?

Maybe?

It will certainly squash this bug, not sure about the others.
We already sort of try to do this - UnrollLoop() bails if the latch is not conditional. I assume the expectation is that one of the edges is the backedge, and the other is the exiting edge, but I don't think this is actually enforced, and it looks like unrolling (as opposed to peeling) manages to deal with it.

mkuper updated this revision to Diff 92045.Mar 16 2017, 1:42 PM

mkuper retitled this revision from [LoopUnroll] Handle loops where the exiting block is different from the latch to [LoopUnroll] Don't peel loops where the latch isn't the exiting block.

mkuper edited the summary of this revision. (Show Details)

This version looks good to me too.

MichaelZ

PS: Can we have a similar problem in the regular loop unrolling?

test/Transforms/LoopUnroll/peel-loop-irreducible.ll
36–37 ↗	(On Diff #92045)	I'd prefer to have LCSSA-ed version of the loop in the test. And, by the way, do we need `%sum`, `%plus` and `%incsum` at all? Maybe remove them to make the test even smaller?

Thanks!

Re the PS - I don't know.
As I wrote above - UnrollLoop() verifies the latch is conditional, but doesn't verify the two edges are actually going to the header and the exit block. But we haven't actually *seen* any issues with this, as far as I know.