From: Torok Edwin <edwintorok@gmail.com>
Date: Wed, 21 Oct 2009 10:49:00 +0000 (+0000)
Subject: Fix PR5262: when folding select into PHI, make sure all operands are available
X-Git-Url: http://plrg.eecs.uci.edu/git/?p=oota-llvm.git;a=commitdiff_plain;h=0739bd0ab2babc99f589cc8e88209b540c3732da

Fix PR5262: when folding select into PHI, make sure all operands are available
in the PHI's Basic Block. This uses a conservative approach, because we don't
have dominator info in instcombine.


git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@84754 91177308-0d34-0410-b5e6-96231b3b80d8
---

diff --git a/lib/Transforms/Scalar/InstructionCombining.cpp b/lib/Transforms/Scalar/InstructionCombining.cpp
index ea2164f6587..46fac196dbb 100644
--- a/lib/Transforms/Scalar/InstructionCombining.cpp
+++ b/lib/Transforms/Scalar/InstructionCombining.cpp
@@ -1985,6 +1985,42 @@ static Instruction *FoldOpIntoSelect(Instruction &Op, SelectInst *SI,
   return 0;
 }
 
+// Check whether all the operands of the PHI dominate the PHI node,
+// knowing that the PHI's operands either dominate BB, or are defined in the BB.
+static bool checkPHI(PHINode *PN, BasicBlock *BB)
+{
+  BasicBlock *phiBB = PN->getParent();
+  for (unsigned i=0;i<PN->getNumIncomingValues();i++) {
+    Instruction *I = dyn_cast<Instruction>(PN->getIncomingValue(i));
+    if (!I)
+      continue;
+    BasicBlock *pBB = I->getParent();
+    if (pBB == BB) {
+      // another PHI in same BB is always before PN (PN is last phi).
+      if (isa<PHINode>(I))
+        continue;
+      // An instruction in the same BB, and not a phi, this is after PN.
+      return false;
+    }
+    if (phiBB == BB)
+      continue;
+    // We don't have dominator info, so just check that the instruction
+    // is not defined in on of the BB on the unique path between BB and phiBB.
+    // If there is no such unique path, or pBB equals to one of the BBs on that
+    // path we know that this operand doesn't dominate the PHI node.
+    BasicBlock *B = PN->getIncomingBlock(i);
+    while ((B = B->getUniquePredecessor())) {
+      if (pBB == B)
+        return false;
+      if (B == phiBB)
+        break;
+    }
+    // No unique path -> doesn't dominate
+    if (!B)
+      return false;
+  }
+  return true;
+}
 
 /// FoldOpIntoPhi - Given a binary operator, cast instruction, or select which
 /// has a PHI node as operand #0, see if we can fold the instruction into the
@@ -2036,9 +2072,13 @@ Instruction *InstCombiner::FoldOpIntoPhi(Instruction &I,
   // Okay, we can do the transformation: create the new PHI node.
   PHINode *NewPN = PHINode::Create(I.getType(), "");
   NewPN->reserveOperandSpace(PN->getNumOperands()/2);
-  InsertNewInstBefore(NewPN, *PN);
-  NewPN->takeName(PN);
+  // We must the PHI node as the last PHI, because it may use one of the other
+  // PHIs.
+  BasicBlock::iterator BBIt = PN;
+  while (isa<PHINode>(BBIt)) ++BBIt;
+  InsertNewInstBefore(NewPN, *BBIt);
 
+  SmallVector<Instruction*, 2> tmpWorklist;
   // Next, add all of the operands to the PHI.
   if (SelectInst *SI = dyn_cast<SelectInst>(&I)) {
     // We only currently try to fold the condition of a select when it is a phi,
@@ -2058,7 +2098,7 @@ Instruction *InstCombiner::FoldOpIntoPhi(Instruction &I,
         InV = SelectInst::Create(PN->getIncomingValue(i), TrueVInPred,
                                  FalseVInPred,
                                  "phitmp", NonConstBB->getTerminator());
-        Worklist.Add(cast<Instruction>(InV));
+        tmpWorklist.push_back(cast<Instruction>(InV));
       }
       NewPN->addIncoming(InV, ThisBB);
     }
@@ -2084,8 +2124,7 @@ Instruction *InstCombiner::FoldOpIntoPhi(Instruction &I,
                                 NonConstBB->getTerminator());
         else
           llvm_unreachable("Unknown binop!");
-        
-        Worklist.Add(cast<Instruction>(InV));
+        tmpWorklist.push_back(cast<Instruction>(InV));
       }
       NewPN->addIncoming(InV, PN->getIncomingBlock(i));
     }
@@ -2101,11 +2140,26 @@ Instruction *InstCombiner::FoldOpIntoPhi(Instruction &I,
         InV = CastInst::Create(CI->getOpcode(), PN->getIncomingValue(i), 
                                I.getType(), "phitmp", 
                                NonConstBB->getTerminator());
-        Worklist.Add(cast<Instruction>(InV));
+        tmpWorklist.push_back(cast<Instruction>(InV));
       }
       NewPN->addIncoming(InV, PN->getIncomingBlock(i));
     }
   }
+  // The PHI's operands must dominate the PHI, if we can't prove that
+  // undo this transformation.
+  if (!checkPHI(NewPN, I.getParent())) {
+    Worklist.Remove(NewPN);
+    NewPN->eraseFromParent();
+    while (!tmpWorklist.empty()) {
+      tmpWorklist.pop_back_val()->eraseFromParent();
+    }
+    return 0;
+  }
+  while (!tmpWorklist.empty()) {
+    Worklist.Add(tmpWorklist.pop_back_val());
+  }
+  NewPN->takeName(PN);
+  Worklist.Add(PN);
   return ReplaceInstUsesWith(I, NewPN);
 }
 
