[PATCH] D51486: Add check to Latch's terminator in UnrollRuntimeLoopRemainder

[Rodrigo Caetano Rocha via Phabricator via llvm-commits]

rcorcs updated this revision to Diff 163966.
rcorcs added a comment.

I think that this patch is mainly important because these functions are externally visible in the library's API.
In fact, I experienced this problem when using the function llvm::UnrollRuntimeLoopRemainder in one of my own passes and then running it on SPEC2006.
This patch avoids unnecessarily making an implicit contract with the API's user.

I've added a test case. Please make sure that it follows the testing standards.
However, as mentioned by Anna, LLVM's loop unroll would not hit this problem as it guarantees externally that the loop latch has an exit branch.


Repository:
  rL LLVM

https://reviews.llvm.org/D51486

Files:
  lib/Transforms/Utils/LoopUnrollRuntime.cpp
  test/Transforms/LoopUnroll/runtime-loop-non-exiting-latch.ll


Index: test/Transforms/LoopUnroll/runtime-loop-non-exiting-latch.ll
===================================================================
--- test/Transforms/LoopUnroll/runtime-loop-non-exiting-latch.ll
+++ test/Transforms/LoopUnroll/runtime-loop-non-exiting-latch.ll
@@ -0,0 +1,27 @@
+; RUN: opt < %s -S -loop-unroll -unroll-runtime=true -unroll-allow-remainder=true -unroll-count=4
+
+; Make sure that the runtime unroll does not break with a non-exiting latch.
+
+define i32 @test(i32* %a, i32* %b, i32* %c, i64 %n) {
+entry:
+  br label %while.cond
+
+while.cond:                                       ; preds = %while.body, %entry
+  %i.0 = phi i64 [ 0, %entry ], [ %inc, %while.body ]
+  %cmp = icmp slt i64 %i.0, %n
+  br i1 %cmp, label %while.body, label %while.end
+
+while.body:                                       ; preds = %while.cond
+  %arrayidx = getelementptr inbounds i32, i32* %b, i64 %i.0
+  %0 = load i32, i32* %arrayidx
+  %arrayidx1 = getelementptr inbounds i32, i32* %c, i64 %i.0
+  %1 = load i32, i32* %arrayidx1
+  %mul = mul nsw i32 %0, %1
+  %arrayidx2 = getelementptr inbounds i32, i32* %a, i64 %i.0
+  store i32 %mul, i32* %arrayidx2
+  %inc = add nsw i64 %i.0, 1
+  br label %while.cond
+
+while.end:                                        ; preds = %while.cond
+  ret i32 0
+}
Index: lib/Transforms/Utils/LoopUnrollRuntime.cpp
===================================================================
--- lib/Transforms/Utils/LoopUnrollRuntime.cpp
+++ lib/Transforms/Utils/LoopUnrollRuntime.cpp
@@ -545,13 +545,27 @@
   BasicBlock *Header = L->getHeader();
 
   BranchInst *LatchBR = cast<BranchInst>(Latch->getTerminator());
+
+  if (!LatchBR || LatchBR->isUnconditional()) {
+    // The loop-rotate pass can be helpful to avoid this in many cases.
+    LLVM_DEBUG(
+        dbgs()
+        << "Loop latch not terminated by a conditional branch.\n");
+    return false;
+  }
+
   unsigned ExitIndex = LatchBR->getSuccessor(0) == Header ? 1 : 0;
   BasicBlock *LatchExit = LatchBR->getSuccessor(ExitIndex);
-  // Cloning the loop basic blocks (`CloneLoopBlocks`) requires that one of the
-  // targets of the Latch be an exit block out of the loop. This needs
-  // to be guaranteed by the callers of UnrollRuntimeLoopRemainder.
-  assert(!L->contains(LatchExit) &&
-         "one of the loop latch successors should be the exit block!");
+
+  if (L->contains(LatchExit)) {
+    // Cloning the loop basic blocks (`CloneLoopBlocks`) requires that one of the
+    // targets of the Latch be an exit block out of the loop.
+    LLVM_DEBUG(
+        dbgs()
+        << "One of the loop latch successors must be the exit block.\n");
+    return false;
+  }
+
   // These are exit blocks other than the target of the latch exiting block.
   SmallVector<BasicBlock *, 4> OtherExits;
   bool isMultiExitUnrollingEnabled =


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D51486.163966.patch
Type: text/x-patch
Size: 2839 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180905/cb7173fd/attachment.bin>