[GR-33886] Remove KillingBeginNode

PullRequest: graal/10143

Author: tkrodriguez

compiler/src/org.graalvm.compiler.graph/src/org/graalvm/compiler/graph/Edges.java

@@ -223,7 +223,7 @@ public void initializeList(Node node, int index, NodeList<Node> value) {
 
     private void verifyUpdateValid(Node node, int index, Object newValue) {
         if (newValue != null && !getType(index).isAssignableFrom(newValue.getClass())) {
-            throw new IllegalArgumentException("Can not assign " + newValue.getClass() + " to " + getType(index) + " in " + node);
+            throw new IllegalArgumentException("Can not assign " + newValue + " to " + getType(index) + " in " + node);
         }
     }
 

compiler/src/org.graalvm.compiler.java/src/org/graalvm/compiler/java/BytecodeParser.java

@@ -3019,7 +3019,7 @@ private AbstractBeginNode updateWithExceptionNode(WithExceptionNode withExceptio
             withExceptionNode.setExceptionEdge(exceptionEdge);
         }
         assert withExceptionNode.next() == null : "new WithExceptionNode with existing next";
-        AbstractBeginNode nextBegin = graph.add(withExceptionNode.createNextBegin());
+        AbstractBeginNode nextBegin = graph.add(new BeginNode());
         withExceptionNode.setNext(nextBegin);
         return nextBegin;
     }

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/BeginNode.java

@@ -31,9 +31,9 @@
 import org.graalvm.compiler.core.common.type.StampFactory;
 import org.graalvm.compiler.debug.DebugCloseable;
 import org.graalvm.compiler.graph.NodeClass;
+import org.graalvm.compiler.nodeinfo.NodeInfo;
 import org.graalvm.compiler.nodes.spi.Simplifiable;
 import org.graalvm.compiler.nodes.spi.SimplifierTool;
