[MemorySSA] Make Phi cleanups consistent.

[asbirlea]

Summary

Make Phi cleanups consistent: remove self as a trivial Phi and
recurse to potentially remove other trivial phis.

[asbirlea]

Make this purely a cleanup patch.

[george.burgess.iv]

Assuming the test in D66495 (in part) covers this, LGTM with an optional nit.

Thanks!

lib/Analysis/MemorySSAUpdater.cpp

494 ↗

(On Diff #216234)

do we have many more calls like this laying around?

if so, would it be good to add a super simple overload that just takes a MPhi, asserts it's non-null, and expands to tryRemoveTrivialPhi(MPhi, MPhi->operands());?

[asbirlea]

Address comment.

llvm/trunk/include/llvm/Analysis/MemorySSAUpdater.h

private:
   MemoryAccess *getPreviousDefInBlock(MemoryAccess *);
   MemoryAccess *
   getPreviousDefFromEnd(BasicBlock *,
                         DenseMap<BasicBlock *, TrackingVH<MemoryAccess>> &);
   MemoryAccess *
   getPreviousDefRecursive(BasicBlock *,
                           DenseMap<BasicBlock *, TrackingVH<MemoryAccess>> &);
   MemoryAccess *recursePhi(MemoryAccess *Phi);
+  MemoryAccess *tryRemoveTrivialPhi(MemoryPhi *Phi);
   template <class RangeType>
   MemoryAccess *tryRemoveTrivialPhi(MemoryPhi *Phi, RangeType &Operands);
   void tryRemoveTrivialPhis(ArrayRef<WeakVH> UpdatedPHIs);
   void fixupDefs(const SmallVectorImpl<WeakVH> &);
   // Clone all uses and defs from BB to NewBB given a 1:1 map of all
   // instructions and blocks cloned, and a map of MemoryPhi : Definition
   // (MemoryAccess Phi or Def). VMap maps old instructions to cloned
   // instructions and old blocks to cloned blocks. MPhiMap, is created in the

llvm/trunk/lib/Analysis/MemorySSAUpdater.cpp

 }
 // Recurse over a set of phi uses to eliminate the trivial ones
 MemoryAccess *MemorySSAUpdater::recursePhi(MemoryAccess *Phi) {
   if (!Phi)
     return nullptr;
   TrackingVH<MemoryAccess> Res(Phi);
   SmallVector<TrackingVH<Value>, 8> Uses;
   std::copy(Phi->user_begin(), Phi->user_end(), std::back_inserter(Uses));
-  for (auto &U : Uses) {
-    if (MemoryPhi *UsePhi = dyn_cast<MemoryPhi>(&*U)) {
-      auto OperRange = UsePhi->operands();
-      tryRemoveTrivialPhi(UsePhi, OperRange);
-    }
-  }
+  for (auto &U : Uses)
+    if (MemoryPhi *UsePhi = dyn_cast<MemoryPhi>(&*U))
+      tryRemoveTrivialPhi(UsePhi);
   return Res;
 }
 
 // Eliminate trivial phis
 // Phis are trivial if they are defined either by themselves, or all the same
 // argument.
 // IE phi(a, a) or b = phi(a, b) or c = phi(a, a, c)
 // We recursively try to remove them.
