Cleanup EscapeAnalysis getNode and fix undef handling.

Consistently handle base and derived pointers.

Consistently avoid creating nodes for undef, which breaks
verification.

Be more precise about creating the node based on the derived pointer
type.

Remove a few extraneous helpers.

Fixes <rdar://58445744> swiftc assert during EscapeAnalysis
verification: (EA->isPointer(Nd->mappedValue)), function verify

Author: atrick

include/swift/SILOptimizer/Analysis/EscapeAnalysis.h

@@ -721,15 +721,12 @@ class EscapeAnalysis : public BottomUpIPAnalysis {
       }
     }
 
-    // Helper for getNode and getValueContent.
-    CGNode *getOrCreateNode(ValueBase *V, PointerKind pointerKind);
-
     /// Gets or creates a node for a value \p V.
     /// If V is a projection(-path) then the base of the projection(-path) is
     /// taken. This means the node is always created for the "outermost" value
     /// where V is contained.
-    /// Returns null, if V is not a "pointer".
-    CGNode *getNode(ValueBase *V, bool createIfNeeded = true);
+    /// Returns null, if V (or its base value) is not a "pointer".
+    CGNode *getNode(SILValue V);
 
     // Helper for getValueContent to create and return a content node with the
     // given \p isInterior and \p hasReferenceOnly flags. \p addrNode
@@ -780,15 +777,6 @@ class EscapeAnalysis : public BottomUpIPAnalysis {
       return ReturnNode;
     }
 
