Skip to content

Commit 902887d

Browse files
gergo-gergo-
committed
[JDK-8335915] Don't enumerate GuardPhis multiple times in AbstractMergeNode.phis()
PullRequest: graal/18260
2 parents c628812 + e14e18b commit 902887d

File tree

4 files changed

+106
-14
lines changed

4 files changed

+106
-14
lines changed

‎compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/nodes/test/LoopPhiCanonicalizerTest.java

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2013, 2019, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2013, 2024, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -24,19 +24,25 @@
2424
*/
2525
package jdk.graal.compiler.nodes.test;
2626

27+
import org.junit.Assert;
28+
import org.junit.BeforeClass;
29+
import org.junit.Test;
30+
2731
import jdk.graal.compiler.api.directives.GraalDirectives;
2832
import jdk.graal.compiler.core.test.GraalCompilerTest;
33+
import jdk.graal.compiler.graph.Node;
34+
import jdk.graal.compiler.graph.iterators.NodeIterable;
2935
import jdk.graal.compiler.graph.iterators.NodePredicate;
36+
import jdk.graal.compiler.nodes.GuardPhiNode;
3037
import jdk.graal.compiler.nodes.LoopBeginNode;
3138
import jdk.graal.compiler.nodes.PhiNode;
3239
import jdk.graal.compiler.nodes.StructuredGraph;
3340
import jdk.graal.compiler.nodes.StructuredGraph.AllowAssumptions;
41+
import jdk.graal.compiler.nodes.ValuePhiNode;
42+
import jdk.graal.compiler.nodes.debug.BlackholeNode;
3443
import jdk.graal.compiler.nodes.spi.CoreProviders;
3544
import jdk.graal.compiler.phases.common.CanonicalizerPhase;
3645
import jdk.graal.compiler.phases.util.GraphOrder;
37-
import org.junit.Assert;
38-
import org.junit.BeforeClass;
39-
import org.junit.Test;
4046

4147
public class LoopPhiCanonicalizerTest extends GraalCompilerTest {
4248

@@ -261,4 +267,52 @@ private void testSchedulable(String snippet, int loopPhisAfter) {
261267
GraphOrder.assertSchedulableGraph(g);
262268
Assert.assertEquals(loopPhisAfter, g.getNodes().filter(PhiNode.class).filter(x -> ((PhiNode) x).isLoopPhi()).count());
263269
}
270+
271+
public static int loopSnippet06() {
272+
int sum = 0;
273+
for (int x : array) {
274+
sum += x;
275+
}
276+
return sum;
277+
}
278+
279+
@Test
280+
public void test06() {
281+
StructuredGraph g = parseEager(getResolvedJavaMethod("loopSnippet06"), AllowAssumptions.NO);
282+
CanonicalizerPhase canonicalizer = CanonicalizerPhase.create();
283+
canonicalizer.apply(g, getDefaultHighTierContext());
284+
285+
/*
286+
* JDK-8335915 regression test: Guard phi that has a loop begin node both as its merge and
287+
* as one of its guard inputs.
288+
*/
289+
LoopBeginNode loopBegin = g.getNodes(LoopBeginNode.TYPE).first();
290+
GuardPhiNode guardPhi = g.addWithoutUnique(new GuardPhiNode(loopBegin, g.start(), loopBegin));
291+
BlackholeNode blackhole = g.add(new BlackholeNode(guardPhi));
292+
g.addAfterFixed(loopBegin, blackhole);
293+
294+
/* The guard phi is counted twice in the loop begin's usages, but only once in its phis. */
295+
assertValueAndGuardPhiCounts(loopBegin.usages(), 2, 2);
296+
assertValueAndGuardPhiCounts(loopBegin.phis(), 2, 1);
297+
298+
canonicalizer.apply(g, getDefaultHighTierContext());
299+
300+
assertValueAndGuardPhiCounts(loopBegin.usages(), 2, 2);
301+
assertValueAndGuardPhiCounts(loopBegin.phis(), 2, 1);
302+
}
303+
304+
private static void assertValueAndGuardPhiCounts(NodeIterable<? extends Node> nodes, int expectedValuePhis, int expectedGuardPhis) {
305+
int actualValuePhis = 0;
306+
int actualGuardPhis = 0;
307+
for (Node n : nodes) {
308+
if (n instanceof ValuePhiNode) {
309+
actualValuePhis++;
310+
}
311+
if (n instanceof GuardPhiNode) {
312+
actualGuardPhis++;
313+
}
314+
}
315+
Assert.assertEquals("value phis", expectedValuePhis, actualValuePhis);
316+
Assert.assertEquals("guard phis", expectedGuardPhis, actualGuardPhis);
317+
}
264318
}

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/graph/iterators/NodeIterator.java

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
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.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* 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> {
3737

3838
@Override
3939
public boolean hasNext() {
40+
if (current != null) {
41+
return true;
42+
}
4043
forward();
4144
return current != null;
4245
}
4346

4447
@Override
4548
public T next() {
46-
forward();
49+
/*
50+
* Normally hasNext() should call forward(). Only call it here if current is null,
51+
* indicating users calling next() without calling hasNext() first.
52+
*/
53+
if (current == null) {
54+
forward();
55+
}
4756
T ret = current;
4857
if (current == null) {
4958
throw new NoSuchElementException();

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/graph/iterators/PredicatedProxyNodeIterator.java

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
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.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -40,11 +40,13 @@ public PredicatedProxyNodeIterator(Iterator<T> iterator, NodePredicate predicate
4040

4141
@Override
4242
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+
}
4849
}
50+
current = null;
4951
}
5052
}

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/AbstractMergeNode.java

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2009, 2022, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2009, 2024, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -30,12 +30,15 @@
3030

