[GR-55073] Fix open-type world analysis.

PullRequest: graal/18580

Author: cstancu

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/PhaseSuite.java

@@ -35,12 +35,12 @@
 import java.util.function.Supplier;
 
 import jdk.graal.compiler.core.common.util.PhasePlan;
+import jdk.graal.compiler.debug.GraalError;
 import jdk.graal.compiler.debug.TTY;
 import jdk.graal.compiler.nodes.GraphState;
 import jdk.graal.compiler.nodes.StructuredGraph;
 import jdk.graal.compiler.options.Option;
 import jdk.graal.compiler.options.OptionKey;
-
 import jdk.graal.compiler.serviceprovider.GraalServices;
 
 /**
@@ -102,6 +102,21 @@ public final void addBeforeLast(BasePhase<? super C> phase) {
         last.add(phase);
     }
 
+    /** The new phase is inserted before the first position the target phase is found at. */
+    public final void insertBeforePhase(Class<? extends BasePhase<? super C>> phaseClass, BasePhase<? super C> newPhase) {
+        ListIterator<BasePhase<? super C>> position = findPhase(phaseClass);
+        GraalError.guarantee(position != null, "Phase %s not found in suite %s.", phaseClass.getName(), this.getName());
+        position.previous();
+        position.add(newPhase);
+    }
+
+    /** The new phase is inserted after the first position the target phase is found at. */
+    public final void insertAfterPhase(Class<? extends BasePhase<? super C>> phaseClass, BasePhase<? super C> newPhase) {
+        ListIterator<BasePhase<? super C>> position = findPhase(phaseClass);
+        GraalError.guarantee(position != null, "Phase %s not found in suite %s.", phaseClass.getName(), this.getName());
+        position.add(newPhase);
+    }
+
     /**
      * Insert a new phase at the specified index and shifts the element currently at that position
      * and any subsequent elements to the right.

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/virtual/phases/ea/PartialEscapeClosure.java

@@ -69,6 +69,7 @@
 import jdk.graal.compiler.nodes.ValuePhiNode;
 import jdk.graal.compiler.nodes.ValueProxyNode;
 import jdk.graal.compiler.nodes.VirtualState;
+import jdk.graal.compiler.nodes.WithExceptionNode;
 import jdk.graal.compiler.nodes.cfg.HIRBlock;
 import jdk.graal.compiler.nodes.java.AbstractNewObjectNode;
 import jdk.graal.compiler.nodes.java.AccessMonitorNode;
@@ -311,7 +312,11 @@ private boolean processVirtualizable(ValueNode node, FixedNode insertBefore, Blo
                         if (state.hasObjectState(id)) {
                             FixedNode materializeBefore = insertBefore;
                             if (insertBefore == node && tool.isDeleted()) {
-                                materializeBefore = ((FixedWithNextNode) insertBefore).next();
+                                if (insertBefore instanceof FixedWithNextNode withNextNode) {
+                                    materializeBefore = withNextNode.next();
+                                } else {
+                                    materializeBefore = ((WithExceptionNode) insertBefore).next();
+                                }
                             }
                             ensureMaterialized(state, id, materializeBefore, effects, COUNTER_MATERIALIZATIONS);
                         }

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/api/HostVM.java

@@ -332,6 +332,10 @@ public boolean isFieldIncluded(BigBang bb, Field field) {
         return true;
     }
 
+    public boolean isClosedTypeWorld() {
+        return true;
+    }
+
     /**
      * Helpers to determine what analysis actions should be taken for a given Multi-Method version.
      */

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/meta/AnalysisMethod.java

@@ -911,6 +911,10 @@ public ResolvedJavaMethod unwrapTowardsOriginalMethod() {
     }
 
     public Executable getJavaMethod() {
+        if (wrapped instanceof BaseLayerMethod) {
+            /* We don't know the corresponding Java method. */
+            return null;
+        }
         return OriginalMethodProvider.getJavaMethod(this);
     }
 

substratevm/src/com.oracle.graal.pointsto/src/com/oracle/graal/pointsto/results/StrengthenGraphs.java

@@ -36,6 +36,7 @@
 import java.util.stream.Stream;
 
 import org.graalvm.collections.EconomicSet;
+import org.graalvm.nativeimage.AnnotationAccess;
 
 import com.oracle.graal.pointsto.BigBang;
 import com.oracle.graal.pointsto.PointsToAnalysis;
@@ -53,6 +54,8 @@
 import com.oracle.graal.pointsto.meta.PointsToAnalysisMethod;
 import com.oracle.graal.pointsto.typestate.PrimitiveConstantTypeState;
 import com.oracle.graal.pointsto.typestate.TypeState;
