[GR-35928] Effects closure: properly mark exception edges known to be dead.

PullRequest: graal/10654

Author: davleopo

compiler/src/org.graalvm.compiler.core.test/src/org/graalvm/compiler/core/test/ea/PartialEscapeWithExceptionTest.java

@@ -0,0 +1,51 @@
+/*
+ * Copyright (c) 2021, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.  Oracle designates this
+ * particular file as subject to the "Classpath" exception as provided
+ * by Oracle in the LICENSE file that accompanied this code.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+package org.graalvm.compiler.core.test.ea;
+
+import org.graalvm.compiler.api.directives.GraalDirectives;
+import org.graalvm.compiler.core.common.GraalOptions;
+import org.graalvm.compiler.core.test.GraalCompilerTest;
+import org.graalvm.compiler.options.OptionValues;
+import org.junit.Test;
+
+public class PartialEscapeWithExceptionTest extends GraalCompilerTest {
+
+    public static int snippet(int length) {
+        int[] a = new int[8];
+        int[] b = new int[8];
+        for (int i = 0; i < length; i++) {
+            System.arraycopy(a, 0, b, 0, a.length);
+            GraalDirectives.blackhole(a);
+        }
+        return 0;
+    }
+
+    @Test
+    public void test01() {
+        OptionValues opt = new OptionValues(getInitialOptions(), GraalOptions.LoopPeeling, false);
+        test(opt, "snippet", 10);
+    }
+
+}

compiler/src/org.graalvm.compiler.phases/src/org/graalvm/compiler/phases/graph/ReentrantBlockIterator.java

@@ -42,6 +42,7 @@
 import org.graalvm.compiler.nodes.LoopBeginNode;
 import org.graalvm.compiler.nodes.StructuredGraph;
 import org.graalvm.compiler.nodes.cfg.Block;
+import org.graalvm.compiler.nodes.cfg.ControlFlowGraph;
 
 public final class ReentrantBlockIterator {
 
@@ -56,16 +57,42 @@ public LoopInfo(int endCount, int exitCount) {
         }
     }
 
+    /**
+     * Abstract base class for reverse post order iteration over the {@link ControlFlowGraph}.
+     */
     public abstract static class BlockIteratorClosure<StateT> {
 
+        /**
+         * Create the initial state for the reverse post order iteration over the
+         * {@link ControlFlowGraph}.
+         */
         protected abstract StateT getInitialState();
 
+        /**
+         * Process the current block with the current state during reverse post order iteration.
+         */
         protected abstract StateT processBlock(Block block, StateT currentState);
 
+        /**
+         * Merge multiple states when processing {@link Block} starting with a
+         * {@link AbstractMergeNode}.
+         */
         protected abstract StateT merge(Block merge, List<StateT> states);
 
+        /**
+         * Clone a state for a successor invocation of
+         * {@link BlockIteratorClosure#processBlock(Block, Object)}.
+         */
         protected abstract StateT cloneState(StateT oldState);
 
+        /**
+         * Hook for subclasses to apply additional operations after
+         * {@link BlockIteratorClosure#cloneState(Object)} for successor blocks.
+         */
+        protected StateT afterSplit(@SuppressWarnings("unused") Block successor, StateT oldState) {
+            return oldState;
+        }
+
         protected abstract List<StateT> processLoop(Loop<Block> loop, StateT initialState);
     }
 
@@ -186,7 +213,7 @@ private static <StateT> Block processMultipleSuccessors(BlockIteratorClosure<Sta
         for (int i = 1; i < successors.length; i++) {
             Block successor = successors[i];
             blockQueue.addFirst(successor);
-            states.put(successor.getBeginNode(), closure.cloneState(state));
+            states.put(successor.getBeginNode(), closure.afterSplit(successor, closure.cloneState(state)));
         }
         return successors[0];
     }

compiler/src/org.graalvm.compiler.virtual/src/org/graalvm/compiler/virtual/phases/ea/EffectsBlockState.java

@@ -28,7 +28,10 @@
 import java.util.Map;
 
 import org.graalvm.collections.EconomicMap;
