[GR-51760] Remove InvocationPlugin.Receiver.get().

PullRequest: graal/16972
oracle · Feb 27, 2024 · 34929a0 · 34929a0
2 parents 092d075 + 95c9ee1
commit 34929a0
Show file tree

Hide file tree

Showing 21 changed files with 249 additions and 316 deletions.
diff --git a/...k.graal.compiler.test/src/jdk/graal/compiler/replacements/test/MethodHandleMacroTest.java b/...k.graal.compiler.test/src/jdk/graal/compiler/replacements/test/MethodHandleMacroTest.java
@@ -28,6 +28,8 @@
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.MethodType;
 
+import org.junit.Test;
+
 import jdk.graal.compiler.core.test.GraalCompilerTest;
 import jdk.graal.compiler.graph.NodeClass;
 import jdk.graal.compiler.nodeinfo.NodeInfo;
@@ -39,8 +41,6 @@
 import jdk.graal.compiler.nodes.graphbuilderconf.InvocationPlugins;
 import jdk.graal.compiler.replacements.InlineDuringParsingPlugin;
 import jdk.graal.compiler.replacements.nodes.MacroNode;
-import org.junit.Test;
-
 import jdk.vm.ci.meta.JavaKind;
 import jdk.vm.ci.meta.ResolvedJavaMethod;
 
@@ -86,7 +86,7 @@ public boolean inlineOnly() {
 
             @Override
             public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, InvocationPlugin.Receiver receiver) {
-                b.addPush(JavaKind.Int, new TestMacroNode(MacroNode.MacroParams.of(b, CallTargetNode.InvokeKind.Special, targetMethod, receiver.get())));
+                b.addPush(JavaKind.Int, new TestMacroNode(MacroNode.MacroParams.of(b, CallTargetNode.InvokeKind.Special, targetMethod, receiver.get(true))));
                 return true;
             }
         });