-    /// Returns the node of the "exact" value \p V (no projections are skipped)
-    /// if one exists.
-    CGNode *lookupNode(ValueBase *V) {
-      CGNode *Node = Values2Nodes.lookup(V);
-      if (Node)
-        return Node->getMergeTarget();
-      return nullptr;
-    }
-    
     /// Re-uses a node for another SIL value.
     void setNode(ValueBase *V, CGNode *Node) {
       assert(Values2Nodes.find(V) == Values2Nodes.end());
@@ -871,13 +859,6 @@ class EscapeAnalysis : public BottomUpIPAnalysis {
     bool forwardTraverseDefer(CGNode *startNode, CGNodeVisitor &&visitor);
 
   public:
-    /// Gets or creates a node for a value \p V.
-    /// If V is a projection(-path) then the base of the projection(-path) is
-    /// taken. This means the node is always created for the "outermost" value
-    /// where V is contained.
-    /// Returns null, if V is not a "pointer".
-    CGNode *getNodeOrNull(ValueBase *V) { return getNode(V, false); }
-
     /// Get the content node pointed to by \p ptrVal.
     ///
     /// If \p ptrVal cannot be mapped to a node, return nullptr.

lib/SILOptimizer/Analysis/EscapeAnalysis.cpp

@@ -389,47 +389,55 @@ void EscapeAnalysis::ConnectionGraph::clear() {
   assert(ToMerge.empty());
 }
 
+// This never returns an interior node. It should never be called directly on an
+// address projection of a reference. To get the interior node for an address
+// projection, always ask for the content of the projection's base instead using
+// getValueContent() or getReferenceContent().
+//
+// Address phis are not allowed, so merging an unknown address with a reference
+// address projection is rare. If that happens, then the projection's node loses
+// it's interior property.
 EscapeAnalysis::CGNode *
-EscapeAnalysis::ConnectionGraph::getOrCreateNode(ValueBase *V,
-                                                 PointerKind pointerKind) {
-  assert(pointerKind != EscapeAnalysis::NoPointer);
-
+EscapeAnalysis::ConnectionGraph::getNode(SILValue V) {
+  // Early filter obvious non-pointer opcodes.
   if (isa<FunctionRefInst>(V) || isa<DynamicFunctionRefInst>(V) ||
       isa<PreviousDynamicFunctionRefInst>(V))
     return nullptr;
 
-  CGNode * &Node = Values2Nodes[V];
-  // Nodes mapped to values must have an indirect pointsTo. Nodes that don't
-  // have an indirect pointsTo are imaginary nodes that don't directly represnt
-  // a SIL value.
-  bool hasReferenceOnly = canOnlyContainReferences(pointerKind);
-  if (!Node) {
-    if (isa<SILFunctionArgument>(V)) {
-      Node = allocNode(V, NodeType::Argument, false, hasReferenceOnly);
-      if (!isSummaryGraph)
-        Node->mergeEscapeState(EscapeState::Arguments);
-    } else {
-      Node = allocNode(V, NodeType::Value, false, hasReferenceOnly);
-    }
-  }
-  return Node->getMergeTarget();
-}
-
-EscapeAnalysis::CGNode *
-EscapeAnalysis::ConnectionGraph::getNode(ValueBase *V, bool createIfNeeded) {
+  // Create the node flags based on the derived value's kind. If the pointer
+  // base type has non-reference pointers but they are never accessed in the
+  // current function, then ignore them.
   PointerKind pointerKind = EA->getPointerKind(V);
   if (pointerKind == EscapeAnalysis::NoPointer)
     return nullptr;
 
   // Look past address projections, pointer casts, and the like within the same
   // object. Does not look past a dereference such as ref_element_addr, or
   // project_box.
-  V = EA->getPointerRoot(V);
+  SILValue ptrBase = EA->getPointerRoot(V);
+  // Do not create a node for undef values so we can verify that node values
+  // have the correct pointer kind.
+  if (!ptrBase->getFunction())
+    return nullptr;
 
-  if (!createIfNeeded)
-    return lookupNode(V);
+  assert(EA->isPointer(ptrBase) &&
+         "The base for derived pointer must also be a pointer type");
 
-  return getOrCreateNode(V, pointerKind);
+  bool hasReferenceOnly = canOnlyContainReferences(pointerKind);
+  // Update the value-to-node map.
+  CGNode *&Node = Values2Nodes[ptrBase];
+  if (Node) {
+    CGNode *targetNode = Node->getMergeTarget();
+    targetNode->mergeFlags(false /*isInterior*/, hasReferenceOnly);
+    return targetNode;
+  }
+  if (isa<SILFunctionArgument>(ptrBase)) {
+    Node = allocNode(ptrBase, NodeType::Argument, false, hasReferenceOnly);
+    if (!isSummaryGraph)
+      Node->mergeEscapeState(EscapeState::Arguments);
+  } else
+    Node = allocNode(ptrBase, NodeType::Value, false, hasReferenceOnly);
+  return Node;
 }
 
 /// Adds an argument/instruction in which the node's memory is released.
@@ -916,8 +924,8 @@ EscapeAnalysis::ConnectionGraph::getOrCreateAddressContent(SILValue addrVal,
 
   bool contentHasReferenceOnly =
       EA->hasReferenceOnly(addrVal->getType().getObjectType(), *F);
-  // Address content always has an indirect pointsTo (only reference content can
-  // have a non-indirect pointsTo).
+  // Address content is never an interior node (only reference content can
+  // be an interior node).
   return getOrCreateContentNode(addrNode, false, contentHasReferenceOnly);
 }
 
@@ -926,12 +934,14 @@ EscapeAnalysis::ConnectionGraph::getOrCreateAddressContent(SILValue addrVal,
 CGNode *
 EscapeAnalysis::ConnectionGraph::getOrCreateReferenceContent(SILValue refVal,
                                                              CGNode *refNode) {
-  // The object node points to internal fields. It neither has indirect pointsTo
-  // nor reference-only pointsTo.
+  // The object node created here points to internal fields. It neither has
+  // indirect pointsTo nor reference-only pointsTo.
   CGNode *objNode = getOrCreateContentNode(refNode, true, false);
   if (!objNode->isInterior())
     return objNode;
 
+  // Determine whether the object that refVal refers to only contains
+  // references.
   bool contentHasReferenceOnly = false;
   if (refVal) {
     SILType refType = refVal->getType();
@@ -972,24 +982,19 @@ EscapeAnalysis::ConnectionGraph::getOrCreateUnknownContent(CGNode *addrNode) {
 // on-the-fly.
 EscapeAnalysis::CGNode *
 EscapeAnalysis::ConnectionGraph::getValueContent(SILValue ptrVal) {
-  // Look past address projections, pointer casts, and the like within the same
-  // object. Does not look past a dereference such as ref_element_addr, or
-  // project_box.
-  SILValue ptrBase = EA->getPointerRoot(ptrVal);
-
-  PointerKind pointerKind = EA->getPointerKind(ptrBase);
-  if (pointerKind == EscapeAnalysis::NoPointer)
-    return nullptr;
-
-  CGNode *addrNode = getOrCreateNode(ptrBase, pointerKind);
+  CGNode *addrNode = getNode(ptrVal);
   if (!addrNode)
     return nullptr;
 
-  if (ptrBase->getType().isAddress())
-    return getOrCreateAddressContent(ptrBase, addrNode);
+  // Create content based on the derived pointer. If the base pointer contains
+  // other types of references, then the content node will be merged when those
+  // references are accessed. If the other references types are never accessed
+  // in this function, then they are ignored.
+  if (ptrVal->getType().isAddress())
+    return getOrCreateAddressContent(ptrVal, addrNode);
 
-  if (canOnlyContainReferences(pointerKind))
-    return getOrCreateReferenceContent(ptrBase, addrNode);
+  if (addrNode->hasReferenceOnly())
+    return getOrCreateReferenceContent(ptrVal, addrNode);
 
   // The pointer value may contain raw pointers.
   return getOrCreateUnknownContent(addrNode);
@@ -2075,10 +2080,12 @@ void EscapeAnalysis::analyzeInstruction(SILInstruction *I,
       // that may eventually be associated with 'fieldContent', so we must
       // assume here that 'fieldContent2' could hold raw pointers. This is
       // implied by passing in invalid SILValue.
-      CGNode *objNode2 =
+      CGNode *escapingNode =
           ConGraph->getOrCreateReferenceContent(SILValue(), fieldNode);
-      CGNode *fieldNode2 = objNode2->getContentNodeOrNull();
-      ConGraph->getOrCreateUnknownContent(fieldNode2)->markEscaping();
+      if (CGNode *indirectFieldNode = escapingNode->getContentNodeOrNull())
+        escapingNode = indirectFieldNode;
+
+      ConGraph->getOrCreateUnknownContent(escapingNode)->markEscaping();
       return;
     }
     case SILInstructionKind::DestroyAddrInst: {

test/SILOptimizer/escape_analysis.sil

@@ -323,7 +323,7 @@ bb0(%0 : $Pointer):
 // CHECK-NEXT:    Val %1 Esc: , Succ: (%1.1)
 // CHECK-NEXT:    Con [ref] %1.1 Esc: G, Succ: %4
 // CHECK-NEXT:    Con [int] %3 Esc: A, Succ: (%4)
-// CHECK-NEXT:    Con %4 Esc: G, Succ:
+// CHECK-NEXT:    Con [ref] %4 Esc: G, Succ:
 // CHECK-NEXT:  End
 sil  @store_content : $@convention(thin) (@owned Pointer) -> () {
 bb0(%0 : $Pointer):
@@ -896,7 +896,7 @@ bb0:
 // CHECK-NEXT:    Val [ref] %4 Esc: , Succ: %0, %1
 // CHECK-NEXT:    Val [ref] %7 Esc: , Succ: (%7.1), %4
 // CHECK-NEXT:    Con [int] %7.1 Esc: A, Succ: (%7.2)
-// CHECK-NEXT:    Con %7.2 Esc: A, Succ: 
+// CHECK-NEXT:    Con [ref] %7.2 Esc: A, Succ: 
 // CHECK-NEXT:    Ret [ref] return Esc: , Succ: %7
 // CHECK-NEXT:  End
 sil @pointer_types : $@convention(thin) (@owned Y, @owned Y) -> @owned Y {
@@ -1107,7 +1107,7 @@ bb0(%0 : $*X, %1 : $X):
 // CHECK-LABEL: CG of test_casts
 // CHECK-NEXT:    Arg [ref] %0 Esc: A, Succ:
 // CHECK-NEXT:    Con [int] %0.1 Esc: A, Succ: (%0.2)
-// CHECK-NEXT:    Con %0.2 Esc: A, Succ: 
+// CHECK-NEXT:    Con [ref] %0.2 Esc: A, Succ: 
 // CHECK-NEXT:    Ret [ref] return Esc: , Succ: %0
 // CHECK-NEXT:  End
 sil @test_casts : $@convention(thin) (@owned AnyObject) -> @owned X {
@@ -1817,3 +1817,30 @@ bb0(%0 : $IntWrapper):
   %tuple = tuple (%bridge : $Builtin.BridgeObject, %ump : $UnsafeMutablePointer<Int64>)
   return %tuple : $(Builtin.BridgeObject, UnsafeMutablePointer<Int64>)
 }
+
+// Test undef -> struct_extract -> bbarg
+// CHECK-LABEL: CG of testMappedUndef
+// CHECK-NEXT:   Val [ref] %4 Esc: , Succ: (%4.1)
+// CHECK-NEXT:   Con [int] %4.1 Esc: G, Succ: (%4.2)
+// CHECK-NEXT:   Con %4.2 Esc: G, Succ:
+// CHECK-LABEL: End
+struct StructWithObject {
+  @_hasStorage var obj: AnyObject { get set }
+  init(obj: AnyObject)
+}
+
+sil @testMappedUndef : $@convention(thin) () -> () {
+bb0:
+  cond_br undef, bb1, bb2
+
+bb1:
+  br bb3(undef : $AnyObject)
+
+bb2:
+  %2 = struct_extract undef : $StructWithObject, #StructWithObject.obj
+  br bb3(%2 : $AnyObject)
+
+bb3(%4 : $AnyObject):
+  %5 = tuple ()
+  return %5 : $()
+}