Skip to content

Commit c23ffb3

Browse files
committed
[GR-51364] Don't add node with dead input to the graph
PullRequest: graal/16633
2 parents 487b4d9 + 21c82ca commit c23ffb3

File tree

4 files changed

+38
-32
lines changed

4 files changed

+38
-32
lines changed

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/graph/test/NodeValidationChecksTest.java

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2015, 2018, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2015, 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,6 +30,7 @@
3030
import org.junit.Assert;
3131
import org.junit.Test;
3232

33+
import jdk.graal.compiler.debug.GraalError;
3334
import jdk.graal.compiler.graph.Graph;
3435
import jdk.graal.compiler.graph.Node;
3536
import jdk.graal.compiler.graph.NodeClass;
@@ -58,8 +59,8 @@ public void testInputNotAlive() {
5859
try {
5960
graph.add(new TestNode(node, null));
6061
Assert.fail("Exception expected.");
61-
} catch (AssertionError e) {
62-
Assert.assertTrue(e.getMessage().contains("Input"));
62+
} catch (GraalError e) {
63+
Assert.assertTrue(e.getMessage().contains("input"));
6364
Assert.assertTrue(e.getMessage().contains("not alive"));
6465
}
6566
}
@@ -71,8 +72,8 @@ public void testSuccessorNotAlive() {
7172
try {
7273
graph.add(new TestNode(null, node));
7374
Assert.fail("Exception expected.");
74-
} catch (AssertionError e) {
75-
Assert.assertTrue(e.getMessage().contains("Successor"));
75+
} catch (GraalError e) {
76+
Assert.assertTrue(e.getMessage().contains("successor"));
7677
Assert.assertTrue(e.getMessage().contains("not alive"));
7778
}
7879
}

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/graph/NodeClass.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2011, 2023, 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
@@ -1340,7 +1340,7 @@ public void registerAtSuccessorsAsPredecessor(Node node) {
13401340
if ((myMask & LIST_MASK) == 0) {
13411341
Node curNode = Edges.getNodeUnsafe(node, offset);
13421342
if (curNode != null) {
1343-
assert curNode.isAlive() : "Successor not alive";
1343+
GraalError.guarantee(curNode.isAlive(), "Adding %s to the graph but its successor %s is not alive", node, curNode);
13441344
node.updatePredecessor(null, curNode);
13451345
}
13461346
} else {
@@ -1356,7 +1356,7 @@ private static void registerAtSuccessorsAsPredecessorHelper(Node node, long offs
13561356
for (int i = 0; i < list.size(); ++i) {
13571357
Node curNode = list.get(i);
13581358
if (curNode != null) {
1359-
assert curNode.isAlive() : "Successor not alive";
1359+
GraalError.guarantee(curNode.isAlive(), "Adding %s to the graph but its successor %s is not alive", node, curNode);
13601360
node.updatePredecessor(null, curNode);
13611361
}
13621362
}
@@ -1419,7 +1419,7 @@ void registerAtInputsAsUsage(Node node) {
14191419
if ((myMask & LIST_MASK) == 0) {
14201420
Node curNode = Edges.getNodeUnsafe(node, offset);
14211421
if (curNode != null) {
1422-
assert curNode.isAlive() : "Input " + curNode + " of node " + node + " is not alive";
1422+
GraalError.guarantee(curNode.isAlive(), "Adding %s to the graph but its input %s is not alive", node, curNode);
14231423
curNode.addUsage(node);
14241424
}
14251425
} else {
@@ -1435,7 +1435,7 @@ private static void registerAtInputsAsUsageHelper(Node node, long offset) {
14351435
for (int i = 0; i < list.size(); ++i) {
14361436
Node curNode = list.get(i);
14371437
if (curNode != null) {
1438-
assert curNode.isAlive() : "Input not alive " + curNode;
1438+
GraalError.guarantee(curNode.isAlive(), "Adding %s to the graph but its input %s is not alive", node, curNode);
14391439
curNode.addUsage(node);
14401440
}
14411441
}

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/java/ArrayLengthNode.java

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2009, 2021, 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
@@ -31,6 +31,7 @@
3131

3232
import jdk.graal.compiler.core.common.type.AbstractObjectStamp;
3333
import jdk.graal.compiler.core.common.type.StampFactory;
34+
import jdk.graal.compiler.graph.Graph;
3435
import jdk.graal.compiler.graph.Node.NodeIntrinsicFactory;
3536
import jdk.graal.compiler.graph.NodeClass;
3637
import jdk.graal.compiler.nodeinfo.NodeInfo;
@@ -170,19 +171,32 @@ public void simplify(SimplifierTool tool) {
170171
* </pre>
171172
*/
172173
StructuredGraph graph = graph();
173-
ValueNode replacement = length;
174-
if (!length.isConstant() && length.stamp(NodeView.DEFAULT).canBeImprovedWith(StampFactory.positiveInt())) {
175-
ValueAnchorNode guard = graph.add(new ValueAnchorNode());
176-
graph.addAfterFixed(this, guard);
177-
replacement = graph.addWithoutUnique(new PiNode(length, StampFactory.positiveInt(), guard));
178-
}
179-
if (!replacement.isAlive()) {
180-
replacement = graph.addOrUnique(replacement);
181-
}
174+
ValueNode replacement = maybeAddPositivePi(length, this);
182175
graph.replaceFixedWithFloating(this, replacement);
183176
}
184177
}
185178

179+
/**
180+
* If necessary, improves the {@code length}'s stamp to a positive value by adding a
181+
* {@link PiNode} for it. The pi will be attached to a new {@link ValueAnchorNode} after the
182+
* {@code insertionPosition}.
183+
*
184+
* @return the {@code length} or its {@linkplain Graph#addOrUnique unique representative} if the
185+
* length's stamp is already positive; otherwise, a new {@link PiNode} proving a
186+
* positive stamp for the length
187+
*/
188+
public static ValueNode maybeAddPositivePi(ValueNode length, FixedWithNextNode insertionPosition) {
189+
StructuredGraph graph = insertionPosition.graph();
190+
length = graph.addOrUnique(length);
191+
ValueNode replacement = length;
192+
if (!length.isConstant() && length.stamp(NodeView.DEFAULT).canBeImprovedWith(StampFactory.positiveInt())) {
193+
ValueAnchorNode g = graph.add(new ValueAnchorNode());
194+
graph.addAfterFixed(insertionPosition, g);
195+
replacement = graph.addWithoutUnique(new PiNode(length, StampFactory.positiveInt(), g));
196+
}
197+
return replacement;
198+
}
199+
186200
/**
187201
* Gets the length of an array if possible.
188202
*

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/memory/ReadNode.java

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2011, 2023, 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
@@ -51,13 +51,12 @@
5151
import jdk.graal.compiler.nodes.FixedWithNextNode;
5252
import jdk.graal.compiler.nodes.FrameState;
5353
import jdk.graal.compiler.nodes.NodeView;
54-
import jdk.graal.compiler.nodes.PiNode;
5554
import jdk.graal.compiler.nodes.StructuredGraph;
5655
import jdk.graal.compiler.nodes.ValueNode;
5756
import jdk.graal.compiler.nodes.calc.NarrowNode;
5857
import jdk.graal.compiler.nodes.calc.ZeroExtendNode;
5958
import jdk.graal.compiler.nodes.extended.GuardingNode;
60-
import jdk.graal.compiler.nodes.extended.ValueAnchorNode;
59+
import jdk.graal.compiler.nodes.java.ArrayLengthNode;
6160
import jdk.graal.compiler.nodes.memory.address.AddressNode;
6261
import jdk.graal.compiler.nodes.memory.address.OffsetAddressNode;
6362
import jdk.graal.compiler.nodes.spi.ArrayLengthProvider;
@@ -166,15 +165,7 @@ public void simplify(SimplifierTool tool) {
166165
ValueNode length = GraphUtil.arrayLength(objAddress.getBase(), ArrayLengthProvider.FindLengthMode.CANONICALIZE_READ, constantReflection);
167166
if (length != null) {
168167
StructuredGraph graph = graph();
169-
ValueNode replacement = length;
170-
if (!length.isConstant() && length.stamp(NodeView.DEFAULT).canBeImprovedWith(StampFactory.positiveInt())) {
171-
ValueAnchorNode g = graph.add(new ValueAnchorNode());
172-
graph.addAfterFixed(this, g);
173-
replacement = graph.addWithoutUnique(new PiNode(length, StampFactory.positiveInt(), g));
174-
}
175-
if (!replacement.isAlive()) {
176-
replacement = graph.addOrUnique(replacement);
177-
}
168+
ValueNode replacement = ArrayLengthNode.maybeAddPositivePi(length, this);
178169
graph.replaceFixedWithFloating(this, replacement);
179170
}
180171
}

0 commit comments

Comments
 (0)