diff --git a/...rc/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java b/...rc/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/meta/HotSpotGraphBuilderPlugins.java
diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/java/BytecodeParser.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/java/BytecodeParser.java
@@ -2431,7 +2431,7 @@ private boolean tryFastInlineAccessor(ValueNode[] args, ResolvedJavaMethod targe
                 int cpi = Bytes.beU2(bytecode, 2);
                 JavaField field = targetMethod.getConstantPool().lookupField(cpi, targetMethod, GETFIELD);
                 if (field instanceof ResolvedJavaField) {
-                    ValueNode receiver = invocationPluginReceiver.init(targetMethod, args).get();
+                    ValueNode receiver = invocationPluginReceiver.init(targetMethod, args).get(true);
                     ResolvedJavaField resolvedField = (ResolvedJavaField) field;
                     try (DebugCloseable context = openNodeContext(targetMethod, 1)) {
                         genGetField(resolvedField, receiver);

diff --git a/...rc/jdk.graal.compiler/src/jdk/graal/compiler/nodes/graphbuilderconf/InvocationPlugin.java b/...rc/jdk.graal.compiler/src/jdk/graal/compiler/nodes/graphbuilderconf/InvocationPlugin.java
@@ -37,7 +37,6 @@
 import jdk.graal.compiler.nodes.Invoke;
 import jdk.graal.compiler.nodes.ValueNode;
 import jdk.graal.compiler.nodes.graphbuilderconf.InvocationPlugins.ClassPlugins;
-import jdk.graal.compiler.nodes.type.StampTool;
 import jdk.vm.ci.meta.MetaUtil;
 import jdk.vm.ci.meta.ResolvedJavaMethod;
 
@@ -53,33 +52,16 @@ public abstract class InvocationPlugin implements GraphBuilderPlugin {
      */
     public interface Receiver {
         /**
-         * Gets the receiver value, null checking it first if necessary.
+         * Returns the receiver value, optionally null checking it first if necessary.
          *
-         * @return the receiver value with a {@linkplain StampTool#isPointerNonNull(ValueNode)
-         *         non-null} stamp
-         */
-        default ValueNode get() {
-            return get(true);
-        }
-
-        /**
-         * Gets the receiver value, optionally null checking it first if necessary.
+         * Note that passing true for {@code performNullCheck} is only allowed if the
+         * {@link InvocationPlugin} unconditionally intrinsifies the invocation in that code path,
+         * i.e., returns true. Otherwise, a premature explicit null check remains in the graph.
+         *
+         * On the other hand, passing true for {@code performNullCheck} is required in any path
+         * where a {@link InvocationPlugin} returns true, otherwise the null check would be missing.
          */
         ValueNode get(boolean performNullCheck);
-
-        /**
-         * Gets the receiver value, asserting that its stamp is non-null. This does not emit any
-         * code but discharges the requirement that an invocation plugin must ensure the receiver of
-         * a non-static method is non-null.
-         */
-        ValueNode requireNonNull();
-
-        /**
-         * Determines if the receiver is constant.
-         */
-        default boolean isConstant() {
-            return false;
-        }
     }
 
     /**

diff --git a/...c/jdk.graal.compiler/src/jdk/graal/compiler/nodes/graphbuilderconf/InvocationPlugins.java b/...c/jdk.graal.compiler/src/jdk/graal/compiler/nodes/graphbuilderconf/InvocationPlugins.java
@@ -101,11 +101,6 @@ public static class Options {
     public static class InvocationPluginReceiver implements InvocationPlugin.Receiver {
         private final GraphBuilderContext parser;
         private ValueNode[] args;
-        /**
-         * Caches the null checked receiver value. If still {@code null} after application of a
-         * plugin, then the plugin never called {@link #get(boolean)} with {@code true}.
-         */
-        private ValueNode value;
 
         public InvocationPluginReceiver(GraphBuilderContext parser) {
             this.parser = parser;
@@ -114,25 +109,13 @@ public InvocationPluginReceiver(GraphBuilderContext parser) {
         @Override
         public ValueNode get(boolean performNullCheck) {
             assert args != null : "Cannot get the receiver of a static method";
-            if (!performNullCheck) {
-                return args[0];
-            }
-            if (value == null) {
-                value = parser.nullCheckedValue(args[0]);
-                if (value != args[0]) {
-                    args[0] = value;
-                }
+            if (performNullCheck) {
+                args[0] = parser.nullCheckedValue(args[0]);
             }
-            return value;
-        }
-
-        @Override
-        public boolean isConstant() {
-            return args[0].isConstant();
+            return args[0];
         }
 
         public InvocationPluginReceiver init(ResolvedJavaMethod targetMethod, ValueNode[] newArgs) {
-            this.value = null;
             if (!targetMethod.isStatic()) {
                 this.args = newArgs;
                 return this;
@@ -141,23 +124,11 @@ public InvocationPluginReceiver init(ResolvedJavaMethod targetMethod, ValueNode[
             return null;
         }
 
-        @Override
-        public ValueNode requireNonNull() {
-            if (value == null) {
-                GraalError.guarantee(args != null, "target method is static");
-                if (!StampTool.isPointerNonNull(args[0])) {
-                    throw new GraalError("receiver might be null: %s", value);
-                }
-                value = args[0];
-            }
-            return value;
-        }
-
         /**
          * Determines if {@link #get(boolean)} was called with {@code true}.
          */
         public boolean nullCheckPerformed() {
-            return value != null;
+            return StampTool.isPointerNonNull(args[0]);
         }
     }
 

diff --git a/...ler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/IntrinsicGraphBuilder.java b/...ler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/IntrinsicGraphBuilder.java
@@ -348,15 +348,6 @@ public ValueNode get(boolean performNullCheck) {
         return arguments[0];
     }
 
-    /**
-     * The non-null check assertion for the receiver is ignored for the reasons stated in
-     * {@link #get(boolean)}.
-     */
-    @Override
-    public ValueNode requireNonNull() {
-        return get(false);
-    }
-
     @SuppressWarnings("try")
     public final StructuredGraph buildGraph(InvocationPlugin plugin) {
         // The caller is expected to have filtered out decorator plugins since they cannot be

diff --git a/...aal.compiler/src/jdk/graal/compiler/replacements/SnippetSubstitutionInvocationPlugin.java b/...aal.compiler/src/jdk/graal/compiler/replacements/SnippetSubstitutionInvocationPlugin.java
@@ -34,7 +34,6 @@
 import jdk.graal.compiler.nodes.graphbuilderconf.GraphBuilderContext;
 import jdk.graal.compiler.nodes.graphbuilderconf.InvocationPlugin;
 import jdk.graal.compiler.replacements.nodes.MacroNode;
-
 import jdk.vm.ci.meta.JavaKind;
 import jdk.vm.ci.meta.ResolvedJavaMethod;
 
@@ -59,7 +58,7 @@ public boolean execute(GraphBuilderContext b, ResolvedJavaMethod targetMethod, I
         }
         if (receiver != null) {
             // Perform the required null check
-            ValueNode r = receiver.get();
+            ValueNode r = receiver.get(true);
             assert args[0] == r : args;
         }