[llvm] bd197ed - [SimplifyCFG] avoid sinking insts within an infinite-loop

[Sanjay Patel via llvm-commits]

Author: Sanjay Patel
Date: 2021-03-12T08:04:57-05:00
New Revision: bd197ed0a57a82187ed3c6265ca811d412acfaef

URL: https://github.com/llvm/llvm-project/commit/bd197ed0a57a82187ed3c6265ca811d412acfaef
DIFF: https://github.com/llvm/llvm-project/commit/bd197ed0a57a82187ed3c6265ca811d412acfaef.diff

LOG: [SimplifyCFG] avoid sinking insts within an infinite-loop

The test is reduced from a C source example in:
https://llvm.org/PR49541

It's possible that the test could be reduced further or
the predicate generalized further, but it seems to require
a few ingredients (including the "late" SimplifyCFG options
on the RUN line) to fall into the infinite-loop trap.

Added: 
    llvm/test/Transforms/SimplifyCFG/sink-inf-loop.ll

Modified: 
    llvm/lib/Transforms/Utils/SimplifyCFG.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 12fd519ce113..1f9fa611a9b2 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -1631,6 +1631,11 @@ static bool canSinkInstructions(
         I->getType()->isTokenTy())
       return false;
 
+    // Do not try to sink an instruction in an infinite loop - it can cause
+    // this algorithm to infinite loop.
+    if (I->getParent()->getSingleSuccessor() == I->getParent())
+      return false;
+
     // Conservatively return false if I is an inline-asm instruction. Sinking
     // and merging inline-asm instructions can potentially create arguments
     // that cannot satisfy the inline-asm constraints.
@@ -1717,13 +1722,13 @@ static bool canSinkInstructions(
   return true;
 }
 
-// Assuming canSinkLastInstruction(Blocks) has returned true, sink the last
+// Assuming canSinkInstructions(Blocks) has returned true, sink the last
 // instruction of every block in Blocks to their common successor, commoning
 // into one instruction.
 static bool sinkLastInstruction(ArrayRef<BasicBlock*> Blocks) {
   auto *BBEnd = Blocks[0]->getTerminator()->getSuccessor(0);
 
-  // canSinkLastInstruction returning true guarantees that every block has at
+  // canSinkInstructions returning true guarantees that every block has at
   // least one non-terminator instruction.
   SmallVector<Instruction*,4> Insts;
   for (auto *BB : Blocks) {
@@ -1736,9 +1741,9 @@ static bool sinkLastInstruction(ArrayRef<BasicBlock*> Blocks) {
   }
 
   // The only checking we need to do now is that all users of all instructions
-  // are the same PHI node. canSinkLastInstruction should have checked this but
-  // it is slightly over-aggressive - it gets confused by commutative instructions
-  // so double-check it here.
+  // are the same PHI node. canSinkInstructions should have checked this but
+  // it is slightly over-aggressive - it gets confused by commutative
+  // instructions so double-check it here.
   Instruction *I0 = Insts.front();
   if (!I0->user_empty()) {
     auto *PNUse = dyn_cast<PHINode>(*I0->user_begin());
@@ -1749,11 +1754,11 @@ static bool sinkLastInstruction(ArrayRef<BasicBlock*> Blocks) {
       return false;
   }
 
-  // We don't need to do any more checking here; canSinkLastInstruction should
+  // We don't need to do any more checking here; canSinkInstructions should
   // have done it all for us.
   SmallVector<Value*, 4> NewOperands;
   for (unsigned O = 0, E = I0->getNumOperands(); O != E; ++O) {
-    // This check is 
diff erent to that in canSinkLastInstruction. There, we
+    // This check is 
diff erent to that in canSinkInstructions. There, we
     // cared about the global view once simplifycfg (and instcombine) have
     // completed - it takes into account PHIs that become trivially
     // simplifiable.  However here we need a more local view; if an operand

diff  --git a/llvm/test/Transforms/SimplifyCFG/sink-inf-loop.ll b/llvm/test/Transforms/SimplifyCFG/sink-inf-loop.ll
new file mode 100644
index 000000000000..37399367efce
--- /dev/null
+++ b/llvm/test/Transforms/SimplifyCFG/sink-inf-loop.ll
@@ -0,0 +1,49 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt %s -simplifycfg -simplifycfg-require-and-preserve-domtree=1 -keep-loops=false -sink-common-insts=true -S | FileCheck %s
+
+; This would infinite-loop because we allowed code sinking to examine an infinite-loop block (%j).
+
+define void @PR49541(i32* %t1, i32 %a, i1 %bool) {
+; CHECK-LABEL: @PR49541(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[I:%.*]]
+; CHECK:       j:
+; CHECK-NEXT:    [[T3:%.*]] = phi i32 [ [[B:%.*]], [[J:%.*]] ], [ [[A:%.*]], [[COND_TRUE:%.*]] ], [ [[A]], [[COND_FALSE:%.*]] ]
+; CHECK-NEXT:    [[T2:%.*]] = phi i32 [ [[T2]], [[J]] ], [ [[PRE2:%.*]], [[COND_TRUE]] ], [ 0, [[COND_FALSE]] ]
+; CHECK-NEXT:    [[B]] = load i32, i32* [[T1:%.*]], align 4
+; CHECK-NEXT:    br label [[J]]
+; CHECK:       i:
+; CHECK-NEXT:    [[G_1:%.*]] = phi i16 [ undef, [[ENTRY:%.*]] ], [ [[G_1]], [[COND_FALSE]] ]
+; CHECK-NEXT:    br i1 [[BOOL:%.*]], label [[COND_FALSE]], label [[COND_TRUE]]
+; CHECK:       cond.true:
+; CHECK-NEXT:    [[TOBOOL9_NOT:%.*]] = icmp eq i16 [[G_1]], 0
+; CHECK-NEXT:    [[PRE2]] = load i32, i32* [[T1]], align 4
+; CHECK-NEXT:    br label [[J]]
+; CHECK:       cond.false:
+; CHECK-NEXT:    [[T5:%.*]] = load i32, i32* [[T1]], align 4
+; CHECK-NEXT:    [[B2:%.*]] = icmp eq i32 [[T5]], 0
+; CHECK-NEXT:    br i1 [[B2]], label [[J]], label [[I]]
+;
+entry:
+  br label %i
+
+j:
+  %t3 = phi i32 [ %b, %j ], [ %a, %cond.true ], [ %a, %cond.false ]
+  %t2 = phi i32 [ %t2, %j ], [ %pre2, %cond.true ], [ 0, %cond.false ]
+  %b = load i32, i32* %t1, align 4
+  br label %j
+
+i:
+  %g.1 = phi i16 [ undef, %entry ], [ %g.1, %cond.false ]
+  br i1 %bool, label %cond.false, label %cond.true
+
+cond.true:
+  %tobool9.not = icmp eq i16 %g.1, 0
+  %pre2 = load i32, i32* %t1, align 4
+  br label %j
+
+cond.false:
+  %t5 = load i32, i32* %t1, align 4
+  %b2 = icmp eq i32 %t5, 0
+  br i1 %b2, label %j, label %i
+}