3131
import java.util.List;
3232

33+
import org.graalvm.collections.EconomicSet;
34+
3335
import jdk.graal.compiler.debug.DebugCloseable;
3436
import jdk.graal.compiler.graph.IterableNodeType;
3537
import jdk.graal.compiler.graph.Node;
3638
import jdk.graal.compiler.graph.NodeClass;
3739
import jdk.graal.compiler.graph.NodeInputList;
3840
import jdk.graal.compiler.graph.iterators.NodeIterable;
41+
import jdk.graal.compiler.graph.iterators.NodePredicate;
3942
import jdk.graal.compiler.nodeinfo.NodeInfo;
4043
import jdk.graal.compiler.nodes.memory.MemoryPhiNode;
4144
import jdk.graal.compiler.nodes.spi.LIRLowerable;
@@ -136,8 +139,32 @@ public AbstractEndNode phiPredecessorAt(int index) {
136139
return forwardEndAt(index);
137140
}
138141

142+
/**
143+
* Returns the phi nodes {@linkplain #isPhiAtMerge anchored at this merge}. The returned node
144+
* iterable does not contain duplicates. The iteration order is not specified.
145+
*/
139146
public NodeIterable<PhiNode> phis() {
140-
return this.usages().filter(PhiNode.class).filter(this::isPhiAtMerge);
147+
return this.usages().filter(PhiNode.class).filter(new NodePredicate() {
148+
/*
149+
* A guard phi can use a merge both as its merge point and as a guard input. Such a phi
150+
* occurs in the usages twice. Eliminate duplicate guard phis using this set.
151+
*/
152+
EconomicSet<GuardPhiNode> seenGuardPhis = null;
153+
154+
@Override
155+
public boolean apply(Node n) {
156+
if (isPhiAtMerge(n)) {
157+
if (n instanceof GuardPhiNode guardPhi) {
158+
if (seenGuardPhis == null) {
159+
seenGuardPhis = EconomicSet.create();
160+
}
161+
return seenGuardPhis.add(guardPhi);
162+
}
163+
return true;
164+
}
165+
return false;
166+
}
167+
});
141168
}
142169

143170
public NodeIterable<ValuePhiNode> valuePhis() {

0 commit comments

Comments
 (0)