[llvm] r204537 - [Constant Hoisting] Fix multiple entries for the same basic block in PHI nodes.

[Juergen Ributzka]

Author: ributzka
Date: Fri Mar 21 20:49:27 2014
New Revision: 204537

URL: http://llvm.org/viewvc/llvm-project?rev=204537&view=rev
Log:
[Constant Hoisting] Fix multiple entries for the same basic block in PHI nodes.

A PHI node usually has only one value/basic block pair per incoming basic block.
In the case of a switch statement it is possible that a following PHI node may
have more than one such pair per incoming basic block. E.g.:
%0 = phi i64 [ 123456, %case2 ], [ 654321, %Entry ], [ 654321, %Entry ]
This is valid and the verfier doesn't complain, because both values are the
same.

Constant hoisting materializes the constant for each operand separately and the
value is still the same, but the variable names have changed. As a result the
verfier can't recognize anymore that they are the same value and complains.

This fix adds special update code for PHI node in constant hoisting to prevent
this corner case.

This fixes <rdar://problem/16394449>

Modified:
    llvm/trunk/lib/Transforms/Scalar/ConstantHoisting.cpp
    llvm/trunk/test/Transforms/ConstantHoisting/X86/phi.ll

Modified: llvm/trunk/lib/Transforms/Scalar/ConstantHoisting.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/ConstantHoisting.cpp?rev=204537&r1=204536&r2=204537&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/ConstantHoisting.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/ConstantHoisting.cpp Fri Mar 21 20:49:27 2014
@@ -456,6 +456,34 @@ void ConstantHoisting::findBaseConstants
   findAndMakeBaseConstant(MinValItr, ConstCandVec.end());
 }
 
+/// \brief Updates the operand at Idx in instruction Inst with the result of
+///        instruction Mat. If the instruction is a PHI node then special
+///        handling for duplicate values form the same incomming basic block is
+///        required.
+/// \return The update will always succeed, but the return value indicated if
+///         Mat was used for the update or not.
+static bool updateOperand(Instruction *Inst, unsigned Idx, Instruction *Mat) {
+  if (auto PHI = dyn_cast<PHINode>(Inst)) {
+    // Check if any previous operand of the PHI node has the same incoming basic
+    // block. This is a very odd case that happens when the incoming basic block
+    // has a switch statement. In this case use the same value as the previous
+    // operand(s), otherwise we will fail verification due to different values.
+    // The values are actually the same, but the variable names are different
+    // and the verifier doesn't like that.
+    BasicBlock *IncomingBB = PHI->getIncomingBlock(Idx);
+    for (unsigned i = 0; i < Idx; ++i) {
+      if (PHI->getIncomingBlock(i) == IncomingBB) {
+        Value *IncomingVal = PHI->getIncomingValue(i);
+        Inst->setOperand(Idx, IncomingVal);
+        return false;
+      }
+    }
+  }
+
+  Inst->setOperand(Idx, Mat);
+  return true;
+}
+
 /// \brief Emit materialization code for all rebased constants and update their
 /// users.
 void ConstantHoisting::emitBaseConstants(Instruction *Base, Constant *Offset,
@@ -477,7 +505,8 @@ void ConstantHoisting::emitBaseConstants
   // Visit constant integer.
   if (isa<ConstantInt>(Opnd)) {
     DEBUG(dbgs() << "Update: " << *ConstUser.Inst << '\n');
-    ConstUser.Inst->setOperand(ConstUser.OpndIdx, Mat);
+    if (!updateOperand(ConstUser.Inst, ConstUser.OpndIdx, Mat) && Offset)
+      Mat->eraseFromParent();
     DEBUG(dbgs() << "To    : " << *ConstUser.Inst << '\n');
     return;
   }
@@ -499,7 +528,7 @@ void ConstantHoisting::emitBaseConstants
     }
 
     DEBUG(dbgs() << "Update: " << *ConstUser.Inst << '\n');
-    ConstUser.Inst->setOperand(ConstUser.OpndIdx, ClonedCastInst);
+    updateOperand(ConstUser.Inst, ConstUser.OpndIdx, ClonedCastInst);
     DEBUG(dbgs() << "To    : " << *ConstUser.Inst << '\n');
     return;
   }
@@ -517,7 +546,11 @@ void ConstantHoisting::emitBaseConstants
     DEBUG(dbgs() << "Create instruction: " << *ConstExprInst << '\n'
                  << "From              : " << *ConstExpr << '\n');
     DEBUG(dbgs() << "Update: " << *ConstUser.Inst << '\n');
-    ConstUser.Inst->setOperand(ConstUser.OpndIdx, ConstExprInst);
+    if (!updateOperand(ConstUser.Inst, ConstUser.OpndIdx, ConstExprInst)) {
+      ConstExprInst->eraseFromParent();
+      if (Offset)
+        Mat->eraseFromParent();
+    }
     DEBUG(dbgs() << "To    : " << *ConstUser.Inst << '\n');
     return;
   }

Modified: llvm/trunk/test/Transforms/ConstantHoisting/X86/phi.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/ConstantHoisting/X86/phi.ll?rev=204537&r1=204536&r2=204537&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/ConstantHoisting/X86/phi.ll (original)
+++ llvm/trunk/test/Transforms/ConstantHoisting/X86/phi.ll Fri Mar 21 20:49:27 2014
@@ -68,3 +68,49 @@ if.then8:
 if.end9:                                          ; preds = %if.then8, %if.end3
   ret i32 undef
 }
+
+; <rdar://problem/16394449>
+define i64 @switch_test1(i64 %a) {
+; CHECK-LABEL: @switch_test1
+; CHECK: %0 = phi i64 [ %const, %case2 ], [ %const_mat, %Entry ], [ %const_mat, %Entry ]
+Entry:
+  %sel = add i64 %a, 4519019440
+  switch i64 %sel, label %fail [
+    i64 462, label %continuation
+    i64 449, label %case2
+    i64 443, label %continuation
+  ]
+
+case2:
+  br label %continuation
+
+continuation:
+  %0 = phi i64 [ 4519019440, %case2 ], [ 4519019460, %Entry ], [ 4519019460, %Entry ]
+  ret i64 0;
+
+fail:
+  ret i64 -1;
+}
+
+define i64 @switch_test2(i64 %a) {
+; CHECK-LABEL: @switch_test2
+; CHECK: %2 = phi i64* [ %1, %case2 ], [ %0, %Entry ], [ %0, %Entry ]
+Entry:
+  %sel = add i64 %a, 4519019440
+  switch i64 %sel, label %fail [
+    i64 462, label %continuation
+    i64 449, label %case2
+    i64 443, label %continuation
+  ]
+
+case2:
+  br label %continuation
+
+continuation:
+  %0 = phi i64* [ inttoptr(i64 4519019440 to i64*), %case2 ], [ inttoptr(i64 4519019460 to i64*), %Entry ], [ inttoptr(i64 4519019460 to i64*), %Entry ]
+  ret i64 0;
+
+fail:
+  ret i64 -1;
+}
+