[GR-43722] Refactor method flows graph init.

cstancu · cstancu · commit c4feee44063f · 2023-02-08T03:44:17.000Z
PullRequest: graal/13674
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/PointsToAnalysis.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/PointsToAnalysis.java
@@ -181,7 +181,7 @@ public void forceUnsafeUpdate(AnalysisField field) {
         // force update of the unsafe loads
         for (AbstractUnsafeLoadTypeFlow unsafeLoad : unsafeLoads.keySet()) {
             /* Force update for unsafe accessed static fields. */
-            unsafeLoad.initFlow(this);
+            unsafeLoad.forceUpdate(this);
 
             /*
              * Force update for unsafe accessed instance fields: post the receiver object flow for
@@ -194,7 +194,7 @@ public void forceUnsafeUpdate(AnalysisField field) {
         // force update of the unsafe stores
         for (AbstractUnsafeStoreTypeFlow unsafeStore : unsafeStores.keySet()) {
             /* Force update for unsafe accessed static fields. */
-            unsafeStore.initFlow(this);
+            unsafeStore.forceUpdate(this);
 
             /*
              * Force update for unsafe accessed instance fields: post the receiver object flow for
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/AbstractStaticInvokeTypeFlow.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/AbstractStaticInvokeTypeFlow.java
@@ -41,6 +41,17 @@ protected AbstractStaticInvokeTypeFlow(PointsToAnalysis bb, MethodFlowsGraph met
         super(bb, methodFlows, original);
     }
 
+    @Override
+    public void initFlow(PointsToAnalysis bb) {
+        /* Trigger the update for static invokes, there is no receiver to trigger it. */
+        bb.postFlow(this);
+    }
+
+    @Override
+    public boolean needsInitialization() {
+        return true;
+    }
+
     @Override
     public String toString() {
         return "StaticInvoke<" + targetMethod.format("%h.%n") + ">" + ":" + getState();
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/CallSiteSensitiveMethodTypeFlow.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/CallSiteSensitiveMethodTypeFlow.java
@@ -25,6 +25,7 @@
 package com.oracle.graal.pointsto.flow;
 
 import java.util.Collection;
+import java.util.List;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 
@@ -81,7 +82,7 @@ public MethodFlowsGraphInfo addContext(PointsToAnalysis bb, AnalysisContext call
     }
 
     @Override
-    protected void initFlowsGraph(PointsToAnalysis bb) {
+    protected void initFlowsGraph(PointsToAnalysis bb, List<TypeFlow<?>> postInitFlows) {
         // nothing to do, cloning does all the initialization
     }
 
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/ConstantTypeFlow.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/ConstantTypeFlow.java
@@ -66,6 +66,11 @@ public void initFlow(PointsToAnalysis bb) {
         addState(bb, constantState);
     }
 
+    @Override
+    public boolean needsInitialization() {
+        return true;
+    }
+
     @Override
     public String toString() {
         return "ConstantFlow<" + getState() + ">";
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/DynamicNewInstanceTypeFlow.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/DynamicNewInstanceTypeFlow.java
@@ -70,6 +70,11 @@ public void initFlow(PointsToAnalysis bb) {
         this.newTypeFlow.addObserver(bb, this);
     }
 
+    @Override
+    public boolean needsInitialization() {
+        return true;
+    }
+
     @Override
     public void onObservedUpdate(PointsToAnalysis bb) {
         /* The state of the new type provider has changed. */
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/InvokeTypeFlow.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/InvokeTypeFlow.java
@@ -183,6 +183,10 @@ protected void updateReceiver(PointsToAnalysis bb, MethodFlowsGraphInfo calleeFl
             if (bb.optimizeReturnedParameter()) {
                 int paramIndex = calleeFlows.getMethod().getTypeFlow().getReturnedParameterIndex();
                 if (actualReturn != null && paramIndex == 0) {
+                    /*
+                     * The callee returns `this`. Propagate the receiver state to the actual-return.
+                     * See also InvokeTypeFlow#linkReturn() for more details.
+                     */
                     actualReturn.addState(bb, receiverTypeState);
                 }
             }
@@ -240,10 +244,21 @@ public void linkReturn(PointsToAnalysis bb, boolean isStatic, MethodFlowsGraphIn
                         if (isStatic || paramNodeIndex != 0) {
                             TypeFlow<?> actualParam = actualParameters[paramNodeIndex];
                             actualParam.addUse(bb, actualReturn);
+                        } else {
+                            /*
+                             * The callee returns `this`. The formal-receiver state is updated in
+                             * InvokeTypeFlow#updateReceiver() for each linked callee and every time
+                             * the formal-receiver is updated then the same update state is
+                             * propagated to the actual-return. One may think that we could simply
+                             * add a direct use link from the formal-receiver in the callee to the
+                             * actual-return in the caller to get the state propagation
+                             * automatically. But that would be wrong because then the actual-return
+                             * would get the state from *all* the other places that callee may be
+                             * called from, and that would defeat the purpose of this optimization:
+                             * we want just the receiver state from the caller of current invoke to
+                             * reach the actual-return.
+                             */
                         }
-                        // else {
-                        // receiver object state is transferred in updateReceiver()
-                        // }
                     } else {
                         /*
                          * The callee may have a return type, hence the actualReturn is non-null,
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/LoadFieldTypeFlow.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/LoadFieldTypeFlow.java
@@ -74,6 +74,11 @@ public void initFlow(PointsToAnalysis bb) {
             fieldFlow.addUse(bb, this);
         }
 
+        @Override
+        public boolean needsInitialization() {
+            return true;
+        }
+
         @Override
         public String toString() {
             return "LoadStaticFieldTypeFlow<" + getState() + ">";
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodFlowsGraph.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodFlowsGraph.java
@@ -41,8 +41,6 @@
 import org.graalvm.compiler.nodes.EncodedGraph.EncodedNodeReference;
 
 import com.oracle.graal.pointsto.PointsToAnalysis;
-import com.oracle.graal.pointsto.flow.OffsetLoadTypeFlow.AbstractUnsafeLoadTypeFlow;
-import com.oracle.graal.pointsto.flow.OffsetStoreTypeFlow.AbstractUnsafeStoreTypeFlow;
 import com.oracle.graal.pointsto.meta.AnalysisMethod;
 import com.oracle.graal.pointsto.meta.PointsToAnalysisMethod;
 import com.oracle.graal.pointsto.util.AnalysisError;
@@ -113,28 +111,6 @@ public <T extends TypeFlow<?>> T lookupCloneOf(@SuppressWarnings("unused") Point
         return original;
     }
 
-    public void init(final PointsToAnalysis bb) {
-        for (TypeFlow<?> flow : flows()) {
-            if (flow instanceof AbstractUnsafeLoadTypeFlow) {
-                bb.registerUnsafeLoad((AbstractUnsafeLoadTypeFlow) flow);
-            }
-            if (flow instanceof AbstractUnsafeStoreTypeFlow) {
-                bb.registerUnsafeStore((AbstractUnsafeStoreTypeFlow) flow);
-            }
-
-            /*
-             * Run initialization code for corner case type flows. This can be used to add link from
-             * 'outside' into the graph.
-             */
-            flow.initFlow(bb);
-
-            /* Trigger the update for static invokes, there is no receiver to trigger it. */
-            if (flow instanceof AbstractStaticInvokeTypeFlow) {
-                bb.postFlow(flow);
-            }
-        }
-    }
-
     protected static boolean nonCloneableFlow(TypeFlow<?> flow) {
         /*
          * References to field flows and to array elements flows are not part of the method itself;
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodFlowsGraphClone.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodFlowsGraphClone.java
@@ -120,12 +120,6 @@ private <V extends TypeFlow<?>> List<V> lookupClonesOf(PointsToAnalysis bb, List
         return result;
     }
 
-    @Override
-    public void init(final PointsToAnalysis bb) {
-        // the cloning mechanism does all the initialization
-        throw AnalysisError.shouldNotReachHere();
-    }
-
     @Override
     @SuppressWarnings("unchecked")
     public <T extends TypeFlow<?>> T lookupCloneOf(PointsToAnalysis bb, T original) {
@@ -175,7 +169,9 @@ void linkCloneFlows(final PointsToAnalysis bb) {
              * Run initialization code for corner case type flows. This can be used to add link from
              * 'outside' into the graph.
              */
-            clone.initFlow(bb);
+            if (clone.needsInitialization()) {
+                clone.initFlow(bb);
+            }
 
             /* Link all 'internal' observers. */
             for (TypeFlow<?> originalObserver : original.getObservers()) {
@@ -210,12 +206,6 @@ void linkCloneFlows(final PointsToAnalysis bb) {
                     clone.addUse(bb, clonedUse);
                 }
             }
-
-            if (clone instanceof AbstractStaticInvokeTypeFlow) {
-                /* Trigger the update for static invokes, there is no receiver to trigger it. */
-                AbstractStaticInvokeTypeFlow invokeFlow = (AbstractStaticInvokeTypeFlow) clone;
-                bb.postFlow(invokeFlow);
-            }
         }
     }
 
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlow.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlow.java
@@ -39,6 +39,7 @@
 import org.graalvm.compiler.nodes.ValueNode;
 
 import com.oracle.graal.pointsto.PointsToAnalysis;
+import com.oracle.graal.pointsto.flow.builder.TypeFlowGraphBuilder;
 import com.oracle.graal.pointsto.meta.AnalysisMethod;
 import com.oracle.graal.pointsto.meta.PointsToAnalysisMethod;
 import com.oracle.graal.pointsto.typestate.TypeState;
@@ -171,7 +172,7 @@ private synchronized void createFlowsGraph(PointsToAnalysis bb, InvokeTypeFlow r
                 flowsGraph = builder.flowsGraph;
                 assert flowsGraph != null;
 
-                initFlowsGraph(bb);
+                initFlowsGraph(bb, builder.postInitFlows);
             } catch (Throwable t) {
                 /* Wrap all other errors as parsing errors. */
                 throw AnalysisError.parsingError(method, t);
@@ -200,8 +201,18 @@ private static int computeReturnedParameterIndex(StructuredGraph graph) {
         }
     }
 
-    protected void initFlowsGraph(PointsToAnalysis bb) {
-        flowsGraph.init(bb);
+    /**
+     * Run type flow initialization. This will trigger state propagation from source flows, link
+     * static load/store field flows, publish unsafe load/store flows, etc. The flows that need
+     * initialization are collected by {@link TypeFlowGraphBuilder#build()}. Their initialization
+     * needs to be triggered only after the graph is fully materialized such that lazily constructed
+     * type flows (like InovkeTypeFlow.actualReturn) can observe the type state that other flows may
+     * generate on initialization.
+     */
+    protected void initFlowsGraph(PointsToAnalysis bb, List<TypeFlow<?>> postInitFlows) {
+        for (TypeFlow<?> flow : postInitFlows) {
+            flow.initFlow(bb);
+        }
     }
 
     public Collection<MethodFlowsGraph> getFlows() {
@@ -318,7 +329,7 @@ public synchronized boolean updateFlowsGraph(PointsToAnalysis bb, MethodFlowsGra
 
             flowsGraph.updateInternalState(newGraphKind);
 
-            initFlowsGraph(bb);
+            initFlowsGraph(bb, builder.postInitFlows);
 
             if (registerAsImplementationInvoked) {
                 if (parsingReason == null) {
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlowBuilder.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/MethodTypeFlowBuilder.java
@@ -161,6 +161,7 @@ public class MethodTypeFlowBuilder {
     private final boolean newFlowsGraph;
 
     protected final TypeFlowGraphBuilder typeFlowGraphBuilder;
+    protected List<TypeFlow<?>> postInitFlows = List.of();
 
     public MethodTypeFlowBuilder(PointsToAnalysis bb, PointsToAnalysisMethod method, MethodFlowsGraph flowsGraph, GraphKind graphKind) {
         this.bb = bb;
@@ -555,8 +556,8 @@ private void createTypeFlow() {
         // Propagate the type flows through the method's graph
         new NodeIterator(graph.start(), typeFlows).apply();
 
-        /* Prune the method graph. Eliminate nodes with no uses. */
-        typeFlowGraphBuilder.build();
+        /* Prune the method graph. Eliminate nodes with no uses. Collect flows that need init. */
+        postInitFlows = typeFlowGraphBuilder.build();
 
         /*
          * Make sure that all existing InstanceOfNodes are registered even when only used as an
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/NewInstanceTypeFlow.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/NewInstanceTypeFlow.java
@@ -74,6 +74,11 @@ public void initFlow(PointsToAnalysis bb) {
         }
     }
 
+    @Override
+    public boolean needsInitialization() {
+        return true;
+    }
+
     NewInstanceTypeFlow(PointsToAnalysis bb, NewInstanceTypeFlow original, MethodFlowsGraph methodFlows) {
         super(original, methodFlows, original.createCloneState(bb, methodFlows));
     }
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/OffsetLoadTypeFlow.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/OffsetLoadTypeFlow.java
@@ -152,18 +152,22 @@ public abstract static class AbstractUnsafeLoadTypeFlow extends OffsetLoadTypeFl
         }
 
         @Override
-        public final AbstractUnsafeLoadTypeFlow copy(PointsToAnalysis bb, MethodFlowsGraph methodFlows) {
-            AbstractUnsafeLoadTypeFlow copy = makeCopy(bb, methodFlows);
-            // Register the unsafe load. It will be force-updated when new unsafe fields are
-            // registered. Only the clones are registered since the original flows are not updated.
-            bb.registerUnsafeLoad(copy);
-            return copy;
+        public void initFlow(PointsToAnalysis bb) {
+            assert !bb.analysisPolicy().isContextSensitiveAnalysis() || this.isClone();
+            /*
+             * Register the unsafe load. It will be force-updated when new unsafe fields are
+             * registered.
+             */
+            bb.registerUnsafeLoad(this);
+            forceUpdate(bb);
         }
 
-        protected abstract AbstractUnsafeLoadTypeFlow makeCopy(PointsToAnalysis bb, MethodFlowsGraph methodFlows);
-
         @Override
-        public void initFlow(PointsToAnalysis bb) {
+        public boolean needsInitialization() {
+            return true;
+        }
+
+        public void forceUpdate(PointsToAnalysis bb) {
             /*
              * Unsafe load type flow models unsafe reads from both instance and static fields. From
              * an analysis stand point for static fields the base doesn't matter. An unsafe load can
@@ -186,7 +190,7 @@ private UnsafeLoadTypeFlow(PointsToAnalysis bb, MethodFlowsGraph methodFlows, Un
         }
 
         @Override
-        public UnsafeLoadTypeFlow makeCopy(PointsToAnalysis bb, MethodFlowsGraph methodFlows) {
+        public AbstractUnsafeLoadTypeFlow copy(PointsToAnalysis bb, MethodFlowsGraph methodFlows) {
             return new UnsafeLoadTypeFlow(bb, methodFlows, this);
         }
 
@@ -244,7 +248,7 @@ private UnsafePartitionLoadTypeFlow(PointsToAnalysis bb, MethodFlowsGraph method
         }
 
         @Override
-        public UnsafePartitionLoadTypeFlow makeCopy(PointsToAnalysis bb, MethodFlowsGraph methodFlows) {
+        public AbstractUnsafeLoadTypeFlow copy(PointsToAnalysis bb, MethodFlowsGraph methodFlows) {
             return new UnsafePartitionLoadTypeFlow(bb, methodFlows, this);
         }
 
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/OffsetStoreTypeFlow.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/OffsetStoreTypeFlow.java
@@ -192,17 +192,28 @@ public abstract static class AbstractUnsafeStoreTypeFlow extends OffsetStoreType
 
         @Override
         public final AbstractUnsafeStoreTypeFlow copy(PointsToAnalysis bb, MethodFlowsGraph methodFlows) {
-            AbstractUnsafeStoreTypeFlow copy = makeCopy(bb, methodFlows);
-            // Register the unsafe store. It will be force-updated when new unsafe fields are
-            // registered. Only the clones are registered since the original flows are not updated.
-            bb.registerUnsafeStore(copy);
-            return copy;
+            return makeCopy(bb, methodFlows);
         }
 
         protected abstract AbstractUnsafeStoreTypeFlow makeCopy(PointsToAnalysis bb, MethodFlowsGraph methodFlows);
 
         @Override
         public void initFlow(PointsToAnalysis bb) {
+            assert !bb.analysisPolicy().isContextSensitiveAnalysis() || this.isClone();
+            /*
+             * Register the unsafe store. It will be force-updated when new unsafe fields are
+             * registered.
+             */
+            bb.registerUnsafeStore(this);
+            forceUpdate(bb);
+        }
+
+        @Override
+        public boolean needsInitialization() {
+            return true;
+        }
+
+        public void forceUpdate(PointsToAnalysis bb) {
             /*
              * Unsafe store type flow models unsafe writes to both instance and static fields. From
              * an analysis stand point for static fields the base doesn't matter. An unsafe store
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/SourceTypeFlow.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/SourceTypeFlow.java
@@ -73,6 +73,11 @@ public void initFlow(PointsToAnalysis bb) {
         declaredType.registerInstantiatedCallback(a -> addState(bb, TypeState.forExactType(bb, declaredType, false)));
     }
 
+    @Override
+    public boolean needsInitialization() {
+        return true;
+    }
+
     @Override
     public void onObservedSaturated(PointsToAnalysis bb, TypeFlow<?> observed) {
         AnalysisError.shouldNotReachHere("NewInstanceTypeFlow cannot saturate.");
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/StoreFieldTypeFlow.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/StoreFieldTypeFlow.java
@@ -85,6 +85,11 @@ public void initFlow(PointsToAnalysis bb) {
             this.addUse(bb, fieldFlow);
         }
 
+        @Override
+        public boolean needsInitialization() {
+            return true;
+        }
+
         @Override
         public String toString() {
             return "StoreStaticFieldTypeFlow<" + getState() + ">";
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/TypeFlow.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/TypeFlow.java
diff --git a/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/builder/TypeFlowGraphBuilder.java b/substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/flow/builder/TypeFlowGraphBuilder.java

Original file line number	Diff line number	Diff line change
`@@ -25,6 +25,7 @@`
`25`	`25`	`package com.oracle.graal.pointsto.flow;`
`26`	`26`
`27`	`27`	`import java.util.Collection;`
	`28`	`+import java.util.List;`
`28`	`29`	`import java.util.concurrent.ConcurrentHashMap;`
`29`	`30`	`import java.util.concurrent.ConcurrentMap;`
`30`	`31`
`@@ -81,7 +82,7 @@ public MethodFlowsGraphInfo addContext(PointsToAnalysis bb, AnalysisContext call`
`81`	`82`	`}`
`82`	`83`
`83`	`84`	`@Override`
`84`		`- protected void initFlowsGraph(PointsToAnalysis bb) {`
	`85`	`+ protected void initFlowsGraph(PointsToAnalysis bb, List<TypeFlow<?>> postInitFlows) {`
`85`	`86`	`// nothing to do, cloning does all the initialization`
`86`	`87`	`}`
`87`	`88`