[GR-36569] Check stamp of MacroNodes

PullRequest: graal/10923

compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/CallSiteTargetNode.java

@@ -26,12 +26,11 @@
 
 import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.graph.NodeClass;
-import org.graalvm.compiler.nodes.spi.Canonicalizable;
-import org.graalvm.compiler.nodes.spi.CanonicalizerTool;
 import org.graalvm.compiler.nodeinfo.NodeInfo;
 import org.graalvm.compiler.nodes.ConstantNode;
-import org.graalvm.compiler.nodes.InvokeNode;
 import org.graalvm.compiler.nodes.ValueNode;
+import org.graalvm.compiler.nodes.spi.Canonicalizable;
+import org.graalvm.compiler.nodes.spi.CanonicalizerTool;
 import org.graalvm.compiler.nodes.spi.Lowerable;
 import org.graalvm.compiler.nodes.spi.LoweringTool;
 import org.graalvm.compiler.replacements.nodes.MacroNode;
@@ -82,9 +81,7 @@ public void lower(LoweringTool tool) {
         if (target != null) {
             graph().replaceFixedWithFloating(this, target);
         } else {
-            InvokeNode invoke = createInvoke();
-            graph().replaceFixedWithFixed(this, invoke);
-            invoke.lower(tool);
+            super.lower(tool);
         }
     }
 }

compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/ObjectCloneNode.java

@@ -31,23 +31,27 @@
 import org.graalvm.compiler.core.common.type.Stamp;
 import org.graalvm.compiler.core.common.type.StampPair;
 import org.graalvm.compiler.debug.DebugContext;
+import org.graalvm.compiler.debug.GraalError;
 import org.graalvm.compiler.graph.NodeClass;
 import org.graalvm.compiler.hotspot.meta.HotSpotLoweringProvider;
 import org.graalvm.compiler.nodeinfo.NodeInfo;
+import org.graalvm.compiler.nodes.InvokeNode;
 import org.graalvm.compiler.nodes.NodeView;
 import org.graalvm.compiler.nodes.ParameterNode;
 import org.graalvm.compiler.nodes.ReturnNode;
 import org.graalvm.compiler.nodes.StructuredGraph;
 import org.graalvm.compiler.nodes.StructuredGraph.AllowAssumptions;
 import org.graalvm.compiler.nodes.ValueNode;
 import org.graalvm.compiler.nodes.java.LoadFieldNode;
+import org.graalvm.compiler.nodes.spi.Lowerable;
 import org.graalvm.compiler.nodes.spi.LoweringTool;
 import org.graalvm.compiler.nodes.spi.Replacements;
 import org.graalvm.compiler.nodes.type.StampTool;
 import org.graalvm.compiler.nodes.virtual.AllocatedObjectNode;
 import org.graalvm.compiler.nodes.virtual.CommitAllocationNode;
 import org.graalvm.compiler.nodes.virtual.VirtualInstanceNode;
 import org.graalvm.compiler.nodes.virtual.VirtualObjectNode;
+import org.graalvm.compiler.phases.common.inlining.InliningUtil;
 import org.graalvm.compiler.replacements.SnippetTemplate.SnippetInfo;
 import org.graalvm.compiler.replacements.nodes.BasicObjectCloneNode;
 import org.graalvm.compiler.replacements.nodes.MacroInvokable;
@@ -75,12 +79,36 @@ public Stamp computeStamp(ValueNode object) {
          * If this call can't be intrinsified don't report a non-null stamp, otherwise the stamp
          * would change when this is lowered back to an invoke and we might lose a null check.
          */
-        return AbstractPointerStamp.pointerMaybeNull(object.stamp(NodeView.DEFAULT));
+        return AbstractPointerStamp.pointerMaybeNull(returnStamp.getTrustedStamp());
     }
 
     @Override
+    public void lower(LoweringTool tool) {
+        StructuredGraph replacementGraph = getLoweredSnippetGraph(tool);
+
+        if (replacementGraph != null) {
+            // Replace this node with an invoke but disable verification of the stamp since the
+            // invoke only exists for the purpose of performing the inling.
+            InvokeNode invoke = createInvoke(false);
+            graph().replaceFixedWithFixed(this, invoke);
+
+            // Pull out the receiver null check so that a replaced
+            // receiver can be lowered if necessary
+            if (!getTargetMethod().isStatic()) {
+                ValueNode nonNullReceiver = InliningUtil.nonNullReceiver(invoke);
+                if (nonNullReceiver instanceof Lowerable) {
+                    ((Lowerable) nonNullReceiver).lower(tool);
+                }
+            }
+            InliningUtil.inline(invoke, replacementGraph, false, getTargetMethod(), "Replace with graph.", "LoweringPhase");
+            replacementGraph.getDebug().dump(DebugContext.DETAILED_LEVEL, asNode().graph(), "After inlining replacement %s", replacementGraph);
+        } else {
+            super.lower(tool);
+        }
+    }
+
     @SuppressWarnings("try")