+import com.oracle.graal.pointsto.util.AnalysisError;
+import com.oracle.svm.core.annotate.Delete;
 import com.oracle.svm.util.ImageBuildStatistics;
 
 import jdk.graal.compiler.core.common.type.AbstractObjectStamp;
@@ -182,6 +185,9 @@ public boolean equals(Object obj) {
     private final StrengthenGraphsCounters beforeCounters;
     private final StrengthenGraphsCounters afterCounters;
 
+    /** Used to avoid aggressive optimizations for open type world analysis. */
+    private final boolean isClosedTypeWorld;
+
     public StrengthenGraphs(BigBang bb, Universe converter) {
         this.bb = bb;
         this.converter = converter;
@@ -197,6 +203,7 @@ public StrengthenGraphs(BigBang bb, Universe converter) {
             beforeCounters = null;
             afterCounters = null;
         }
+        this.isClosedTypeWorld = converter.hostVM().isClosedTypeWorld();
     }
 
     private static void reportNeverNullInstanceFields(BigBang bb) {
@@ -291,7 +298,7 @@ public final void applyResults(AnalysisMethod method) {
     protected abstract boolean simplifyDelegate(Node n, SimplifierTool tool);
 
     /* Wrapper to clearly identify phase in IGV graph dumps. */
-    class AnalysisStrengthenGraphsPhase extends BasePhase<CoreProviders> {
+    public class AnalysisStrengthenGraphsPhase extends BasePhase<CoreProviders> {
         final CanonicalizerPhase phase;
 
         AnalysisStrengthenGraphsPhase(AnalysisMethod method, StructuredGraph graph) {
@@ -554,7 +561,8 @@ private void handleInvoke(Invoke invoke, SimplifierTool tool) {
             FixedNode node = invoke.asFixedNode();
             MethodCallTargetNode callTarget = (MethodCallTargetNode) invoke.callTarget();
 
-            if (callTarget.invokeKind().isDirect() && !((AnalysisMethod) callTarget.targetMethod()).isSimplyImplementationInvoked()) {
+            AnalysisMethod targetMethod = (AnalysisMethod) callTarget.targetMethod();
+            if (callTarget.invokeKind().isDirect() && !targetMethod.isSimplyImplementationInvoked()) {
                 /*
                  * This is a direct call to a method that the static analysis did not see as
                  * invoked. This can happen when the receiver is always null. In most cases, the
@@ -619,27 +627,66 @@ private void handleInvoke(Invoke invoke, SimplifierTool tool) {
                 }
             }
 
-            if (callees.size() == 1) {
+            if (callTarget.invokeKind().isDirect()) {
+                /*
+                 * Note: A direct invoke doesn't necessarily imply that the analysis should have
+                 * discovered a single callee. When dealing with interfaces it is in fact possible
+                 * that the Graal stamps are more accurate than the analysis results. So an
+                 * interface call may have already been optimized to a special call by stamp
+                 * strengthening of the receiver object, hence the invoke kind is direct, whereas
+                 * the points-to analysis inaccurately concluded there can be more than one callee.
+                 * 
+                 * Below we just check that if there is a direct invoke *and* the analysis
+                 * discovered a single callee, then the callee should match the target method.
+                 */
+                if (callees.size() == 1) {
+                    AnalysisMethod singleCallee = callees.iterator().next();
+                    assert targetMethod.equals(singleCallee) : "Direct invoke target mismatch: " + targetMethod + " != " + singleCallee + ". Called from " + graph.method().format("%H.%n");
+                }
+            } else if (AnnotationAccess.isAnnotationPresent(targetMethod, Delete.class)) {
+                /* We de-virtualize invokes to deleted methods since the callee must be unique. */
+                AnalysisError.guarantee(callees.size() == 1, "@Delete methods should have a single callee.");
                 AnalysisMethod singleCallee = callees.iterator().next();
-                if (callTarget.invokeKind().isDirect()) {
-                    assert callTarget.targetMethod().equals(singleCallee) : "Direct invoke target mismatch: " + callTarget.targetMethod() + " != " + singleCallee;
-                } else {
+                devirtualizeInvoke(singleCallee, invoke);
+            } else if (targetMethod.canBeStaticallyBound() || isClosedTypeWorld) {
+                /*
+                 * We only de-virtualize invokes if we run a closed type world analysis or the
+                 * target method can be trivially statically bound.
+                 */
+                if (callees.size() == 1) {
+                    AnalysisMethod singleCallee = callees.iterator().next();
                     devirtualizeInvoke(singleCallee, invoke);
-                }
+                } else {
+                    TypeState receiverTypeState = null;
+                    /* If the receiver flow is saturated, its exact type state does not matter. */
+                    if (invokeFlow.getTargetMethod().hasReceiver() && !methodFlow.isSaturated((PointsToAnalysis) bb, invokeFlow.getReceiver())) {
+                        receiverTypeState = methodFlow.foldTypeFlow((PointsToAnalysis) bb, invokeFlow.getReceiver());
+                    }
 
-            } else {
-                TypeState receiverTypeState = null;
-                /* If the receiver flow is saturated, its exact type state does not matter. */
-                if (invokeFlow.getTargetMethod().hasReceiver() && !methodFlow.isSaturated((PointsToAnalysis) bb, invokeFlow.getReceiver())) {
-                    receiverTypeState = methodFlow.foldTypeFlow((PointsToAnalysis) bb, invokeFlow.getReceiver());
-                }
+                    /*
+                     * In an open type world we cannot trust the type state of the receiver for
+                     * virtual calls as new subtypes could be added later.
+                     *
+                     * Note: MethodFlowsGraph.saturateAllParameters() does saturate the receiver in
+                     * many cases, so the check above would also lead to a null typeProfile, but we
+                     * cannot guarantee that we cover all cases.
+                     */
+                    JavaTypeProfile typeProfile = makeTypeProfile(receiverTypeState);
+                    /*
+                     * In a closed type world analysis the method profile of an invoke is complete
+                     * and contains all the callees reachable at that invocation location. Even if
+                     * that invoke is saturated it is still correct as it contains all the reachable
+                     * implementations of the target method. However, in an open type world the
+                     * method profile of an invoke, saturated or not, is incomplete, as there can be
+                     * implementations that we haven't yet seen.
+                     */
+                    JavaMethodProfile methodProfile = makeMethodProfile(callees);
 
-                JavaTypeProfile typeProfile = makeTypeProfile(receiverTypeState);
-                JavaMethodProfile methodProfile = makeMethodProfile(callees);
-                assert typeProfile == null || typeProfile.getTypes().length > 1 : "Should devirtualize with typeProfile=" + typeProfile + " and methodProfile=" + methodProfile;
-                assert methodProfile == null || methodProfile.getMethods().length > 1 : "Should devirtualize with typeProfile=" + typeProfile + " and methodProfile=" + methodProfile;
+                    assert typeProfile == null || typeProfile.getTypes().length > 1 : "Should devirtualize with typeProfile=" + typeProfile + " and methodProfile=" + methodProfile;
+                    assert methodProfile == null || methodProfile.getMethods().length > 1 : "Should devirtualize with typeProfile=" + typeProfile + " and methodProfile=" + methodProfile;
 
-                setInvokeProfiles(invoke, typeProfile, methodProfile);
+                    setInvokeProfiles(invoke, typeProfile, methodProfile);
+                }
             }
 
             if (allowOptimizeReturnParameter) {

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/SubstrateOptions.java

@@ -1264,10 +1264,23 @@ public static boolean includeAll() {
     public static final HostedOptionKey<Boolean> ForeignAPISupport = new HostedOptionKey<>(false);
 
     @Option(help = "Assume new types cannot be added after analysis", type = OptionType.Expert) //
-    public static final HostedOptionKey<Boolean> ClosedTypeWorld = new HostedOptionKey<>(true);
+    public static final HostedOptionKey<Boolean> ClosedTypeWorld = new HostedOptionKey<>(true) {
+        @Override
+        protected void onValueUpdate(EconomicMap<OptionKey<?>, Object> values, Boolean oldValue, Boolean newValue) {
+            ClosedTypeWorldHubLayout.update(values, newValue);
+        }
+    };
+
+    @Option(help = "Use the closed type world dynamic hub representation. This is only allowed when the option ClosedTypeWorld is also set to true.", type = OptionType.Expert) //
+    public static final HostedOptionKey<Boolean> ClosedTypeWorldHubLayout = new HostedOptionKey<>(true);
+
+    @Fold
+    public static boolean useClosedTypeWorldHubLayout() {
+        return ClosedTypeWorldHubLayout.getValue();
+    }
 
     @Fold
-    public static boolean closedTypeWorld() {
+    public static boolean useClosedTypeWorld() {
         return ClosedTypeWorld.getValue();
     }
 

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/meta/KnownOffsets.java

@@ -125,7 +125,7 @@ public int getVTableOffset(int vTableIndex, boolean fromDynamicHubStart) {
     }
 
     public int getTypeIDSlotsOffset() {
-        assert isFullyInitialized() && SubstrateOptions.closedTypeWorld();
+        assert isFullyInitialized() && SubstrateOptions.useClosedTypeWorldHubLayout();
         return typeIDSlotsOffset;
     }
 

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/meta/SubstrateBasicLoweringProvider.java

@@ -162,7 +162,7 @@ private void lowerLoadMethodNode(LoadMethodNode loadMethodNode, LoweringTool too
         SharedMethod method = (SharedMethod) loadMethodNode.getMethod();
         ValueNode hub = loadMethodNode.getHub();
 
-        if (SubstrateOptions.closedTypeWorld()) {
+        if (SubstrateOptions.useClosedTypeWorldHubLayout()) {
 
             int vtableEntryOffset = knownOffsets.getVTableOffset(method.getVTableIndex(), true);
             assert vtableEntryOffset > 0;

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/graal/snippets/NonSnippetLowerings.java

@@ -293,11 +293,13 @@ public static class InvokeLowering implements NodeLoweringProvider<FixedNode> {
         protected final RuntimeConfiguration runtimeConfig;
         protected final boolean verifyTypes;
         protected final KnownOffsets knownOffsets;
+        private final boolean isClosedTypeWorld;
 
         public InvokeLowering(RuntimeConfiguration runtimeConfig, boolean verifyTypes, KnownOffsets knownOffsets) {
             this.runtimeConfig = runtimeConfig;
             this.verifyTypes = verifyTypes;
             this.knownOffsets = knownOffsets;
+            isClosedTypeWorld = SubstrateOptions.useClosedTypeWorld();
         }
 
         @Override
@@ -366,14 +368,15 @@ public void lower(FixedNode node, LoweringTool tool) {
                 }
 
                 CallTargetNode loweredCallTarget;
-                if (invokeKind.isDirect() || implementations.length == 1) {
+                if (invokeKind.isDirect() || (implementations.length == 1 && (isClosedTypeWorld || method.canBeStaticallyBound()))) {
                     SharedMethod targetMethod = method;
                     if (!invokeKind.isDirect()) {
                         /*
                          * We only have one possible implementation for an indirect call, so we can
                          * emit a direct call to the unique implementation.
                          */
                         targetMethod = implementations[0];
+                        assert targetMethod != null : "Expecting a unique callee for target method " + method;
                     }
 
                     if (SubstrateUtil.HOSTED && targetMethod.forceIndirectCall()) {
@@ -421,7 +424,7 @@ public void lower(FixedNode node, LoweringTool tool) {
                                         address, parameters.toArray(new ValueNode[parameters.size()]), callTarget.returnStamp(), signature, targetMethod, callType, invokeKind));
                         graph.addBeforeFixed(node, codeStart);
                     }
-                } else if (implementations.length == 0) {
+                } else if (implementations.length == 0 && isClosedTypeWorld) {
                     /*
                      * We are calling an abstract method with no implementation, i.e., the
                      * closed-world analysis showed that there is no concrete receiver ever
@@ -437,7 +440,7 @@ public void lower(FixedNode node, LoweringTool tool) {
                     LoadHubNode hub = graph.unique(new LoadHubNode(runtimeConfig.getProviders().getStampProvider(), graph.addOrUnique(PiNode.create(receiver, nullCheck))));
                     nodesToLower.add(hub);
 
-                    if (SubstrateOptions.closedTypeWorld()) {
+                    if (SubstrateOptions.useClosedTypeWorldHubLayout()) {
                         int vtableEntryOffset = knownOffsets.getVTableOffset(method.getVTableIndex(), true);
 
                         AddressNode address = graph.unique(new OffsetAddressNode(hub, ConstantNode.forIntegerKind(ConfigurationValues.getWordKind(), vtableEntryOffset, graph)));

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/jni/access/JNIAccessibleMethod.java

@@ -128,7 +128,7 @@ CodePointer getCallWrapperAddress() {
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     CodePointer getJavaCallAddress(Object instance, boolean nonVirtual) {
         if (!nonVirtual) {
-            if (SubstrateOptions.closedTypeWorld()) {
+            if (SubstrateOptions.useClosedTypeWorldHubLayout()) {
                 assert vtableOffset != JNIAccessibleMethod.VTABLE_OFFSET_NOT_YET_COMPUTED;
                 if (vtableOffset != JNIAccessibleMethod.STATICALLY_BOUND_METHOD) {
                     return BarrieredAccess.readWord(instance.getClass(), vtableOffset, NamedLocationIdentity.FINAL_LOCATION);