Merge pull request #28273 from atrick/escape-initializePointsTo

Cleanup EscapeAnalysis::ConnectionGraph::initializePointsTo.

Author: atrick

lib/SILOptimizer/Analysis/EscapeAnalysis.cpp

@@ -456,24 +456,18 @@ void EscapeAnalysis::ConnectionGraph::initializePointsTo(CGNode *initialNode,
                                                          CGNode *newPointsTo,
                                                          bool createEdge) {
   // Track nodes that require pointsTo edges.
-  llvm::SmallVector<CGNode *, 4> pointsToEdges;
+  llvm::SmallVector<CGNode *, 4> pointsToEdgeNodes;
+  if (createEdge)
+    pointsToEdgeNodes.push_back(initialNode);
 
   // Step 1: Visit each node that reaches or is reachable via defer edges until
   // reaching a node with the newPointsTo or with a proper pointsTo edge.
 
   // A worklist to gather updated nodes in the defer web.
   CGNodeWorklist updatedNodes(this);
-  updatedNodes.push(initialNode);
-  if (createEdge)
-    pointsToEdges.push_back(initialNode);
-  unsigned updateCount = 1;
-  assert(updateCount == updatedNodes.size());
-  // Augment the worlist with the nodes that were reached via backward
-  // traversal. It's not as precise as DFS, but helps avoid redundant pointsTo
-  // edges in most cases.
-  llvm::SmallPtrSet<CGNode *, 8> backwardReachable;
-
-  auto visitDeferTarget = [&](CGNode *node, bool isSuccessor) {
+  unsigned updateCount = 0;
+
+  auto visitDeferTarget = [&](CGNode *node, bool /*isSuccessor*/) {
     if (updatedNodes.contains(node))
       return true;
 
@@ -484,7 +478,7 @@ void EscapeAnalysis::ConnectionGraph::initializePointsTo(CGNode *initialNode,
       // nodes are initialized one at a time, each time a new defer edge is
       // created. If this were not complete, then the backward traversal below
       // in Step 2 could reach uninitialized nodes not seen here in Step 1.
-      pointsToEdges.push_back(node);
+      pointsToEdgeNodes.push_back(node);
       return true;
     }
     ++updateCount;
@@ -493,31 +487,29 @@ void EscapeAnalysis::ConnectionGraph::initializePointsTo(CGNode *initialNode,
       // edge. Create a "fake" pointsTo edge to maintain the graph invariant
       // (this changes the structure of the graph but adding this edge has no
       // effect on the process of merging nodes or creating new defer edges).
-      pointsToEdges.push_back(node);
+      pointsToEdgeNodes.push_back(node);
     }
     updatedNodes.push(node);
-    if (!isSuccessor)
-      backwardReachable.insert(node);
-
     return true;
   };
+  // Seed updatedNodes with initialNode.
+  visitDeferTarget(initialNode, true);
   // updatedNodes may grow during this loop.
-  unsigned nextUpdatedNodeIdx = 0;
-  for (; nextUpdatedNodeIdx < updatedNodes.size(); ++nextUpdatedNodeIdx)
-    updatedNodes[nextUpdatedNodeIdx]->visitDefers(visitDeferTarget);
+  for (unsigned idx = 0; idx < updatedNodes.size(); ++idx)
+    updatedNodes[idx]->visitDefers(visitDeferTarget);
   // Reset this worklist so others can be used, but updateNode.nodeVector still
   // holds all the nodes found by step 1.
   updatedNodes.reset();
 
   // Step 2: Update pointsTo fields by propagating backward from nodes that
   // already have a pointsTo edge.
-  assert(nextUpdatedNodeIdx == updatedNodes.size());
-  --nextUpdatedNodeIdx;
-  bool processBackwardReachable = false;
   do {
-    while (!pointsToEdges.empty()) {
-      CGNode *edgeNode = pointsToEdges.pop_back_val();
+    while (!pointsToEdgeNodes.empty()) {
+      CGNode *edgeNode = pointsToEdgeNodes.pop_back_val();
       if (!edgeNode->pointsTo) {
+        // This node is either (1) a leaf node in the defer web (identified in
+        // step 1) or (2) an arbitrary node in a defer-cycle (identified in a
+        // previous iteration of the outer loop).
         edgeNode->setPointsToEdge(newPointsTo);
         newPointsTo->mergeUsePoints(edgeNode);
         assert(updateCount--);
@@ -542,35 +534,19 @@ void EscapeAnalysis::ConnectionGraph::initializePointsTo(CGNode *initialNode,
         return Traversal::Follow;
       });
     }
-    // For all nodes visited in step 1, if any node was not backward-reachable
-    // from a pointsTo edge, create an edge for it and restart traversal.
-    //
-    // First process all forward-reachable nodes in backward order, then process
-    // all backwardReachable nodes in forward order.
-    while (nextUpdatedNodeIdx != updatedNodes.size()) {
-      CGNode *node = updatedNodes[nextUpdatedNodeIdx];
-      // When processBackwardReachable == true, the backwardReachable set is
-      // empty and all forward reachable nodes already have a pointsTo edge.
-      if (!backwardReachable.count(node)) {
-        if (!node->pointsTo) {
-          pointsToEdges.push_back(node);
-          break;
-        }
-      }
-      if (processBackwardReachable) {
-        ++nextUpdatedNodeIdx;
-        continue;
-      }
-      if (nextUpdatedNodeIdx > 0) {
-        --nextUpdatedNodeIdx;
-        continue;
+    // For all nodes visited in step 1, pick a single node that was not
+    // backward-reachable from a pointsTo edge, create an edge for it and
+    // restart traversal. This only happens when step 1 fails to find leaves in
+    // the defer web because of defer edge cycles.
+    while (!updatedNodes.empty()) {
+      CGNode *node = updatedNodes.nodeVector.pop_back_val();
+      if (!node->pointsTo) {
+        pointsToEdgeNodes.push_back(node);
+        break;
       }
-      // reverse direction
-      backwardReachable.clear();
-      processBackwardReachable = true;
     }
     // This outer loop is exceedingly unlikely to execute more than twice.
-  } while (!pointsToEdges.empty());
+  } while (!pointsToEdgeNodes.empty());
   assert(updateCount == 0);
 }
 

test/SILOptimizer/escape_analysis.sil

@@ -1535,3 +1535,123 @@ bb0:
   return %7 : $()
 }
 
+// Test the absence of redundant pointsTo edges
+// CHECK-LABEL: CG of testInitializePointsToLeaf
+// CHECK:   Arg %0 Esc: A, Succ: (%0.1)
+// CHECK:   Con %0.1 Esc: A, Succ: (%0.2) [rc]
+// CHECK:   Con %0.2 Esc: A, Succ: (%12.1)
+// CHECK:   Val %2 Esc: %4, Succ: %0.2
+// CHECK:   Val %4 Esc: %4, Succ: %2
+// CHECK:   Val %7 Esc: %12, Succ: (%12.1), %0.2
+// CHECK:   Val %12 Esc: %12, Succ: (%12.1), %7
+// CHECK:   Con %12.1 Esc: A, Succ: (%13) [rc]
+// CHECK:   Con %13 Esc: A, Succ:
+// CHECK-LABEL: End
+class C {
+  var c: C
+}
+
+sil @testInitializePointsToWrapOptional : $@convention(method) (@guaranteed LinkedNode) -> Optional<LinkedNode> {
+bb0(%0: $LinkedNode):
+  %adr = ref_element_addr %0 : $LinkedNode, #LinkedNode.next
+  %val = load %adr : $*LinkedNode
+  %optional = enum $Optional<LinkedNode>, #Optional.some!enumelt.1, %val : $LinkedNode
+  return %optional : $Optional<LinkedNode>
+}
+
+sil @testInitializePointsToLeaf : $@convention(method) (@guaranteed LinkedNode) -> () {
+bb0(%0 : $LinkedNode):
+  %f1 = function_ref @testInitializePointsToWrapOptional : $@convention(method) (@guaranteed LinkedNode) -> Optional<LinkedNode>
+  %call1 = apply %f1(%0) : $@convention(method) (@guaranteed LinkedNode) -> Optional<LinkedNode>
+  switch_enum %call1 : $Optional<LinkedNode>, case #Optional.some!enumelt.1: bb2, case #Optional.none!enumelt: bb3
+
+bb2(%arg1 : $LinkedNode):
+  br bb4
+
+bb3:
+  br bb4
+
+bb4:
+  %call2 = apply %f1(%0) : $@convention(method) (@guaranteed LinkedNode) -> Optional<LinkedNode>
+  switch_enum %call2 : $Optional<LinkedNode>, case #Optional.some!enumelt.1: bb10, case #Optional.none!enumelt: bb9
+
+bb9:
+  %37 = integer_literal $Builtin.Int1, -1
+  cond_fail %37 : $Builtin.Int1, "Unexpectedly found nil while unwrapping an Optional value"
+  unreachable
+
+// %40
+bb10(%arg2 : $LinkedNode):
+  %adr = ref_element_addr %arg2 : $LinkedNode, #LinkedNode.next
+  %val = load %adr : $*LinkedNode
+  %66 = tuple ()
+  return %66 : $()
+}
+
+// Another test for redundant pointsTo edges. In the original
+// implementation, redundant points edges were created whenever adding
+// a defer edge from a node with uninitialized pointsTo to a node with
+// already-initialized pointsTo.
+// CHECK-LABEL: CG of testInitializePointsToRedundant
+// CHECK:   Arg %0 Esc: A, Succ: (%0.1)
+// CHECK:   Con %0.1 Esc: A, Succ: (%2) [rc]
+// CHECK:   Arg %1 Esc: A, Succ: (%0.1)
+// CHECK:   Con %2 Esc: A, Succ:
+// CHECK:   Val %7 Esc: %7,%18, Succ: %0
+// CHECK:   Val %12 Esc: %12,%14,%18, Succ: %1
+// CHECK:   Val %14 Esc: %18, Succ: (%0.1), %1, %12
+// CHECK:   Val %18 Esc: %18, Succ: %7, %14
+// CHECK-LABEL: End
+sil @testInitializePointsToMerge : $@convention(method) (@guaranteed C, @guaranteed C) -> C {
+bb0(%0: $C, %1 : $C):
+  cond_br undef, bb1, bb2
+
+bb1:
+  br bb3(%0 : $C)
+
+bb2:
+  br bb3(%1 : $C)
+
+bb3(%arg : $C):
+  return %arg : $C
+}
+
+sil @testInitializePointsToRedundant : $@convention(method) (@guaranteed C, @guaranteed C) -> () {
+bb0(%0 : $C, %1 : $C):
+  %adr0 = ref_element_addr %0 : $C, #C.c
+  %val0 = load %adr0 : $*C
+  cond_br undef, bb1, bb2
+
+bb1:
+  br bb3(%0 : $C)
+
+bb2:
+  br bb3(%0 : $C)
+
+bb3(%arg1 : $C):
+  br bb4
+
+bb4:
+  cond_br undef, bb5, bb6
+
+bb5:
+  br bb7(%1 : $C)
+
+bb6:
+  br bb7(%1 : $C)
+
+bb7(%arg2 : $C):
+  %f1 = function_ref @testInitializePointsToMerge : $@convention(method) (@guaranteed C, @guaranteed C) -> C
+  %call1 = apply %f1(%arg2, %1) : $@convention(method) (@guaranteed C, @guaranteed C) -> C
+  cond_br undef, bb8, bb9
+
+bb8:
+  br bb10(%call1 : $C)
+
+bb9:
+  br bb10(%arg1 : $C)
+
+bb10(%arg3 : $C):
+  %66 = tuple ()
+  return %66 : $()
+}