-    public StructuredGraph getLoweredSnippetGraph(LoweringTool tool) {
+    private StructuredGraph getLoweredSnippetGraph(LoweringTool tool) {
         ResolvedJavaType type = StampTool.typeOrNull(getObject());
 
         if (type != null) {
@@ -103,7 +131,7 @@ public StructuredGraph getLoweredSnippetGraph(LoweringTool tool) {
                     assert getConcreteType(stamp(NodeView.DEFAULT)) != null;
                     return MacroInvokable.lowerReplacement(graph(), (StructuredGraph) snippetGraph.copy(getDebug()), tool);
                 }
-                assert false : "unhandled array type " + type.getComponentType().getJavaKind();
+                GraalError.shouldNotReachHere("unhandled array type " + type.getComponentType().getJavaKind());
             } else {
                 Assumptions assumptions = graph().getAssumptions();
                 type = getConcreteType(getObject().stamp(NodeView.DEFAULT));

compiler/src/org.graalvm.compiler.replacements/src/org/graalvm/compiler/replacements/SnippetTemplate.java

@@ -1924,6 +1924,13 @@ private void replaceMemoryUsages(ValueNode node, MemoryMap map) {
         }
     }
 
+    public Node getReturnValue(UnmodifiableEconomicMap<Node, Node> duplicates) {
+        if (returnNode.result() != null) {
+            return duplicates.get(returnNode.result());
+        }
+        return null;
+    }
+
     /**
      * Replaces a given fixed node with this specialized snippet.
      *

compiler/src/org.graalvm.compiler.replacements/src/org/graalvm/compiler/replacements/nodes/FallbackInvokeWithExceptionNode.java

@@ -66,15 +66,6 @@ public LocationIdentity getKilledLocationIdentity() {
         return LocationIdentity.any();
     }
 
-    /**
-     * At the time when this node was created (i.e. when creating the snippet template), we did not
-     * have actual parameters so the precise stamp was not known. To fix that, we allow updating it
-     * after the snippet was instantiated.
-     */
-    public boolean updateInitialStamp(Stamp newStamp) {
-        return updateStamp(newStamp);
-    }
-
     /**
      * @see FallbackInvokeWithExceptionNode
      */

compiler/src/org.graalvm.compiler.replacements/src/org/graalvm/compiler/replacements/nodes/MacroInvokable.java

@@ -32,7 +32,6 @@
 import org.graalvm.compiler.nodes.CallTargetNode;
 import org.graalvm.compiler.nodes.Invokable;
 import org.graalvm.compiler.nodes.Invoke;
-import org.graalvm.compiler.nodes.InvokeNode;
 import org.graalvm.compiler.nodes.StateSplit;
 import org.graalvm.compiler.nodes.StructuredGraph;
 import org.graalvm.compiler.nodes.ValueNode;
@@ -44,20 +43,15 @@
 import org.graalvm.compiler.phases.common.GuardLoweringPhase;
 import org.graalvm.compiler.phases.common.LoweringPhase;
 import org.graalvm.compiler.phases.common.RemoveValueProxyPhase;
-import org.graalvm.compiler.phases.common.inlining.InliningUtil;
 
 import jdk.vm.ci.meta.ResolvedJavaMethod;
 
 /**
  * Macro invokable nodes can be used to temporarily replace an invoke. They can, for example, be
  * used to implement constant folding for known JDK functions like {@link Class#isInterface()}.<br/>
  * <br/>
- * During lowering, multiple sources are queried in order to look for a replacement:
- * <ul>
- * <li>If {@link #getLoweredSnippetGraph(LoweringTool)} returns a non-null result, this graph is
- * used as a replacement.</li>
- * <li>Otherwise, the macro node is replaced with an {@link InvokeNode}.</li>
- * </ul>
+ * During lowering subclasses may lower the node as appropriate. Otherwise, the macro node is
+ * replaced with an {@link Invoke}.
  */
 public interface MacroInvokable extends Invokable, Lowerable, StateSplit, SingleMemoryKill {
 
@@ -101,17 +95,6 @@ default void setBci(int bci) {
      */
     Invoke replaceWithInvoke();
 
-    /**
-     * Gets a snippet to be used for lowering this macro node. The returned graph (if non-null) must
-     * have been
-     * {@linkplain MacroInvokable#lowerReplacement(StructuredGraph, StructuredGraph, LoweringTool)
-     * lowered}.
-     */
-    @SuppressWarnings("unused")
-    default StructuredGraph getLoweredSnippetGraph(LoweringTool tool) {
-        return null;
-    }
-
     /**
      * Applies {@linkplain LoweringPhase lowering} to a replacement graph.
      *
@@ -140,31 +123,16 @@ static StructuredGraph lowerReplacement(StructuredGraph graph, StructuredGraph r
 
     @Override
     default void lower(LoweringTool tool) {
-        StructuredGraph replacementGraph = getLoweredSnippetGraph(tool);
-
         Invoke invoke = replaceWithInvoke();
         assert invoke.asNode().verify();
 
-        if (replacementGraph != null) {
-            // Pull out the receiver null check so that a replaced
-            // receiver can be lowered if necessary
-            if (!getTargetMethod().isStatic()) {
-                ValueNode nonNullReceiver = InliningUtil.nonNullReceiver(invoke);
-                if (nonNullReceiver instanceof Lowerable) {
-                    ((Lowerable) nonNullReceiver).lower(tool);
-                }
-            }
-            InliningUtil.inline(invoke, replacementGraph, false, getTargetMethod(), "Replace with graph.", "LoweringPhase");
-            replacementGraph.getDebug().dump(DebugContext.DETAILED_LEVEL, asNode().graph(), "After inlining replacement %s", replacementGraph);
-        } else {
-            if (isPlaceholderBci(invoke.bci())) {
-                throw new GraalError("%s: cannot lower to invoke with placeholder BCI: %s", asNode().graph(), this);
-            }
+        if (isPlaceholderBci(invoke.bci())) {
+            throw new GraalError("%s: cannot lower to invoke with placeholder BCI: %s", asNode().graph(), this);
+        }
 
-            if (invoke.stateAfter() == null) {
-                throw new GraalError("%s: cannot lower to invoke without state: %s", asNode().graph(), this);
-            }
-            invoke.lower(tool);
+        if (invoke.stateAfter() == null) {
+            throw new GraalError("%s: cannot lower to invoke without state: %s", asNode().graph(), this);
         }
+        invoke.lower(tool);
     }
 }

compiler/src/org.graalvm.compiler.replacements/src/org/graalvm/compiler/replacements/nodes/MacroNode.java

@@ -30,6 +30,7 @@
 
 import org.graalvm.compiler.core.common.type.StampPair;
 import org.graalvm.compiler.debug.DebugCloseable;
+import org.graalvm.compiler.debug.GraalError;
 import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.graph.NodeClass;
 import org.graalvm.compiler.graph.NodeInputList;
@@ -40,6 +41,7 @@
 import org.graalvm.compiler.nodes.FrameState;
 import org.graalvm.compiler.nodes.Invoke;
 import org.graalvm.compiler.nodes.InvokeNode;
+import org.graalvm.compiler.nodes.NodeView;
 import org.graalvm.compiler.nodes.ValueNode;
 import org.graalvm.compiler.nodes.graphbuilderconf.GraphBuilderContext;
 import org.graalvm.compiler.nodes.java.MethodCallTargetNode;
@@ -126,6 +128,17 @@ protected MacroNode(NodeClass<? extends MacroNode> c, MacroParams p) {
         assert MacroInvokable.assertArgumentCount(this);
     }
 
+    @Override
+    public boolean inferStamp() {
+        verifyStamp();
+        return false;
+    }
+
+    protected void verifyStamp() {
+        GraalError.guarantee(returnStamp.getTrustedStamp().equals(stamp(NodeView.DEFAULT)), "Stamp of replaced node %s must be the same as the original Invoke %s, but is %s ",
+                        this, returnStamp.getTrustedStamp(), stamp(NodeView.DEFAULT));
+    }
+
     @Override
     public ResolvedJavaMethod getContextMethod() {
         return callerMethod;
@@ -174,7 +187,7 @@ public final boolean hasSideEffect() {
 
     /**
      * Returns {@link LocationIdentity#any()}. This node needs to kill any location because it might
-     * get {@linkplain #replaceWithInvoke() replaced with an invoke} and
+     * get {@linkplain MacroInvokable#replaceWithInvoke() replaced with an invoke} and
      * {@link InvokeNode#getKilledLocationIdentity()} kills {@link LocationIdentity#any()} and the
      * kill location must not get broader.
      */
@@ -192,8 +205,9 @@ protected void afterClone(Node other) {
     @SuppressWarnings("try")
     public Invoke replaceWithInvoke() {
         try (DebugCloseable context = withNodeSourcePosition()) {
-            InvokeNode invoke = createInvoke();
+            InvokeNode invoke = createInvoke(true);
             graph().replaceFixedWithFixed(this, invoke);
+            assert invoke.verify();
             return invoke;
         }
     }
@@ -202,7 +216,12 @@ public LocationIdentity getLocationIdentity() {
         return LocationIdentity.any();
     }
 
-    protected InvokeNode createInvoke() {
+    /**
+     * Replace this node with the equivalent invoke. If we are falling back to the original invoke
+     * then the stamp of the current node isn't permitted to be different than the actual invoke
+     * because this would leave the graph in an inconsistent state.
+     */
+    protected InvokeNode createInvoke(boolean verifyStamp) {
         MethodCallTargetNode callTarget = graph().add(new MethodCallTargetNode(invokeKind, targetMethod, getArguments().toArray(new ValueNode[0]), returnStamp, null));
         InvokeNode invoke = graph().add(new InvokeNode(callTarget, bci, getLocationIdentity()));
         if (stateAfter() != null) {
@@ -211,6 +230,9 @@ protected InvokeNode createInvoke() {
                 invoke.stateAfter().replaceFirstInput(this, invoke);
             }
         }
+        if (verifyStamp) {
+            verifyStamp();
+        }
         return invoke;
     }
 }

compiler/src/org.graalvm.compiler.replacements/src/org/graalvm/compiler/replacements/nodes/MacroWithExceptionNode.java

@@ -30,6 +30,7 @@
 
 import org.graalvm.compiler.core.common.type.StampPair;
 import org.graalvm.compiler.debug.DebugCloseable;
+import org.graalvm.compiler.debug.GraalError;
 import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.graph.NodeClass;
 import org.graalvm.compiler.graph.NodeInputList;
@@ -39,6 +40,7 @@
 import org.graalvm.compiler.nodes.FrameState;
 import org.graalvm.compiler.nodes.Invoke;
 import org.graalvm.compiler.nodes.InvokeWithExceptionNode;
+import org.graalvm.compiler.nodes.NodeView;
 import org.graalvm.compiler.nodes.ValueNode;
 import org.graalvm.compiler.nodes.WithExceptionNode;
 import org.graalvm.compiler.nodes.java.MethodCallTargetNode;
@@ -86,6 +88,17 @@ protected MacroWithExceptionNode(NodeClass<? extends MacroWithExceptionNode> c,
         assert MacroInvokable.assertArgumentCount(this);
     }
 
+    @Override
+    public boolean inferStamp() {
+        verifyStamp();
+        return false;
+    }
+
+    protected void verifyStamp() {
+        GraalError.guarantee(returnStamp.getTrustedStamp().equals(stamp(NodeView.DEFAULT)), "Stamp of replaced node %s must be the same as the original Invoke %s, but is %s ",
+                        this, returnStamp.getTrustedStamp(), stamp(NodeView.DEFAULT));
+    }
+
     @Override
     public ResolvedJavaMethod getContextMethod() {
         return callerMethod;
@@ -120,16 +133,13 @@ protected void afterClone(Node other) {
     @SuppressWarnings("try")
     public Invoke replaceWithInvoke() {
         try (DebugCloseable context = withNodeSourcePosition()) {
-            InvokeWithExceptionNode invoke = createInvoke();
+            InvokeWithExceptionNode invoke = createInvoke(this);
             graph().replaceWithExceptionSplit(this, invoke);
+            assert invoke.verify();
             return invoke;
         }
     }
 
-    protected InvokeWithExceptionNode createInvoke() {
-        return createInvoke(this);
-    }
-
     /**
      * Creates an invoke for the {@link #getTargetMethod()} associated with this node. The exception
      * edge of the result is not set by this function. This node is not modified.
@@ -147,6 +157,7 @@ public InvokeWithExceptionNode createInvoke(Node oldResult) {
                 invoke.stateAfter().replaceFirstInput(oldResult, invoke);
             }
         }
+        verifyStamp();
         return invoke;
     }
 

compiler/src/org.graalvm.compiler.replacements/src/org/graalvm/compiler/replacements/nodes/ReflectionGetCallerClassNode.java

@@ -27,12 +27,11 @@
 import org.graalvm.compiler.debug.GraalError;
 import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.graph.NodeClass;
-import org.graalvm.compiler.nodes.spi.Canonicalizable;
-import org.graalvm.compiler.nodes.spi.CanonicalizerTool;
 import org.graalvm.compiler.nodeinfo.NodeInfo;
 import org.graalvm.compiler.nodes.ConstantNode;
 import org.graalvm.compiler.nodes.FrameState;
-import org.graalvm.compiler.nodes.InvokeNode;
+import org.graalvm.compiler.nodes.spi.Canonicalizable;
+import org.graalvm.compiler.nodes.spi.CanonicalizerTool;
 import org.graalvm.compiler.nodes.spi.Lowerable;
 import org.graalvm.compiler.nodes.spi.LoweringTool;
 
@@ -66,9 +65,7 @@ public void lower(LoweringTool tool) {
         if (callerClassNode != null) {
             graph().replaceFixedWithFloating(this, graph().addOrUniqueWithInputs(callerClassNode));
         } else {
-            InvokeNode invoke = createInvoke();
-            graph().replaceFixedWithFixed(this, invoke);
-            invoke.lower(tool);
+            super.lower(tool);
         }
     }