[GR-51492] Array length test: add test penetrating non inlined case.

PullRequest: graal/16690
oracle · Jan 25, 2024 · 3eb587c · 3eb587c
2 parents 44a1d4b + 2f1f657
commit 3eb587c
Show file tree

Hide file tree

Showing 3 changed files with 78 additions and 19 deletions.
diff --git a/...src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/ArrayLengthProviderTest.java b/...src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/ArrayLengthProviderTest.java
@@ -30,6 +30,12 @@
 
 import org.junit.Test;
 
+import jdk.graal.compiler.core.common.GraalOptions;
+import jdk.graal.compiler.java.BytecodeParserOptions;
+import jdk.graal.compiler.nodes.spi.ProfileProvider;
+import jdk.graal.compiler.options.OptionValues;
+import jdk.vm.ci.meta.ResolvedJavaMethod;
+
 public class ArrayLengthProviderTest extends GraalCompilerTest {
 
     public static Object test0Snippet(ArrayList<?> list, boolean a) {
@@ -56,7 +62,7 @@ public static Object test0Snippet(ArrayList<?> list, boolean a) {
         }
     }
 
-    public static Object test1Snippet(ArrayList<?> list, boolean a, boolean b) {
+    public static Object test1Snippet(List<?> list, boolean a, boolean b) {
         while (true) {
             Object[] array = toArray(list);
             if (a || b) {
@@ -72,6 +78,16 @@ public static Object[] toArray(List<?> list) {
         return new Object[list.size()];
     }
 
+    @Override
+    protected ProfileProvider getProfileProvider(ResolvedJavaMethod method) {
+        if (profile) {
+            return null;
+        }
+        return NO_PROFILE_PROVIDER;
+    }
+
+    boolean profile = true;
+
     @Test
     public void test0() {
         test("test0Snippet", new ArrayList<>(Arrays.asList("a", "b")), true);
@@ -80,5 +96,15 @@ public void test0() {
     @Test
     public void test1() {
         test("test1Snippet", new ArrayList<>(Arrays.asList("a", "b")), true, true);
+
+    }
+
+    @Test
+    public void test2() {
+        resetCache();
+        profile = false;
+        OptionValues o = new OptionValues(getInitialOptions(), GraalOptions.LoopUnswitch, false, BytecodeParserOptions.InlineDuringParsing, false);
+        test(o, "test1Snippet", new ArrayList<>(Arrays.asList("a", "b")), true, true);
+        profile = true;
     }
 }
diff --git a/...ompiler/src/jdk/graal/compiler/phases/common/DominatorBasedGlobalValueNumberingPhase.java b/...ompiler/src/jdk/graal/compiler/phases/common/DominatorBasedGlobalValueNumberingPhase.java
@@ -28,6 +28,7 @@
 import java.util.Arrays;
 import java.util.Optional;
 
+import jdk.graal.compiler.phases.common.util.LoopUtility;
 import jdk.graal.compiler.phases.util.GraphOrder;
 import org.graalvm.collections.EconomicMap;
 import jdk.graal.compiler.core.common.GraalOptions;
@@ -612,24 +613,9 @@ public void substitute(Node n, ControlFlowGraph cfg, NodeBitMap licmNodes, LoopE
                     }
                 }
 
-                HIRBlock useBlock = cfg.blockFor(n);
-                Loop<HIRBlock> defLoop = defBlock.getLoop();
-                Loop<HIRBlock> useLoop = useBlock.getLoop();
-
-                if (defLoop != null) {
-                    // the def is inside a loop, either a parent or a disjunct loop
-                    if (useLoop != null) {
-                        // we are only safe without proxies if we are included in the def loop,
-                        // i.e., the def loop is a parent loop
-                        if (!useLoop.isAncestorOrSelf(defLoop)) {
-                            earlyGVNAbort.increment(graph.getDebug());
-                            return;
-                        }
-                    } else {
-                        // the use is not in a loop but the def is, needs proxies, fail
-                        earlyGVNAbort.increment(graph.getDebug());
-                        return;
-                    }
+                if (!LoopUtility.canUseWithoutProxy(cfg, edgeDataEqual, n)) {
+                    earlyGVNAbort.increment(graph.getDebug());
+                    return;
                 }
 
                 graph.getDebug().log(DebugContext.VERY_DETAILED_LEVEL, "Early GVN: replacing %s with %s", n, edgeDataEqual);

diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/util/LoopUtility.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/util/LoopUtility.java
@@ -26,10 +26,14 @@
 
 import java.util.EnumSet;
 
+import jdk.graal.compiler.core.common.cfg.Loop;
 import jdk.graal.compiler.core.common.type.IntegerStamp;
 import jdk.graal.compiler.core.common.type.Stamp;
 import jdk.graal.compiler.graph.Graph.NodeEvent;
 import jdk.graal.compiler.graph.Graph.NodeEventScope;
+import jdk.graal.compiler.graph.Node;
+import jdk.graal.compiler.nodes.FixedNode;
+import jdk.graal.compiler.nodes.GraphState.StageFlag;
 import jdk.graal.compiler.nodes.LoopExitNode;
 import jdk.graal.compiler.nodes.NodeView;
 import jdk.graal.compiler.nodes.ProxyNode;
@@ -39,6 +43,8 @@
 import jdk.graal.compiler.nodes.calc.AddNode;
 import jdk.graal.compiler.nodes.calc.IntegerConvertNode;
 import jdk.graal.compiler.nodes.calc.MulNode;
+import jdk.graal.compiler.nodes.cfg.ControlFlowGraph;
+import jdk.graal.compiler.nodes.cfg.HIRBlock;
 import jdk.graal.compiler.nodes.loop.BasicInductionVariable;
 import jdk.graal.compiler.nodes.loop.InductionVariable;
 import jdk.graal.compiler.nodes.loop.LoopEx;
@@ -48,6 +54,47 @@
 
 public class LoopUtility {
 
+    /**
+     * Determine if the def can use node {@code use} without the need for value proxies. This means
+     * there is no loop exit between the schedule point of def and use that would require a
+     * {@link ProxyNode}.
+     */
+    public static boolean canUseWithoutProxy(ControlFlowGraph cfg, Node def, Node use) {
+        if (def.graph() instanceof StructuredGraph g && g.isAfterStage(StageFlag.VALUE_PROXY_REMOVAL)) {
+            return true;
+        }
+        if (!isFixedNode(def) || !isFixedNode(use)) {
+            /*
+             * If def or use are not fixed nodes we cannot determine the schedule point for them.
+             * Without the schedule point we cannot find their basic block in the control flow
+             * graph. If we would schedule the graph we could answer the question for floating nodes
+             * as well but this is too much overhead. Thus, for floating nodes we give up and assume
+             * a proxy is necessary.
+             */
+            return false;
+        }
+        HIRBlock useBlock = cfg.blockFor(use);
+        HIRBlock defBlock = cfg.blockFor(def);
+        Loop<HIRBlock> defLoop = defBlock.getLoop();
+        Loop<HIRBlock> useLoop = useBlock.getLoop();
+        if (defLoop != null) {
+            // the def is inside a loop, either a parent or a disjunct loop
+            if (useLoop != null) {
+                // we are only safe without proxies if we are included in the def loop,
+                // i.e., the def loop is a parent loop
+                return useLoop.isAncestorOrSelf(defLoop);
+            } else {
+                // the use is not in a loop but the def is, needs proxies, fail
+                return false;
+            }
+        }
+        return true;
+    }
+
+    private static boolean isFixedNode(Node n) {
+        return n instanceof FixedNode;
+    }
+
     public static boolean isNumericInteger(ValueNode v) {
         Stamp s = v.stamp(NodeView.DEFAULT);
         return s instanceof IntegerStamp;