diff --git a/test/Transforms/InstCombine/crash.ll b/test/Transforms/InstCombine/crash.ll
index d475ab5bc57..ab6185670a8 100644
--- a/test/Transforms/InstCombine/crash.ll
+++ b/test/Transforms/InstCombine/crash.ll
@@ -44,3 +44,58 @@ entry:
   store i32 %ins, i32* %arrayidx20
   ret void
 }
+
+; PR5262
+@tmp2 = global i64 0                              ; <i64*> [#uses=1]
+
+declare void @use(i64) nounwind
+
+define void @foo(i1) nounwind align 2 {
+; <label>:1
+  br i1 %0, label %2, label %3
+
+; <label>:2                                       ; preds = %1
+  br label %3
+
+; <label>:3                                       ; preds = %2, %1
+  %4 = phi i8 [ 1, %2 ], [ 0, %1 ]                ; <i8> [#uses=1]
+  %5 = icmp eq i8 %4, 0                           ; <i1> [#uses=1]
+  %6 = load i64* @tmp2, align 8                   ; <i64> [#uses=1]
+  %7 = select i1 %5, i64 0, i64 %6                ; <i64> [#uses=1]
+  br label %8
+
+; <label>:8                                       ; preds = %3
+  call void @use(i64 %7)
+  ret void
+}
+
+%t0 = type { i32, i32 }
+%t1 = type { i32, i32, i32, i32, i32* }
+
+declare %t0* @bar2(i64)
+
+define void @bar3(i1, i1) nounwind align 2 {
+; <label>:2
+  br i1 %1, label %10, label %3
+
+; <label>:3                                       ; preds = %2
+  %4 = getelementptr inbounds %t0* null, i64 0, i32 1 ; <i32*> [#uses=0]
+  %5 = getelementptr inbounds %t1* null, i64 0, i32 4 ; <i32**> [#uses=1]
+  %6 = load i32** %5, align 8                     ; <i32*> [#uses=1]
+  %7 = icmp ne i32* %6, null                      ; <i1> [#uses=1]
+  %8 = zext i1 %7 to i32                          ; <i32> [#uses=1]
+  %9 = add i32 %8, 0                              ; <i32> [#uses=1]
+  br label %10
+
+; <label>:10                                      ; preds = %3, %2
+  %11 = phi i32 [ %9, %3 ], [ 0, %2 ]             ; <i32> [#uses=1]
+  br i1 %1, label %12, label %13
+
+; <label>:12                                      ; preds = %10
+  br label %13
+
+; <label>:13                                      ; preds = %12, %10
+  %14 = zext i32 %11 to i64                       ; <i64> [#uses=1]
+  %15 = tail call %t0* @bar2(i64 %14) nounwind      ; <%0*> [#uses=0]
+  ret void
+}
diff --git a/test/Transforms/InstCombine/fold-intophi.ll b/test/Transforms/InstCombine/fold-intophi.ll
new file mode 100644
index 00000000000..23b482db19e
--- /dev/null
+++ b/test/Transforms/InstCombine/fold-intophi.ll
@@ -0,0 +1,54 @@
+; RUN: opt -instcombine -S <%s | FileCheck %s
+; PR5262
+; Make sure the PHI node gets put in a place where all of its operands dominate it
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@tmp2 = global i64 0                              ; <i64*> [#uses=1]
+
+declare void @use(i64) nounwind
+
+define void @foo(i1) nounwind align 2 {
+; <label>:1
+  br i1 %0, label %2, label %3
+
+; <label>:2                                       ; preds = %1
+  br label %3
+
+; <label>:3                                       ; preds = %2, %1
+; CHECK: <label>:3
+; CHECK-NEXT: %v4 = phi i1 [ false, %2 ], [ true, %1 ]
+; CHECK-NEXT: %v4_ =  phi i1 [ true, %2 ], [ false, %1 ]
+; CHECK-NEXT: br label %l8
+  %v4 = phi i8 [ 1, %2 ], [ 0, %1 ]                ; <i8> [#uses=1]
+  %v4_ = phi i8 [ 0, %2 ], [ 1, %1 ]                ; <i8> [#uses=1]
+  %v5 = icmp eq i8 %v4, 0                           ; <i1> [#uses=1]
+  %v5_ = icmp eq i8 %v4_, 0                           ; <i1> [#uses=1]
+  %v6 = load i64* @tmp2, align 8                   ; <i64> [#uses=1]
+  %v7 = select i1 %v5, i64 0, i64 %v6                ; <i64> [#uses=1]
+  br label %l8
+
+l8:
+  %v9 = load i64* @tmp2, align 8
+  call void @use(i64 %v7)
+  br label %l10
+l10:
+  %v11 = select i1 %v5_, i64 0, i64 %v9                ; <i64> [#uses=1]
+  call void @use(i64 %v11)
+  br label %l11
+l11:
+  %v12 = load i64* @tmp2, align 8
+  %v13 = select i1 %v5_, i64 0, i64 %v12                ; <i64> [#uses=1]
+  call void @use(i64 %v13)
+  br i1 %0, label %l12, label %l13
+l12:
+  br label %l13
+l13:
+;CHECK: l13:
+;CHECK-NEXT: %v14 = phi i64 [ %v12, %l12 ], [ 0, %l11 ]
+;CHECK-NEXT: call void @use(i64 %v14)
+  %v14 = phi i1 [0, %l12], [1, %l11]
+  %v16 = select i1 %v14, i64 0, i64 %v12                ; <i64> [#uses=1]
+  call void @use(i64 %v16)
+  ret void
+}