Page MenuHomePhabricator
Differential D30757

[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
rL297993: [LoopUnroll] Don't peel loops where the latch isn't the exiting block
rG2da2bfa088e9: [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
mzolotukhin accepted this revision.Mar 15 2017, 4:38 PM

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
mkuper added a comment.Mar 15 2017, 4:58 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.

mzolotukhin added a comment.Mar 15 2017, 5:14 PM

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?

mkuper added a comment.Mar 15 2017, 5:23 PM
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)
mzolotukhin added a comment.Mar 16 2017, 1:54 PM

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?

mkuper added a comment.Mar 16 2017, 1:58 PM

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.

test/Transforms/LoopUnroll/peel-loop-irreducible.ll
36–37 ↗(On Diff #92045)

Sure, I'll see if it minimizes more.

Closed by commit rL297993: [LoopUnroll] Don't peel loops where the latch isn't the exiting block (authored by mkuper). · Explain WhyMar 16 2017, 2:20 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 92056

View Options

llvm/trunk/lib/Transforms/Utils/LoopUnrollPeel.cpp

Loading...
View Options

llvm/trunk/test/Transforms/LoopUnroll/peel-loop-irreducible.ll

Loading...