Merge pull request #29410 from atrick/escape-verify-unmapped

Add EscapeAnalysis verification to catch unmapped SILValues.

Author: atrick

include/swift/SILOptimizer/Analysis/EscapeAnalysis.h

@@ -660,7 +660,7 @@ class EscapeAnalysis : public BottomUpIPAnalysis {
         : F(F), EA(EA), isSummaryGraph(isSummaryGraph) {}
 
     /// Returns true if the connection graph is empty.
-    bool isEmpty() {
+    bool isEmpty() const {
       return Values2Nodes.empty() && Nodes.empty() && UsePoints.empty();
     }
 
@@ -906,11 +906,10 @@ class EscapeAnalysis : public BottomUpIPAnalysis {
     /// Dump the connection graph to a DOT file for remote debugging.
     void dumpCG() const;
 
-    /// Checks if the graph is OK.
-    void verify(bool allowMerge = false) const;
+    /// Checks if the graph is valid and complete.
+    void verify() const;
 
-    /// Just verifies the graph structure. This function can also be called
-    /// during the graph is modified, e.g. in mergeAllScheduledNodes().
+    /// Just verifies the graph nodes.
     void verifyStructure(bool allowMerge = false) const;
 
     friend struct ::CGForDotView;
@@ -1045,8 +1044,9 @@ class EscapeAnalysis : public BottomUpIPAnalysis {
 
   /// If \p ai is an optimizable @_semantics("array.uninitialized") call, return
   /// valid call information.
-  ArrayUninitCall canOptimizeArrayUninitializedCall(ApplyInst *ai,
-                                                    ConnectionGraph *conGraph);
+  ArrayUninitCall
+  canOptimizeArrayUninitializedCall(ApplyInst *ai,
+                                    const ConnectionGraph *conGraph);
 
   /// Return true of this tuple_extract is the result of an optimizable
   /// @_semantics("array.uninitialized") call.
@@ -1174,25 +1174,9 @@ class EscapeAnalysis : public BottomUpIPAnalysis {
 
   virtual bool needsNotifications() override { return true; }
 
-  virtual void verify() const override {
-#ifndef NDEBUG
-    for (auto Iter : Function2Info) {
-      FunctionInfo *FInfo = Iter.second;
-      FInfo->Graph.verify();
-      FInfo->SummaryGraph.verify();
-    }
-#endif
-  }
-
-  virtual void verify(SILFunction *F) const override {
-#ifndef NDEBUG
-    if (FunctionInfo *FInfo = Function2Info.lookup(F)) {
-      FInfo->Graph.verify();
-      FInfo->SummaryGraph.verify();
-    }
-#endif
-  }
+  virtual void verify() const override;
 
+  virtual void verify(SILFunction *F) const override;
 };
 
 } // end namespace swift

lib/SILOptimizer/Analysis/EscapeAnalysis.cpp

@@ -609,7 +609,7 @@ void EscapeAnalysis::ConnectionGraph::mergeAllScheduledNodes() {
   //   To    pointsTo-> NodeB (but still has a pointsTo edge to NodeB)
   while (!ToMerge.empty()) {
     if (EnableInternalVerify)
-      verify(/*allowMerge=*/true);
+      verifyStructure(true /*allowMerge*/);
 
     CGNode *From = ToMerge.pop_back_val();
     CGNode *To = From->getMergeTarget();
@@ -745,7 +745,6 @@ void EscapeAnalysis::ConnectionGraph::mergeAllScheduledNodes() {
     From->isMerged = true;
 
     if (From->mappedValue) {
-      // If possible, transfer 'From's mappedValue to 'To' for clarity. Any
       // values previously mapped to 'From' but not transferred to 'To's
       // mappedValue must remain mapped to 'From'. Lookups on those values will
       // find 'To' via the mergeTarget. Dropping a value's mapping is illegal
@@ -762,7 +761,7 @@ void EscapeAnalysis::ConnectionGraph::mergeAllScheduledNodes() {
     From->pointsTo = nullptr;
   }
   if (EnableInternalVerify)
-    verify(/*allowMerge=*/true);
+    verifyStructure(true /*allowMerge*/);
 }
 
 // As a result of a merge, update the pointsTo field of initialNode and
@@ -1574,21 +1573,40 @@ bool CGNode::matchPointToOfDefers(bool allowMerge) const {
   return true;
 }
 
-void EscapeAnalysis::ConnectionGraph::verify(bool allowMerge) const {
+void EscapeAnalysis::ConnectionGraph::verify() const {
 #ifndef NDEBUG
-  verifyStructure(allowMerge);
+  // Invalidating EscapeAnalysis clears the connection graph.
+  if (isEmpty())
+    return;
 
-  // Check graph invariants
-  for (CGNode *Nd : Nodes) {
-    // ConnectionGraph invariant #4: For any node N, all paths starting at N
-    // which consist of only defer-edges and a single trailing points-to edge
-    // must lead to the same
-    assert(Nd->matchPointToOfDefers(allowMerge));
-    if (Nd->mappedValue && !(allowMerge && Nd->isMerged)) {
-      assert(Nd == Values2Nodes.lookup(Nd->mappedValue));
-      assert(EA->isPointer(Nd->mappedValue));
-      // Nodes must always be mapped from the pointer root value.
-      assert(Nd->mappedValue == EA->getPointerRoot(Nd->mappedValue));
+  verifyStructure();
+
+  // Verify that all pointer nodes are still mapped, otherwise the process of
+  // merging nodes may have lost information.
+  for (SILBasicBlock &BB : *F) {
+    for (auto &I : BB) {
+      if (isNonWritableMemoryAddress(&I))
+        continue;
+
+      if (auto ai = dyn_cast<ApplyInst>(&I)) {
+        if (EA->canOptimizeArrayUninitializedCall(ai, this).isValid())
+          continue;
+      }
+      for (auto result : I.getResults()) {
+        if (EA->getPointerBase(result))
+          continue;
+
+        if (!EA->isPointer(result))
+          continue;
+
+        if (!Values2Nodes.lookup(result)) {
+          llvm::dbgs() << "No CG mapping for ";
+          result->dumpInContext();
+          llvm::dbgs() << " in:\n";
+          F->dump();
+          llvm_unreachable("Missing escape connection graph mapping");
+        }
+      }
     }
   }
 #endif
@@ -1597,6 +1615,7 @@ void EscapeAnalysis::ConnectionGraph::verify(bool allowMerge) const {
 void EscapeAnalysis::ConnectionGraph::verifyStructure(bool allowMerge) const {
 #ifndef NDEBUG
   for (CGNode *Nd : Nodes) {
+    // Verify the graph structure...
     if (Nd->isMerged) {
       assert(Nd->mergeTo);
       assert(!Nd->pointsTo);
@@ -1625,6 +1644,19 @@ void EscapeAnalysis::ConnectionGraph::verifyStructure(bool allowMerge) const {
     }
     if (Nd->isInterior())
       assert(Nd->pointsTo && "Interior content node requires a pointsTo node");
+
+    // ConnectionGraph invariant #4: For any node N, all paths starting at N
+    // which consist of only defer-edges and a single trailing points-to edge
+    // must lead to the same
+    assert(Nd->matchPointToOfDefers(allowMerge));
+
+    // Verify the node to value mapping...
+    if (Nd->mappedValue && !(allowMerge && Nd->isMerged)) {
+      assert(Nd == Values2Nodes.lookup(Nd->mappedValue));
+      assert(EA->isPointer(Nd->mappedValue));
+      // Nodes must always be mapped from the pointer root value.
+      assert(Nd->mappedValue == EA->getPointerRoot(Nd->mappedValue));
+    }
   }
 #endif
 }
@@ -1780,8 +1812,8 @@ bool EscapeAnalysis::buildConnectionGraphForDestructor(
 }
 
 EscapeAnalysis::ArrayUninitCall
-EscapeAnalysis::canOptimizeArrayUninitializedCall(ApplyInst *ai,
-                                                  ConnectionGraph *conGraph) {
+EscapeAnalysis::canOptimizeArrayUninitializedCall(
+    ApplyInst *ai, const ConnectionGraph *conGraph) {
   ArrayUninitCall call;
   // This must be an exact match so we don't accidentally optimize
   // "array.uninitialized_intrinsic".
@@ -2425,7 +2457,7 @@ void EscapeAnalysis::recompute(FunctionInfo *Initial) {
     if (BottomUpOrder.wasRecomputedWithCurrentUpdateID(FInfo)) {
       FInfo->Graph.computeUsePoints();
       FInfo->Graph.verify();
-      FInfo->SummaryGraph.verify();
+      FInfo->SummaryGraph.verifyStructure();
     }
   }
 }
@@ -2766,6 +2798,25 @@ void EscapeAnalysis::handleDeleteNotification(SILNode *node) {
   }
 }
 
+void EscapeAnalysis::verify() const {
+#ifndef NDEBUG
+  for (auto Iter : Function2Info) {
+    FunctionInfo *FInfo = Iter.second;
+    FInfo->Graph.verify();
+    FInfo->SummaryGraph.verifyStructure();
+  }
+#endif
+}
+
+void EscapeAnalysis::verify(SILFunction *F) const {
+#ifndef NDEBUG
+  if (FunctionInfo *FInfo = Function2Info.lookup(F)) {
+    FInfo->Graph.verify();
+    FInfo->SummaryGraph.verifyStructure();
+  }
+#endif
+}
+
 SILAnalysis *swift::createEscapeAnalysis(SILModule *M) {
   return new EscapeAnalysis(M);
 }