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

[mkuper]

Summary

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

This fixes PR32178.

[mkuper]

ping

[mzolotukhin]

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

[mkuper]

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]

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]

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.

[mzolotukhin]

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]

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.

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

static bool canPeel(Loop *L) {
   // Make sure the loop is in simplified form
   if (!L->isLoopSimplifyForm())
     return false;
 
   // Only peel loops that contain a single exit
   if (!L->getExitingBlock() || !L->getUniqueExitBlock())
     return false;
 
+  // Don't try to peel loops where the latch is not the exiting block.
+  // This can be an indication of two different things:
+  // 1) The loop is not rotated.
+  // 2) The loop contains irreducible control flow that involves the latch.
+  if (L->getLoopLatch() != L->getExitingBlock())
+    return false;
+
   return true;
 }
 
 // Return the number of iterations we want to peel off.
 void llvm::computePeelCount(Loop *L, unsigned LoopSize,
                             TargetTransformInfo::UnrollingPreferences &UP,
                             unsigned &TripCount) {
   UP.PeelCount = 0;

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

+; RUN: opt < %s -S -loop-unroll -unroll-force-peel-count=1 | FileCheck %s
+
+; Check we don't peel loops where the latch is not the exiting block.
+; CHECK-LABEL: @invariant_backedge_irreducible
+; CHECK: entry:
+; CHECK: br label %header
+; CHECK-NOT: peel
+; CHECK: header:
+; CHECK: br i1 {{.*}} label %latch, label %exiting
+; CHECK: latch:
+; CHECK: br i1 {{.*}} label %header, label %exiting
+; CHECK: exiting:
+; CHECK: br i1 {{.*}} label %latch, label %exit
+
+define i32 @invariant_backedge_irreducible(i32 %a, i32 %b) {
+entry:
+  br label %header
+
+header:
+  %i = phi i32 [ 0, %entry ], [ %inc, %latch ]
+  %cmp.phi = phi i1 [ false, %entry ], [ %cmp, %latch ]
+  br i1 %cmp.phi, label %latch, label %exiting
+
+latch:
+  %inc = add i32 %i, 1
+  %cmp = icmp slt i32 %i, 1000
+  br i1 %cmp, label %header, label %exiting
+
+exiting:
+  %cmp.exiting = phi i1 [ %cmp.phi, %header ], [ %cmp, %latch ]
+  br i1 %cmp.exiting, label %latch, label %exit
+
+exit:
+  ret i32 0
+}
+