[JDK-8335915] Don't enumerate GuardPhis multiple times in AbstractMergeNode.phis()

gergo- · gergo- · commit 902887d38877 · 2024-07-25T08:28:20.000Z
PullRequest: graal/18260
diff --git a/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/nodes/test/LoopPhiCanonicalizerTest.java b/compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/nodes/test/LoopPhiCanonicalizerTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 2024, 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
@@ -24,19 +24,25 @@
  */
 package jdk.graal.compiler.nodes.test;
 
+import org.junit.Assert;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
 import jdk.graal.compiler.api.directives.GraalDirectives;
 import jdk.graal.compiler.core.test.GraalCompilerTest;
+import jdk.graal.compiler.graph.Node;
+import jdk.graal.compiler.graph.iterators.NodeIterable;
 import jdk.graal.compiler.graph.iterators.NodePredicate;
+import jdk.graal.compiler.nodes.GuardPhiNode;
 import jdk.graal.compiler.nodes.LoopBeginNode;
 import jdk.graal.compiler.nodes.PhiNode;
 import jdk.graal.compiler.nodes.StructuredGraph;
 import jdk.graal.compiler.nodes.StructuredGraph.AllowAssumptions;
+import jdk.graal.compiler.nodes.ValuePhiNode;
+import jdk.graal.compiler.nodes.debug.BlackholeNode;
 import jdk.graal.compiler.nodes.spi.CoreProviders;
 import jdk.graal.compiler.phases.common.CanonicalizerPhase;
 import jdk.graal.compiler.phases.util.GraphOrder;
-import org.junit.Assert;
-import org.junit.BeforeClass;
-import org.junit.Test;
 
 public class LoopPhiCanonicalizerTest extends GraalCompilerTest {
 
@@ -261,4 +267,52 @@ private void testSchedulable(String snippet, int loopPhisAfter) {
         GraphOrder.assertSchedulableGraph(g);
         Assert.assertEquals(loopPhisAfter, g.getNodes().filter(PhiNode.class).filter(x -> ((PhiNode) x).isLoopPhi()).count());
     }
+
+    public static int loopSnippet06() {
+        int sum = 0;
+        for (int x : array) {
+            sum += x;
+        }
+        return sum;
+    }
+
+    @Test
+    public void test06() {
+        StructuredGraph g = parseEager(getResolvedJavaMethod("loopSnippet06"), AllowAssumptions.NO);
+        CanonicalizerPhase canonicalizer = CanonicalizerPhase.create();
+        canonicalizer.apply(g, getDefaultHighTierContext());
+
+        /*
+         * JDK-8335915 regression test: Guard phi that has a loop begin node both as its merge and
+         * as one of its guard inputs.
+         */
+        LoopBeginNode loopBegin = g.getNodes(LoopBeginNode.TYPE).first();
+        GuardPhiNode guardPhi = g.addWithoutUnique(new GuardPhiNode(loopBegin, g.start(), loopBegin));
+        BlackholeNode blackhole = g.add(new BlackholeNode(guardPhi));
+        g.addAfterFixed(loopBegin, blackhole);
+
+        /* The guard phi is counted twice in the loop begin's usages, but only once in its phis. */
+        assertValueAndGuardPhiCounts(loopBegin.usages(), 2, 2);
+        assertValueAndGuardPhiCounts(loopBegin.phis(), 2, 1);
+
+        canonicalizer.apply(g, getDefaultHighTierContext());
+
+        assertValueAndGuardPhiCounts(loopBegin.usages(), 2, 2);
+        assertValueAndGuardPhiCounts(loopBegin.phis(), 2, 1);
+    }
+
+    private static void assertValueAndGuardPhiCounts(NodeIterable<? extends Node> nodes, int expectedValuePhis, int expectedGuardPhis) {
+        int actualValuePhis = 0;
+        int actualGuardPhis = 0;
+        for (Node n : nodes) {
+            if (n instanceof ValuePhiNode) {
+                actualValuePhis++;
+            }
+            if (n instanceof GuardPhiNode) {
+                actualGuardPhis++;
+            }
+        }
+        Assert.assertEquals("value phis", expectedValuePhis, actualValuePhis);
+        Assert.assertEquals("guard phis", expectedGuardPhis, actualGuardPhis);
+    }
 }
diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/graph/iterators/NodeIterator.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/graph/iterators/NodeIterator.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2024, 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
@@ -37,13 +37,22 @@ public abstract class NodeIterator<T extends Node> implements Iterator<T> {
 
     @Override
     public boolean hasNext() {
+        if (current != null) {
+            return true;
+        }
         forward();
         return current != null;
     }
 
     @Override
     public T next() {
-        forward();
+        /*
+         * Normally hasNext() should call forward(). Only call it here if current is null,
+         * indicating users calling next() without calling hasNext() first.
+         */
+        if (current == null) {
+            forward();
+        }
         T ret = current;
         if (current == null) {
             throw new NoSuchElementException();
diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/graph/iterators/PredicatedProxyNodeIterator.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/graph/iterators/PredicatedProxyNodeIterator.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2018, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2024, 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
@@ -40,11 +40,13 @@ public PredicatedProxyNodeIterator(Iterator<T> iterator, NodePredicate predicate
 
     @Override
     protected void forward() {
-        while ((current == null || !current.isAlive() || !predicate.apply(current)) && iterator.hasNext()) {
-            current = iterator.next();
-        }
-        if (current != null && (!current.isAlive() || !predicate.apply(current))) {
-            current = null;
+        while (iterator.hasNext()) {
+            T c = iterator.next();
+            if (c != null && c.isAlive() && predicate.apply(c)) {
+                current = c;
+                return;
+            }
         }
+        current = null;
     }
 }
diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/AbstractMergeNode.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/AbstractMergeNode.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2009, 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2009, 2024, 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
@@ -30,12 +30,15 @@
 
 import java.util.List;
 
+import org.graalvm.collections.EconomicSet;
+
 import jdk.graal.compiler.debug.DebugCloseable;
 import jdk.graal.compiler.graph.IterableNodeType;
 import jdk.graal.compiler.graph.Node;
 import jdk.graal.compiler.graph.NodeClass;
 import jdk.graal.compiler.graph.NodeInputList;
 import jdk.graal.compiler.graph.iterators.NodeIterable;
+import jdk.graal.compiler.graph.iterators.NodePredicate;
 import jdk.graal.compiler.nodeinfo.NodeInfo;
 import jdk.graal.compiler.nodes.memory.MemoryPhiNode;
 import jdk.graal.compiler.nodes.spi.LIRLowerable;
@@ -136,8 +139,32 @@ public AbstractEndNode phiPredecessorAt(int index) {
         return forwardEndAt(index);
     }
 
+    /**
+     * Returns the phi nodes {@linkplain #isPhiAtMerge anchored at this merge}. The returned node
+     * iterable does not contain duplicates. The iteration order is not specified.
+     */
     public NodeIterable<PhiNode> phis() {
-        return this.usages().filter(PhiNode.class).filter(this::isPhiAtMerge);
+        return this.usages().filter(PhiNode.class).filter(new NodePredicate() {
+            /*
+             * A guard phi can use a merge both as its merge point and as a guard input. Such a phi
+             * occurs in the usages twice. Eliminate duplicate guard phis using this set.
+             */
+            EconomicSet<GuardPhiNode> seenGuardPhis = null;
+
+            @Override
+            public boolean apply(Node n) {
+                if (isPhiAtMerge(n)) {
+                    if (n instanceof GuardPhiNode guardPhi) {
+                        if (seenGuardPhis == null) {
+                            seenGuardPhis = EconomicSet.create();
+                        }
+                        return seenGuardPhis.add(guardPhi);
+                    }
+                    return true;
+                }
+                return false;
+            }
+        });
     }
 
     public NodeIterable<ValuePhiNode> valuePhis() {

Original file line number	Diff line number	Diff line change
`@@ -1,5 +1,5 @@`
`1`	`1`	`/*`
`2`		`- * Copyright (c) 2011, 2018, Oracle and/or its affiliates. All rights reserved.`
	`2`	`+ * Copyright (c) 2011, 2024, Oracle and/or its affiliates. All rights reserved.`
`3`	`3`	`* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.`
`4`	`4`	`*`
`5`	`5`	`* This code is free software; you can redistribute it and/or modify it`
`@@ -40,11 +40,13 @@ public PredicatedProxyNodeIterator(Iterator<T> iterator, NodePredicate predicate`
`40`	`40`
`41`	`41`	`@Override`
`42`	`42`	`protected void forward() {`
`43`		`- while ((current == null \|\| !current.isAlive() \|\| !predicate.apply(current)) && iterator.hasNext()) {`
`44`		`- current = iterator.next();`
`45`		`- }`
`46`		`- if (current != null && (!current.isAlive() \|\| !predicate.apply(current))) {`
`47`		`- current = null;`
	`43`	`+ while (iterator.hasNext()) {`
	`44`	`+ T c = iterator.next();`
	`45`	`+ if (c != null && c.isAlive() && predicate.apply(c)) {`
	`46`	`+ current = c;`
	`47`	`+ return;`
	`48`	`+ }`
`48`	`49`	`}`
	`50`	`+ current = null;`
`49`	`51`	`}`
`50`	`52`	`}`