EscapeAnalysis: fix a problem where a inconsistent connection graph can be generated.

rdar://problem/24686791

Author: eeckstein

include/swift/SILOptimizer/Analysis/EscapeAnalysis.h

@@ -501,11 +501,25 @@ class EscapeAnalysis : public BottomUpIPAnalysis {
     }
 
     /// Creates a defer-edge between \p From and \p To.
-    /// This may invalidate the graph invariance 4). See addDeferEdge.
-    bool defer(CGNode *From, CGNode *To) {
-      bool EdgeAdded = addDeferEdge(From, To);
+    /// This may trigger node merges to keep the graph invariance 4).
+    /// Returns the \p From node or its merge-target in case \p From was merged
+    /// during adding the edge.
+    /// The \p EdgeAdded is set to true if there was no defer-edge between
+    /// \p From and \p To, yet.
+    CGNode *defer(CGNode *From, CGNode *To, bool &EdgeAdded) {
+      if (addDeferEdge(From, To))
+        EdgeAdded = true;
       mergeAllScheduledNodes();
-      return EdgeAdded;
+      return From->getMergeTarget();
+    }
+
+    /// Creates a defer-edge between \p From and \p To.
+    /// This may trigger node merges to keep the graph invariance 4).
+    /// Returns the \p From node or its merge-target in case \p From was merged
+    /// during adding the edge.
+    CGNode *defer(CGNode *From, CGNode *To) {
+      bool UnusedEdgeAddedFlag = false;
+      return defer(From, To, UnusedEdgeAddedFlag);
     }
 
     /// Merges the \p SourceGraph into this graph. The \p Mapping contains the

lib/SILOptimizer/Analysis/EscapeAnalysis.cpp

@@ -502,9 +502,7 @@ bool EscapeAnalysis::ConnectionGraph::mergeFrom(ConnectionGraph *SourceGraph,
       // Create the edge in this graph. Note: this may trigger merging of
       // content nodes.
       if (DestReachable) {
-        Changed |= defer(DestFrom, DestReachable);
-        // In case DestFrom is merged during adding the defer-edge.
-        DestFrom = DestFrom->getMergeTarget();
+        DestFrom = defer(DestFrom, DestReachable, Changed);
       }
 
       for (auto *Deferred : SourceReachable->defersTo) {
@@ -1080,7 +1078,7 @@ void EscapeAnalysis::buildConnectionGraph(FunctionInfo *FInfo,
       for (SILValue Src : Incoming) {
         CGNode *SrcArg = ConGraph->getNode(Src, this);
         if (SrcArg) {
-          ConGraph->defer(ArgNode, SrcArg);
+          ArgNode = ConGraph->defer(ArgNode, SrcArg);
         } else {
           ConGraph->setEscapesGlobal(ArgNode);
           break;
@@ -1269,7 +1267,7 @@ void EscapeAnalysis::analyzeInstruction(SILInstruction *I,
       assert(ResultNode && "thick functions must have a CG node");
       for (const Operand &Op : I->getAllOperands()) {
         if (CGNode *ArgNode = ConGraph->getNode(Op.get(), this)) {
-          ConGraph->defer(ResultNode, ArgNode);
+          ResultNode = ConGraph->defer(ResultNode, ArgNode);
         }
       }
       return;
@@ -1296,7 +1294,7 @@ void EscapeAnalysis::analyzeInstruction(SILInstruction *I,
             ResultNode = FieldNode;
             assert(isPointer(I));
           } else {
-            ConGraph->defer(ResultNode, FieldNode);
+            ResultNode = ConGraph->defer(ResultNode, FieldNode);
           }
         }
       }
@@ -1332,8 +1330,7 @@ void EscapeAnalysis::analyzeInstruction(SILInstruction *I,
     case ValueKind::ReturnInst:
       if (CGNode *ValueNd = ConGraph->getNode(cast<ReturnInst>(I)->getOperand(),
                                               this)) {
-        ConGraph->defer(ConGraph->getReturnNode(),
-                                 ValueNd);
+        ConGraph->defer(ConGraph->getReturnNode(), ValueNd);
       }
       return;
     default:
@@ -1353,7 +1350,7 @@ analyzeSelectInst(SelectInst *SI, ConnectionGraph *ConGraph) {
       auto *ArgNode = ConGraph->getNode(CaseVal, this);
       assert(ArgNode &&
              "there should be an argument node if there is a result node");
-      ConGraph->defer(ResultNode, ArgNode);
+      ResultNode = ConGraph->defer(ResultNode, ArgNode);
     }
     // ... also including the default value.
     auto *DefaultNode = ConGraph->getNode(SI->getDefaultResult(), this);

test/SILOptimizer/escape_analysis.sil

@@ -56,6 +56,16 @@ struct MyError : ErrorType {
 final class ErrorClass : ErrorType {
 }
 
+struct SomeData {
+  let x : X
+}
+
+struct FourFields {
+  let a : SomeData
+  let b : Pointer
+  let c : Builtin.RawPointer
+  let d : Y
+}
 
 // Sanity check with a simple function.
 
@@ -966,6 +976,37 @@ bb0(%0 : $Optional<Builtin.NativeObject>):
   return %2 : $()
 }
 
+sil_global @global_y : $SomeData
+
+// CHECK-LABEL: CG of test_node_merge_during_struct_inst
+// CHECK-NEXT:    Arg %0 Esc: G, Succ: (%4.1)
+// CHECK-NEXT:    Val %1 Esc: G, Succ: (%4.1)
+// CHECK-NEXT:    Val %4 Esc: G, Succ: (%4.1)
+// CHECK-NEXT:    Con %4.1 Esc: G, Succ: (%4.1), %0, %1, %4
+// CHECK-NEXT:  End
+sil @test_node_merge_during_struct_inst : $@convention(thin) (Y) -> () {
+bb0(%0 : $Y):
+  %1 = global_addr @global_y : $*SomeData
+  %2 = address_to_pointer %1 : $*SomeData to $Builtin.RawPointer
+  %3 = raw_pointer_to_ref %2 : $Builtin.RawPointer to $Y
+
+  %4 = alloc_stack $Pointer
+  %5 = struct_element_addr %4 : $*Pointer, #Pointer.y
+  store %3 to %5 : $*Y
+
+  %7 = load %1 : $*SomeData
+  %8 = load %4 : $*Pointer
+  %9 = address_to_pointer %4 : $*Pointer to $Builtin.RawPointer
+
+  // Analyzing this instruction causes a node merge.
+  // Check that we don't crash on this.
+  %l = struct $FourFields(%7 : $SomeData, %8 : $Pointer, %9 : $Builtin.RawPointer, %0 : $Y)
+
+  dealloc_stack %4 : $*Pointer
+  %r = tuple ()
+  return %r : $()
+}
+
 // CHECK-LABEL: CG of arraysemantics_is_native_no_typecheck
 // CHECK-NEXT:    Arg %0 Esc: A, Succ: 
 // CHECK-NEXT:  End