-import org.graalvm.compiler.nodeinfo.NodeInfo;
 
 @NodeInfo(cycles = CYCLES_0, size = SIZE_0)
 public final class BeginNode extends AbstractBeginNode implements Simplifiable {
@@ -48,17 +48,6 @@ public BeginNode(Stamp stamp) {
         super(TYPE, stamp);
     }
 
-    public void trySimplify() {
-        FixedNode prev = (FixedNode) this.predecessor();
-        if (prev instanceof ControlSplitNode) {
-            // This begin node is necessary.
-        } else {
-            // This begin node can be removed and all guards moved up to the preceding begin node.
-            prepareDelete();
-            graph().removeFixed(this);
-        }
-    }
-
     @Override
     public void simplify(SimplifierTool tool) {
         FixedNode prev = (FixedNode) this.predecessor();

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/GraphDecoder.java

@@ -400,24 +400,21 @@ protected static class InvokeData {
         public final int stateAfterOrderId;
         public final int nextOrderId;
 
-        public final int nextNextOrderId;
         public final int exceptionOrderId;
         public final int exceptionStateOrderId;
         public final int exceptionNextOrderId;
         public JavaConstant constantReceiver;
         public CallTargetNode callTarget;
         public FixedWithNextNode invokePredecessor;
 
-        protected InvokeData(Invoke invoke, ResolvedJavaType contextType, int invokeOrderId, int callTargetOrderId, int stateAfterOrderId, int nextOrderId, int nextNextOrderId,
-                        int exceptionOrderId,
-                        int exceptionStateOrderId, int exceptionNextOrderId) {
+        protected InvokeData(Invoke invoke, ResolvedJavaType contextType, int invokeOrderId, int callTargetOrderId, int stateAfterOrderId, int nextOrderId,
+                        int exceptionOrderId, int exceptionStateOrderId, int exceptionNextOrderId) {
             this.invoke = invoke;
             this.contextType = contextType;
             this.invokeOrderId = invokeOrderId;
             this.callTargetOrderId = callTargetOrderId;
             this.stateAfterOrderId = stateAfterOrderId;
             this.nextOrderId = nextOrderId;
-            this.nextNextOrderId = nextNextOrderId;
             this.exceptionOrderId = exceptionOrderId;
             this.exceptionStateOrderId = exceptionStateOrderId;
             this.exceptionNextOrderId = exceptionNextOrderId;
@@ -813,14 +810,13 @@ protected InvokeData readInvokeData(MethodScope methodScope, int invokeOrderId,
         int nextOrderId = readOrderId(methodScope);
 
         if (invoke instanceof InvokeWithExceptionNode) {
-            int nextNextOrderId = readOrderId(methodScope);
             int exceptionOrderId = readOrderId(methodScope);
             int exceptionStateOrderId = readOrderId(methodScope);
             int exceptionNextOrderId = readOrderId(methodScope);
-            return new InvokeData(invoke, contextType, invokeOrderId, callTargetOrderId, stateAfterOrderId, nextOrderId, nextNextOrderId, exceptionOrderId, exceptionStateOrderId,
+            return new InvokeData(invoke, contextType, invokeOrderId, callTargetOrderId, stateAfterOrderId, nextOrderId, exceptionOrderId, exceptionStateOrderId,
                             exceptionNextOrderId);
         } else {
-            return new InvokeData(invoke, contextType, invokeOrderId, callTargetOrderId, stateAfterOrderId, nextOrderId, -1, -1, -1, -1);
+            return new InvokeData(invoke, contextType, invokeOrderId, callTargetOrderId, stateAfterOrderId, nextOrderId, -1, -1, -1);
         }
     }
 
@@ -1539,6 +1535,22 @@ protected void makeSuccessorStubs(MethodScope methodScope, LoopScope loopScope,
         }
     }
 
+    protected NodeClass<?> getNodeClass(MethodScope methodScope, LoopScope loopScope, int nodeOrderId) {
+        if (nodeOrderId == GraphEncoder.NULL_ORDER_ID) {
+            return null;
+        }
+        FixedNode node = (FixedNode) lookupNode(loopScope, nodeOrderId);
+        if (node != null) {
+            return node.getNodeClass();
+        }
+
+        long readerByteIndex = methodScope.reader.getByteIndex();
+        methodScope.reader.setByteIndex(methodScope.encodedGraph.nodeStartOffsets[nodeOrderId]);
+        NodeClass<?> nodeClass = methodScope.encodedGraph.getNodeClasses()[methodScope.reader.getUVInt()];
+        methodScope.reader.setByteIndex(readerByteIndex);
+        return nodeClass;
+    }
+
     protected FixedNode makeStubNode(MethodScope methodScope, LoopScope loopScope, int nodeOrderId) {
         if (nodeOrderId == GraphEncoder.NULL_ORDER_ID) {
             return null;

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/GraphEncoder.java

@@ -294,7 +294,6 @@ public int encode(StructuredGraph graph) {
                     InvokeWithExceptionNode invokeWithExcpetion = (InvokeWithExceptionNode) invoke;
                     ExceptionObjectNode exceptionEdge = (ExceptionObjectNode) invokeWithExcpetion.exceptionEdge();
 
-                    writeOrderId(invokeWithExcpetion.next().next(), nodeOrder);
                     writeOrderId(invokeWithExcpetion.exceptionEdge(), nodeOrder);
                     writeOrderId(exceptionEdge.stateAfter(), nodeOrder);
                     writeOrderId(exceptionEdge.next(), nodeOrder);

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/InvokeWithExceptionNode.java

@@ -35,7 +35,6 @@
 import java.util.Map;
 
 import org.graalvm.compiler.core.common.type.Stamp;
-import org.graalvm.compiler.debug.DebugCloseable;
 import org.graalvm.compiler.graph.IterableNodeType;
 import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.graph.NodeClass;
@@ -137,7 +136,7 @@ public int bci() {
     @Override
     public void setNext(FixedNode x) {
         if (x != null) {
-            this.setNext(KillingBeginNode.begin(x, this.getKilledLocationIdentity()));
+            this.setNext(BeginNode.begin(x));
         } else {
             this.setNext(null);
         }
@@ -178,24 +177,6 @@ public Map<Object, Object> getDebugProperties(Map<Object, Object> map) {
         return debugProperties;
     }
 
-    public AbstractBeginNode killKillingBegin() {
-        return killKillingBegin(next());
-    }
-
-    @SuppressWarnings("try")
-    public AbstractBeginNode killKillingBegin(AbstractBeginNode begin) {
-        if (begin instanceof KillingBeginNode) {
-            try (DebugCloseable position = begin.withNodeSourcePosition()) {
-                AbstractBeginNode newBegin = new BeginNode();
-                graph().addAfterFixed(begin, graph().add(newBegin));
-                begin.replaceAtUsages(newBegin);
-                graph().removeFixed(begin);
-                return newBegin;
-            }
-        }
-        return begin;
-    }
-
     @Override
     public void setBci(int newBci) {
         assert BytecodeFrame.isPlaceholderBci(bci) && !BytecodeFrame.isPlaceholderBci(newBci) : "can only replace placeholder with better bci";
@@ -259,19 +240,7 @@ public InvokeNode replaceWithNonThrowing() {
     @Override
     public void simplify(SimplifierTool tool) {
         if (exceptionEdge() instanceof UnreachableBeginNode) {
-            AbstractBeginNode storedNext = next();
-            InvokeNode replacement = replaceWithInvoke();
-            if (graph().isAfterStage(StructuredGraph.StageFlag.FLOATING_READS)) {
-                if (!tool.allUsagesAvailable()) {
-                    // we don't know about the usages - do nothing
-                    return;
-                }
-                storedNext.replaceAtUsages(replacement, Memory);
-            }
-            // kill the killing begin
-            AbstractBeginNode newBegin = killKillingBegin(storedNext);
-            tool.addToWorkList(newBegin.next());
-            tool.addToWorkList(newBegin);
+            replaceWithInvoke();
         }
     }
 }

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/KillingBeginNode.java

@@ -32,10 +32,10 @@
 import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.graph.NodeClass;
 import org.graalvm.compiler.graph.iterators.NodeIterable;
-import org.graalvm.compiler.nodes.spi.Simplifiable;
-import org.graalvm.compiler.nodes.spi.SimplifierTool;
 import org.graalvm.compiler.nodeinfo.NodeInfo;
 import org.graalvm.compiler.nodes.StructuredGraph.FrameStateVerificationFeature;
+import org.graalvm.compiler.nodes.spi.Simplifiable;
+import org.graalvm.compiler.nodes.spi.SimplifierTool;
 import org.graalvm.compiler.nodes.util.GraphUtil;
 
 @NodeInfo(allowedUsageTypes = {Association}, cycles = CYCLES_0, size = SIZE_4)
@@ -118,9 +118,14 @@ public void simplify(SimplifierTool tool) {
         Node prev = this.predecessor();
         while (tool.allUsagesAvailable() && prev instanceof BeginNode && prev.hasNoUsages()) {
             AbstractBeginNode begin = (AbstractBeginNode) prev;
-            this.setNodeSourcePosition(begin.getNodeSourcePosition());
-            prev = prev.predecessor();
-            graph().removeFixed(begin);
+            // Keep a single BeginNode in between LoopExitNodes and InvokeWithExceptionNodes
+            if (!(prev.predecessor() instanceof InvokeWithExceptionNode)) {
+                this.setNodeSourcePosition(begin.getNodeSourcePosition());
+                graph().removeFixed(begin);
+                prev = prev.predecessor();
+            } else {
+                break;
+            }
         }
     }
 
@@ -132,6 +137,11 @@ public boolean verify() {
          * state assignment, thus we only verify them until their removal
          */
         assert !this.graph().getFrameStateVerification().implies(FrameStateVerificationFeature.LOOP_EXITS) || this.stateAfter != null : "Loop exit must have a state until FSA " + this;
+
+        // Because the scheduler doesn't schedule ProxyNodes, the inputs to the ProxyNode can end up
+        // in the wrong place in the earliest local schedule. Ensuring there's a BeginNode before
+        // the LoopExitNode creates an earlier location where those nodes can be scheduled.
+        assert !(predecessor() instanceof InvokeWithExceptionNode);
         return super.verify();
     }
 }