[MemorySSA] Add additional verification for phis.

Summary:
Verify that the incoming defs into phis are the last defs from the
respective incoming blocks.
When moving blocks, insertDef must RenameUses. Adding this verification
makes GVNHoist tests fail that uncovered this issue.

Reviewers: george.burgess.iv

Subscribers: jlebar, Prazek, llvm-commits

Tags: #llvm

Differential Revision: https://reviews.llvm.org/D63147

llvm-svn: 367451

include/llvm/Analysis/MemorySSA.h

@@ -793,6 +793,7 @@ protected:
   friend class MemorySSAPrinterLegacyPass;
   friend class MemorySSAUpdater;
 
+  void verifyPrevDefInPhis(Function &F) const;
   void verifyDefUses(Function &F) const;
   void verifyDomination(Function &F) const;
   void verifyOrdering(Function &F) const;

lib/Analysis/MemorySSA.cpp

@@ -49,6 +49,7 @@
 #include "llvm/Support/raw_ostream.h"
 #include <algorithm>
 #include <cassert>
+#include <cstdlib>
 #include <iterator>
 #include <memory>
 #include <utility>
@@ -1852,6 +1853,7 @@ void MemorySSA::verifyMemorySSA() const {
   verifyDomination(F);
   verifyOrdering(F);
   verifyDominationNumbers(F);
+  verifyPrevDefInPhis(F);
   // Previously, the verification used to also verify that the clobberingAccess
   // cached by MemorySSA is the same as the clobberingAccess found at a later
   // query to AA. This does not hold true in general due to the current fragility
@@ -1864,6 +1866,46 @@ void MemorySSA::verifyMemorySSA() const {
   // example, see test4 added in D51960.
 }
 
+void MemorySSA::verifyPrevDefInPhis(Function &F) const {
+#ifndef NDEBUG
+  for (const BasicBlock &BB : F) {
+    if (MemoryPhi *Phi = getMemoryAccess(&BB)) {
+      for (unsigned I = 0, E = Phi->getNumIncomingValues(); I != E; ++I) {
+        auto *Pred = Phi->getIncomingBlock(I);
+        auto *IncAcc = Phi->getIncomingValue(I);
+        // If Pred has no unreachable predecessors, get last def looking at
+        // IDoms. If, while walkings IDoms, any of these has an unreachable
+        // predecessor, then the expected incoming def is LoE.
+        if (auto *DTNode = DT->getNode(Pred)) {
+          while (DTNode) {
+            if (auto *DefList = getBlockDefs(DTNode->getBlock())) {
+              auto *LastAcc = &*(--DefList->end());
+              assert(LastAcc == IncAcc &&
+                     "Incorrect incoming access into phi.");
+              break;
+            }
+            DTNode = DTNode->getIDom();
+          }
+          assert((DTNode || IncAcc == getLiveOnEntryDef()) &&
+                 "Expected LoE inc");
+        } else if (auto *DefList = getBlockDefs(Pred)) {
+          // If Pred has unreachable predecessors, but has at least a Def, the
+          // incoming access can be the last Def in Pred, or it could have been
+          // optimized to LoE.
+          auto *LastAcc = &*(--DefList->end());
+          assert((LastAcc == IncAcc || IncAcc == getLiveOnEntryDef()) &&
+                 "Incorrect incoming access into phi.");
+        } else {
+          // If Pred has unreachable predecessors and no Defs, incoming access
+          // should be LoE.
+          assert(IncAcc == getLiveOnEntryDef() && "Expected LoE inc");
+        }
+      }
+    }
+  }
+#endif
+}
+
 /// Verify that all of the blocks we believe to have valid domination numbers
 /// actually have valid domination numbers.
 void MemorySSA::verifyDominationNumbers(const Function &F) const {

lib/Analysis/MemorySSAUpdater.cpp

@@ -1074,7 +1074,7 @@ void MemorySSAUpdater::moveTo(MemoryUseOrDef *What, BasicBlock *BB,
 
   // Now reinsert it into the IR and do whatever fixups needed.
   if (auto *MD = dyn_cast<MemoryDef>(What))
-    insertDef(MD);
+    insertDef(MD, true);
   else
     insertUse(cast<MemoryUse>(What));
 

test/Analysis/MemorySSA/unreachable.ll

@@ -0,0 +1,31 @@
+; RUN: opt -licm -enable-mssa-loop-dependency -verify-memoryssa %s -S | FileCheck %s
+; REQUIRES: asserts
+; Ensure verification doesn't fail with unreachable blocks.
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-grtev4-linux-gnu"
+
+declare dso_local void @f()
+
+; CHECK-LABEL: @foo
+define dso_local void @foo() {
+entry:
+  br i1 undef, label %if.then, label %if.end
+
+if.then:                                          ; preds = %entry
+  br label %try.cont
+
+if.end:                                           ; preds = %entry
+; 1 = MemoryDef(liveOnEntry)
+  call void @f()
+  br label %try.cont
+
+catch:                                            ; No predecessors!
+; 2 = MemoryDef(liveOnEntry)
+  call void @f()
+  br label %try.cont
+
+try.cont:                                         ; preds = %if.end, %catch, %if.then
+; 3 = MemoryPhi({if.then,liveOnEntry},{if.end,1},{catch,liveOnEntry})
+  ret void
+}