+import org.graalvm.collections.EconomicSet;
 import org.graalvm.collections.UnmodifiableMapCursor;
+import org.graalvm.compiler.nodes.WithExceptionNode;
+import org.graalvm.compiler.nodes.cfg.Block;
 
 public abstract class EffectsBlockState<T extends EffectsBlockState<T>> {
 
@@ -38,12 +41,22 @@ public abstract class EffectsBlockState<T extends EffectsBlockState<T>> {
      */
     private boolean dead;
 
+    /**
+     * Exception edges marked dead for this state by dominating {@link WithExceptionNode} control
+     * flow split nodes.
+     */
+    protected EconomicSet<Block> exceptionEdgesToKill;
+
     public EffectsBlockState() {
         // emtpy
     }
 
     public EffectsBlockState(EffectsBlockState<T> other) {
         this.dead = other.dead;
+        EconomicSet<Block> otherExceptionEdgesToKill = other.exceptionEdgesToKill;
+        if (otherExceptionEdgesToKill != null) {
+            this.exceptionEdgesToKill = EconomicSet.create(otherExceptionEdgesToKill);
+        }
     }
 
     @Override

compiler/src/org.graalvm.compiler.virtual/src/org/graalvm/compiler/virtual/phases/ea/EffectsClosure.java

@@ -55,6 +55,7 @@
 import org.graalvm.compiler.nodes.StructuredGraph.ScheduleResult;
 import org.graalvm.compiler.nodes.ValueNode;
 import org.graalvm.compiler.nodes.ValuePhiNode;
+import org.graalvm.compiler.nodes.WithExceptionNode;
 import org.graalvm.compiler.nodes.cfg.Block;
 import org.graalvm.compiler.nodes.cfg.ControlFlowGraph;
 import org.graalvm.compiler.nodes.extended.BoxNode;
@@ -191,6 +192,7 @@ protected List<Void> processLoop(Loop<Block> loop, Void initialState) {
                 apply(loopMergeEffects.get(loop));
                 return info.exitStates;
             }
+
         };
         ReentrantBlockIterator.apply(closure, cfg.getStartBlock());
         for (GraphEffectList effects : effectList) {
@@ -259,7 +261,19 @@ protected BlockT processBlock(Block block, BlockT state) {
                     }
                     processLoopExit(loopExit, loopEntryStates.get(loopExit.loopBegin()), state, blockEffects.get(block));
                 }
-                changed |= processNode(node, state, effects, lastFixedNode) && isSignificantNode(node);
+                Block exceptionEdgeToKill = node instanceof WithExceptionNode ? cfg.blockFor(((WithExceptionNode) node).exceptionEdge()) : null;
+                boolean lastNodeChanged = processNode(node, state, effects, lastFixedNode) && isSignificantNode(node);
+                changed |= lastNodeChanged;
+                if (lastNodeChanged && exceptionEdgeToKill != null) {
+                    /*
+                     * We deleted a exception node, per definition the exception edge died in that
+                     * process, no need to process the exception edge
+                     */
+                    if (state.exceptionEdgesToKill == null) {
+                        state.exceptionEdgesToKill = EconomicSet.create();
+                    }
+                    state.exceptionEdgesToKill.add(exceptionEdgeToKill);
+                }
                 if (node instanceof FixedWithNextNode) {
                     lastFixedNode = (FixedWithNextNode) node;
                 }
@@ -272,6 +286,15 @@ protected BlockT processBlock(Block block, BlockT state) {
         return state;
     }
 
+    @Override
+    protected BlockT afterSplit(Block successor, BlockT oldState) {
+        BlockT state = oldState;
+        if (oldState.exceptionEdgesToKill != null && oldState.exceptionEdgesToKill.contains(successor)) {
+            state.markAsDead();
+        }
+        return state;
+    }
+
     /**
      * Changes to {@link CommitAllocationNode}s, {@link AllocatedObjectNode}s and {@link BoxNode}s
      * are not considered to be "important". If only changes to those nodes are discovered during