+MemoryAccess *MemorySSAUpdater::tryRemoveTrivialPhi(MemoryPhi *Phi) {
+  assert(Phi && "Can only remove concrete Phi.");
+  auto OperRange = Phi->operands();
+  return tryRemoveTrivialPhi(Phi, OperRange);
+}
 template <class RangeType>
 MemoryAccess *MemorySSAUpdater::tryRemoveTrivialPhi(MemoryPhi *Phi,
                                                     RangeType &Operands) {
   // Bail out on non-opt Phis.
   if (NonOptPhis.count(Phi))
     return Phi;
 
   // Detect equal or self arguments
while (!Worklist.empty()) {
       }
     }
   }
 }
 
 void MemorySSAUpdater::removeEdge(BasicBlock *From, BasicBlock *To) {
   if (MemoryPhi *MPhi = MSSA->getMemoryAccess(To)) {
     MPhi->unorderedDeleteIncomingBlock(From);
-    if (MPhi->getNumIncomingValues() == 1)
-      removeMemoryAccess(MPhi);
+    tryRemoveTrivialPhi(MPhi);
   }
 }
 
 void MemorySSAUpdater::removeDuplicatePhiEdgesBetween(const BasicBlock *From,
                                                       const BasicBlock *To) {
   if (MemoryPhi *MPhi = MSSA->getMemoryAccess(To)) {
     bool Found = false;
     MPhi->unorderedDeleteIncomingIf([&](const MemoryAccess *, BasicBlock *B) {
       if (From != B)
         return false;
       if (Found)
         return true;
       Found = true;
       return false;
     });
-    if (MPhi->getNumIncomingValues() == 1)
-      removeMemoryAccess(MPhi);
+    tryRemoveTrivialPhi(MPhi);
   }
 }
 
 static MemoryAccess *getNewDefiningAccessForClone(MemoryAccess *MA,
                                                   const ValueToValueMapTy &VMap,
                                                   PhiToDefMap &MPhiMap,
                                                   bool CloneWasSimplified,
                                                   MemorySSA *MSSA) {
void MemorySSAUpdater::updatePhisWhenInsertingUniqueBackedgeBlock(
   MPhi->setIncomingValue(0, AccFromPreheader);
   MPhi->setIncomingBlock(0, Preheader);
   for (unsigned I = MPhi->getNumIncomingValues() - 1; I >= 1; --I)
     MPhi->unorderedDeleteIncoming(I);
   MPhi->addIncoming(NewMPhi, BEBlock);
 
   // If NewMPhi is a trivial phi, remove it. Its use in the header MPhi will be
   // replaced with the unique value.
-  if (HasUniqueIncomingValue)
-    removeMemoryAccess(NewMPhi);
+  tryRemoveTrivialPhi(MPhi);
 }
 
 void MemorySSAUpdater::updateForClonedLoop(const LoopBlocksRPO &LoopBlocks,
                                            ArrayRef<BasicBlock *> ExitBlocks,
                                            const ValueToValueMapTy &VMap,
                                            bool IgnoreIncomingWithNoClones) {
   PhiToDefMap MPhiMap;
 
Phi->unorderedDeleteIncomingIf([&](MemoryAccess *MA, BasicBlock *B) {
         NewPhi->addIncoming(MA, B);
         if (!IdenticalEdgesWereMerged)
           PredsSet.erase(B);
         return true;
       }
       return false;
     });
     Phi->addIncoming(NewPhi, New);
-    if (onlySingleValue(NewPhi))
-      removeMemoryAccess(NewPhi);
+    tryRemoveTrivialPhi(NewPhi);
   }
 }
 
 void MemorySSAUpdater::removeMemoryAccess(MemoryAccess *MA, bool OptimizePhis) {
   assert(!MSSA->isLiveOnEntryDef(MA) &&
          "Trying to remove the live on entry def");
   // We can only delete phi nodes if they have no uses, or we can replace all
   // uses with a single definition.
void MemorySSAUpdater::removeMemoryAccess(MemoryAccess *MA, bool OptimizePhis) {
   if (!PhisToCheck.empty()) {
     SmallVector<WeakVH, 16> PhisToOptimize{PhisToCheck.begin(),
                                            PhisToCheck.end()};
     PhisToCheck.clear();
 
     unsigned PhisSize = PhisToOptimize.size();
     while (PhisSize-- > 0)
       if (MemoryPhi *MP =
-              cast_or_null<MemoryPhi>(PhisToOptimize.pop_back_val())) {
-        auto OperRange = MP->operands();
-        tryRemoveTrivialPhi(MP, OperRange);
-      }
+              cast_or_null<MemoryPhi>(PhisToOptimize.pop_back_val()))
+        tryRemoveTrivialPhi(MP);
   }
 }
 
 void MemorySSAUpdater::removeBlocks(
     const SmallSetVector<BasicBlock *, 8> &DeadBlocks) {
   // First delete all uses of BB in MemoryPhis.
   for (BasicBlock *BB : DeadBlocks) {
     Instruction *TI = BB->getTerminator();
     assert(TI && "Basic block expected to have a terminator instruction");
     for (BasicBlock *Succ : successors(TI))
       if (!DeadBlocks.count(Succ))
         if (MemoryPhi *MP = MSSA->getMemoryAccess(Succ)) {
           MP->unorderedDeleteIncomingBlock(BB);
-          if (MP->getNumIncomingValues() == 1)
-            removeMemoryAccess(MP);
+          tryRemoveTrivialPhi(MP);
         }
     // Drop all references of all accesses in BB
     if (MemorySSA::AccessList *Acc = MSSA->getWritableBlockAccesses(BB))
       for (MemoryAccess &MA : *Acc)
         MA.dropAllReferences();
   }
 
   // Next, delete all memory accesses in each block
   for (BasicBlock *BB : DeadBlocks) {
     MemorySSA::AccessList *Acc = MSSA->getWritableBlockAccesses(BB);
     if (!Acc)
       continue;
     for (auto AB = Acc->begin(), AE = Acc->end(); AB != AE;) {
       MemoryAccess *MA = &*AB;
       ++AB;
       MSSA->removeFromLookups(MA);
       MSSA->removeFromLists(MA);
     }
   }
 }
 
 void MemorySSAUpdater::tryRemoveTrivialPhis(ArrayRef<WeakVH> UpdatedPHIs) {
   for (auto &VH : UpdatedPHIs)
-    if (auto *MPhi = cast_or_null<MemoryPhi>(VH)) {
-      auto OperRange = MPhi->operands();
-      tryRemoveTrivialPhi(MPhi, OperRange);
-    }
+    if (auto *MPhi = cast_or_null<MemoryPhi>(VH))
+      tryRemoveTrivialPhi(MPhi);
 }
 
 void MemorySSAUpdater::changeToUnreachable(const Instruction *I) {
   const BasicBlock *BB = I->getParent();
   // Remove memory accesses in BB for I and all following instructions.
   auto BBI = I->getIterator(), BBE = BB->end();
   // FIXME: If this becomes too expensive, iterate until the first instruction
   // with a memory access, then iterate over MemoryAccesses.