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

[davide]

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

[efriedma]

lib/Transforms/Utils/LoopUnroll.cpp

407 ↗

(On Diff #112711)

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

[davide]

lib/Transforms/Utils/LoopUnroll.cpp

407 ↗

(On Diff #112711)

Yes, probably. Let me update it.

[davide]

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

[mzolotukhin]

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

[davide]

In D37153#854336, @mzolotukhin wrote:

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

[mzolotukhin]

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

[davide]

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

[davide]

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.

[mzolotukhin]

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

Won't opt %s -S overwrite the input file with the output?

[davide]

In D37153#854871, @mzolotukhin wrote:

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

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

[sanjoy]

llvm/trunk/test/Transforms/LoopUnroll/pr33437.ll

2

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.

[davide]

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

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

bool llvm::UnrollLoop(Loop *L, unsigned Count, unsigned TripCount, bool Force,
   // figure out the loop trip count and the unroll-runtime
   // flag is specified.
   bool RuntimeTripCount = (TripCount == 0 && Count > 0 && AllowRuntime);
 
   assert((!RuntimeTripCount || !PeelCount) &&
          "Did not expect runtime trip-count unrolling "
          "and peeling for the same loop");
 
-  if (PeelCount)
-    peelLoop(L, PeelCount, LI, SE, DT, AC, PreserveLCSSA);
+  if (PeelCount) {
+    bool Peeled = peelLoop(L, PeelCount, LI, SE, DT, AC, PreserveLCSSA);
+
+    // Successful peeling may result in a change in the loop preheader/trip
+    // counts. If we later unroll the loop, we want these to be updated.
+    if (Peeled) {
+      BasicBlock *ExitingBlock = L->getExitingBlock();
+      assert(ExitingBlock && "Loop without exiting block?");
+      Preheader = L->getLoopPreheader();
+      TripCount = SE->getSmallConstantTripCount(L, ExitingBlock);
+      TripMultiple = SE->getSmallConstantTripMultiple(L, ExitingBlock);
+    }
+  }
 
   // Loops containing convergent instructions must have a count that divides
   // their TripMultiple.
   DEBUG(
       {
         bool HasConvergent = false;
         for (auto &BB : L->blocks())
           for (auto &I : *BB)

llvm/trunk/test/Transforms/LoopUnroll/pr33437.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -loop-unroll %s -S | FileCheck %s
+
+declare zeroext i8 @patatino()
+
+define fastcc void @tinky() {
+; CHECK-LABEL: @tinky(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[FOR_COND93:%.*]]
+; CHECK:       for.cond93:
+; CHECK-NEXT:    br label [[FOR_BODY198:%.*]]
+; CHECK:       for.body198:
+; CHECK-NEXT:    [[CALL593:%.*]] = tail call zeroext i8 @patatino()
+; CHECK-NEXT:    ret void
+;
+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
+}