Avoid creating redundant PHIs in SSAUpdater::GetValueInMiddleOfBlock.

This was already being done in SSAUpdater::GetValueAtEndOfBlock so I've
just changed SSAUpdater to check for existing PHIs in both places.


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

lib/Transforms/Utils/SSAUpdater.cpp

@@ -71,6 +71,50 @@ void SSAUpdater::AddAvailableValue(BasicBlock *BB, Value *V) {
   getAvailableVals(AV)[BB] = V;
 }
 
+/// IsEquivalentPHI - Check if PHI has the same incoming value as specified
+/// in ValueMapping for each predecessor block.
+static bool IsEquivalentPHI(PHINode *PHI, 
+                            DenseMap<BasicBlock*, Value*> &ValueMapping) {
+  unsigned PHINumValues = PHI->getNumIncomingValues();
+  if (PHINumValues != ValueMapping.size())
+    return false;
+
+  // Scan the phi to see if it matches.
+  for (unsigned i = 0, e = PHINumValues; i != e; ++i)
+    if (ValueMapping[PHI->getIncomingBlock(i)] !=
+        PHI->getIncomingValue(i)) {
+      return false;
+    }
+
+  return true;
+}
+
+/// GetExistingPHI - Check if BB already contains a phi node that is equivalent
+/// to the specified mapping from predecessor blocks to incoming values.
+static Value *GetExistingPHI(BasicBlock *BB,
+                             DenseMap<BasicBlock*, Value*> &ValueMapping) {
+  PHINode *SomePHI;
+  for (BasicBlock::iterator It = BB->begin();
+       (SomePHI = dyn_cast<PHINode>(It)); ++It) {
+    if (IsEquivalentPHI(SomePHI, ValueMapping))
+      return SomePHI;
+  }
+  return 0;
+}
+
+/// GetExistingPHI - Check if BB already contains an equivalent phi node.
+/// The InputIt type must be an iterator over std::pair<BasicBlock*, Value*>
+/// objects that specify the mapping from predecessor blocks to incoming values.
+template<typename InputIt>
+static Value *GetExistingPHI(BasicBlock *BB, const InputIt &I,
+                             const InputIt &E) {
+  // Avoid create the mapping if BB has no phi nodes at all.
+  if (!isa<PHINode>(BB->begin()))
+    return 0;
+  DenseMap<BasicBlock*, Value*> ValueMapping(I, E);
+  return GetExistingPHI(BB, ValueMapping);
+}
+
 /// GetValueAtEndOfBlock - Construct SSA form, materializing a value that is
 /// live at the end of the specified block.
 Value *SSAUpdater::GetValueAtEndOfBlock(BasicBlock *BB) {
@@ -149,28 +193,11 @@ Value *SSAUpdater::GetValueInMiddleOfBlock(BasicBlock *BB) {
   if (SingularValue != 0)
     return SingularValue;
 
-  // Otherwise, we do need a PHI: check to see if we already have one available
+  // Otherwise, we do need a PHI.
-  // in this block that produces the right value.
+  if (Value *ExistingPHI = GetExistingPHI(BB, PredValues.begin(),
-  if (isa<PHINode>(BB->begin())) {
+                                          PredValues.end()))
-    DenseMap<BasicBlock*, Value*> ValueMapping(PredValues.begin(),
+    return ExistingPHI;
-                                               PredValues.end());
+
-    PHINode *SomePHI;
-    for (BasicBlock::iterator It = BB->begin();
-         (SomePHI = dyn_cast<PHINode>(It)); ++It) {
-      // Scan this phi to see if it is what we need.
-      bool Equal = true;
-      for (unsigned i = 0, e = SomePHI->getNumIncomingValues(); i != e; ++i)
-        if (ValueMapping[SomePHI->getIncomingBlock(i)] !=
-            SomePHI->getIncomingValue(i)) {
-          Equal = false;
-          break;
-        }
-         
-      if (Equal)
-        return SomePHI;
-    }
-  }
-  
   // Ok, we have no way out, insert a new one now.
   PHINode *InsertedPHI = PHINode::Create(PrototypeValue->getType(),
                                          PrototypeValue->getName(),
@@ -255,7 +282,7 @@ Value *SSAUpdater::GetValueAtEndOfBlockInternal(BasicBlock *BB) {
   // producing the same value.  If so, this value will capture it, if not, it
   // will get reset to null.  We distinguish the no-predecessor case explicitly
   // below.
-  TrackingVH<Value> SingularValue;
+  TrackingVH<Value> ExistingValue;
 
   // We can get our predecessor info by walking the pred_iterator list, but it
   // is relatively slow.  If we already have PHI nodes in this block, walk one
@@ -266,11 +293,11 @@ Value *SSAUpdater::GetValueAtEndOfBlockInternal(BasicBlock *BB) {
       Value *PredVal = GetValueAtEndOfBlockInternal(PredBB);
       IncomingPredInfo.push_back(std::make_pair(PredBB, PredVal));
 
-      // Compute SingularValue.
+      // Set ExistingValue to singular value from all predecessors so far.
       if (i == 0)
-        SingularValue = PredVal;
+        ExistingValue = PredVal;
-      else if (PredVal != SingularValue)
+      else if (PredVal != ExistingValue)
-        SingularValue = 0;
+        ExistingValue = 0;
     }
   } else {
     bool isFirstPred = true;
@@ -279,12 +306,12 @@ Value *SSAUpdater::GetValueAtEndOfBlockInternal(BasicBlock *BB) {
       Value *PredVal = GetValueAtEndOfBlockInternal(PredBB);
       IncomingPredInfo.push_back(std::make_pair(PredBB, PredVal));
 
-      // Compute SingularValue.
+      // Set ExistingValue to singular value from all predecessors so far.
       if (isFirstPred) {
-        SingularValue = PredVal;
+        ExistingValue = PredVal;
         isFirstPred = false;
-      } else if (PredVal != SingularValue)
+      } else if (PredVal != ExistingValue)
-        SingularValue = 0;
+        ExistingValue = 0;
     }
   }
 
@@ -300,31 +327,38 @@ Value *SSAUpdater::GetValueAtEndOfBlockInternal(BasicBlock *BB) {
   /// above.
   TrackingVH<Value> &InsertedVal = AvailableVals[BB];
 
-  // If all the predecessor values are the same then we don't need to insert a
+  // If the predecessor values are not all the same, then check to see if there
+  // is an existing PHI that can be used.
+  if (!ExistingValue)
+    ExistingValue = GetExistingPHI(BB,
+                                   IncomingPredInfo.begin()+FirstPredInfoEntry,
+                                   IncomingPredInfo.end());
+
+  // If there is an existing value we can use, then we don't need to insert a
   // PHI.  This is the simple and common case.
-  if (SingularValue) {
+  if (ExistingValue) {
-    // If a PHI node got inserted, replace it with the singlar value and delete
+    // If a PHI node got inserted, replace it with the existing value and delete
     // it.
     if (InsertedVal) {
       PHINode *OldVal = cast<PHINode>(InsertedVal);
       // Be careful about dead loops.  These RAUW's also update InsertedVal.
-      if (InsertedVal != SingularValue)
+      if (InsertedVal != ExistingValue)
-        OldVal->replaceAllUsesWith(SingularValue);
+        OldVal->replaceAllUsesWith(ExistingValue);
       else
         OldVal->replaceAllUsesWith(UndefValue::get(InsertedVal->getType()));
       OldVal->eraseFromParent();
     } else {
-      InsertedVal = SingularValue;
+      InsertedVal = ExistingValue;
     }
 
-    // Either path through the 'if' should have set insertedVal -> SingularVal.
+    // Either path through the 'if' should have set InsertedVal -> ExistingVal.
-    assert((InsertedVal == SingularValue || isa<UndefValue>(InsertedVal)) &&
+    assert((InsertedVal == ExistingValue || isa<UndefValue>(InsertedVal)) &&
-           "RAUW didn't change InsertedVal to be SingularVal");
+           "RAUW didn't change InsertedVal to be ExistingValue");
 
     // Drop the entries we added in IncomingPredInfo to restore the stack.
     IncomingPredInfo.erase(IncomingPredInfo.begin()+FirstPredInfoEntry,
                            IncomingPredInfo.end());
-    return SingularValue;
+    return ExistingValue;
   }
 
   // Otherwise, we do need a PHI: insert one now if we don't already have one.

test/Transforms/GVN/rle-nonlocal.ll

@@ -1,4 +1,4 @@
-; RUN: opt < %s -gvn -S | grep {%DEAD = phi i32. }
+; RUN: opt < %s -gvn -S | FileCheck %s
 
 define i32 @main(i32** %p) {
 block1:
@@ -13,7 +13,12 @@ block3:
   br label %block4
 
 block4:
+; CHECK-NOT: %existingPHI = phi
+; CHECK: %DEAD = phi
+  %existingPHI = phi i32* [ %a, %block2 ], [ %b, %block3 ] 
   %DEAD = load i32** %p
   %c = load i32* %DEAD
-  ret i32 %c
+  %d = load i32* %existingPHI
+  %e = add i32 %c, %d
+  ret i32 %e
 }