Skip to content

Commit a6781fa

Browse files
author
Christian Wimmer
committed
[GR-9881] Correctly re-introduce loop exit proxy nodes when simplifying ArrayLengthNode.
PullRequest: graal/1508
2 parents 1b109ea + 6808d3e commit a6781fa

File tree

15 files changed

+198
-81
lines changed

15 files changed

+198
-81
lines changed
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
/*
2+
* Copyright (c) 2015, 2015, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation.
8+
*
9+
* This code is distributed in the hope that it will be useful, but WITHOUT
10+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
11+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
12+
* version 2 for more details (a copy is included in the LICENSE file that
13+
* accompanied this code).
14+
*
15+
* You should have received a copy of the GNU General Public License version
16+
* 2 along with this work; if not, write to the Free Software Foundation,
17+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
18+
*
19+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
20+
* or visit www.oracle.com if you need additional information or have any
21+
* questions.
22+
*/
23+
package org.graalvm.compiler.core.test;
24+
25+
import java.util.ArrayList;
26+
import java.util.Arrays;
27+
import java.util.List;
28+
29+
import org.junit.Test;
30+
31+
public class ArrayLengthProviderTest extends GraalCompilerTest {
32+
33+
public static Object test0Snippet(ArrayList<?> list, boolean a) {
34+
while (true) {
35+
Object[] array = toArray(list);
36+
if (array.length < 1) {
37+
return null;
38+
}
39+
if (array[0] instanceof String || a) {
40+
/*
41+
* This code is outside of the loop. Accessing the array reqires a ValueProxyNode.
42+
* When the simplification of the ArrayLengthNode replaces the length access with
43+
* the ArrayList.size used to create the array, then the value needs to have a
44+
* ValueProxyNode too. In addition, the two parts of the if-condition actually lead
45+
* to two separate loop exits, with two separate proxy nodes. A ValuePhiNode is
46+
* present originally for the array, and the array length simplification needs to
47+
* create a new ValuePhiNode for the two newly introduced ValueProxyNode.
48+
*/
49+
if (array.length < 1) {
50+
return null;
51+
}
52+
return array[0];
53+
}
54+
}
55+
}
56+
57+
public static Object test1Snippet(ArrayList<?> list, boolean a, boolean b) {
58+
while (true) {
59+
Object[] array = toArray(list);
60+
if (a || b) {
61+
if (array.length < 1) {
62+
return null;
63+
}
64+
return array[0];
65+
}
66+
}
67+
}
68+
69+
public static Object[] toArray(List<?> list) {
70+
return new Object[list.size()];
71+
}
72+
73+
@Test
74+
public void test0() {
75+
test("test0Snippet", new ArrayList<>(Arrays.asList("a", "b")), true);
76+
}
77+
78+
@Test
79+
public void test1() {
80+
test("test1Snippet", new ArrayList<>(Arrays.asList("a", "b")), true, true);
81+
}
82+
}

compiler/src/org.graalvm.compiler.hotspot/src/org/graalvm/compiler/hotspot/replacements/ObjectCloneNode.java

+1-3
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,8 @@
4040
import org.graalvm.compiler.nodes.java.LoadFieldNode;
4141
import org.graalvm.compiler.nodes.java.NewInstanceNode;
4242
import org.graalvm.compiler.nodes.java.StoreFieldNode;
43-
import org.graalvm.compiler.nodes.spi.ArrayLengthProvider;
4443
import org.graalvm.compiler.nodes.spi.LoweringTool;
4544
import org.graalvm.compiler.nodes.spi.Replacements;
46-
import org.graalvm.compiler.nodes.spi.VirtualizableAllocation;
4745
import org.graalvm.compiler.nodes.type.StampTool;
4846
import org.graalvm.compiler.replacements.nodes.BasicObjectCloneNode;
4947

@@ -53,7 +51,7 @@
5351
import jdk.vm.ci.meta.ResolvedJavaType;
5452

5553
@NodeInfo
56-
public final class ObjectCloneNode extends BasicObjectCloneNode implements VirtualizableAllocation, ArrayLengthProvider {
54+
public final class ObjectCloneNode extends BasicObjectCloneNode {
5755

5856
public static final NodeClass<ObjectCloneNode> TYPE = NodeClass.create(ObjectCloneNode.class);
5957

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/PiArrayNode.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public final class PiArrayNode extends PiNode implements ArrayLengthProvider {
4646
@Input ValueNode length;
4747

4848
@Override
49-
public ValueNode length() {
49+
public ValueNode findLength(ArrayLengthProvider.FindLengthMode mode) {
5050
return length;
5151
}
5252

@@ -57,7 +57,7 @@ public PiArrayNode(ValueNode object, ValueNode length, Stamp stamp) {
5757

5858
@Override
5959
public Node canonical(CanonicalizerTool tool) {
60-
if (GraphUtil.arrayLength(object()) != length()) {
60+
if (GraphUtil.arrayLength(object(), ArrayLengthProvider.FindLengthMode.SEARCH_ONLY) != length) {
6161
return this;
6262
}
6363
return super.canonical(tool);

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/ValuePhiNode.java

+1-23
Original file line numberDiff line numberDiff line change
@@ -29,16 +29,14 @@
2929
import org.graalvm.compiler.graph.NodeClass;
3030
import org.graalvm.compiler.graph.NodeInputList;
3131
import org.graalvm.compiler.nodeinfo.NodeInfo;
32-
import org.graalvm.compiler.nodes.spi.ArrayLengthProvider;
3332
import org.graalvm.compiler.nodes.type.StampTool;
34-
import org.graalvm.compiler.nodes.util.GraphUtil;
3533
import org.graalvm.util.CollectionsUtil;
3634

3735
/**
3836
* Value {@link PhiNode}s merge data flow values at control flow merges.
3937
*/
4038
@NodeInfo(nameTemplate = "Phi({i#values}, {p#valueDescription})")
41-
public class ValuePhiNode extends PhiNode implements ArrayLengthProvider {
39+
public class ValuePhiNode extends PhiNode {
4240

4341
public static final NodeClass<ValuePhiNode> TYPE = NodeClass.create(ValuePhiNode.class);
4442
@Input protected NodeInputList<ValueNode> values;
@@ -79,26 +77,6 @@ public boolean inferStamp() {
7977
return updateStamp(valuesStamp);
8078
}
8179

82-
@Override
83-
public ValueNode length() {
84-
if (merge() instanceof LoopBeginNode) {
85-
return null;
86-
}
87-
ValueNode length = null;
88-
for (ValueNode input : values()) {
89-
ValueNode l = GraphUtil.arrayLength(input);
90-
if (l == null) {
91-
return null;
92-
}
93-
if (length == null) {
94-
length = l;
95-
} else if (length != l) {
96-
return null;
97-
}
98-
}
99-
return length;
100-
}
101-
10280
@Override
10381
public boolean verify() {
10482
Stamp s = null;

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/java/AbstractNewArrayNode.java

+5-1
Original file line numberDiff line numberDiff line change
@@ -38,11 +38,15 @@ public abstract class AbstractNewArrayNode extends AbstractNewObjectNode impleme
3838
public static final NodeClass<AbstractNewArrayNode> TYPE = NodeClass.create(AbstractNewArrayNode.class);
3939
@Input protected ValueNode length;
4040

41-
@Override
4241
public ValueNode length() {
4342
return length;
4443
}
4544

45+
@Override
46+
public ValueNode findLength(ArrayLengthProvider.FindLengthMode mode) {
47+
return length;
48+
}
49+
4650
protected AbstractNewArrayNode(NodeClass<? extends AbstractNewArrayNode> c, Stamp stamp, ValueNode length, boolean fillContents, FrameState stateBefore) {
4751
super(c, stamp, fillContents, stateBefore);
4852
this.length = length;

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/java/ArrayLengthNode.java

+3-28
Original file line numberDiff line numberDiff line change
@@ -33,10 +33,9 @@
3333
import org.graalvm.compiler.nodes.ConstantNode;
3434
import org.graalvm.compiler.nodes.FixedWithNextNode;
3535
import org.graalvm.compiler.nodes.ValueNode;
36-
import org.graalvm.compiler.nodes.ValueProxyNode;
36+
import org.graalvm.compiler.nodes.spi.ArrayLengthProvider;
3737
import org.graalvm.compiler.nodes.spi.Lowerable;
3838
import org.graalvm.compiler.nodes.spi.LoweringTool;
39-
import org.graalvm.compiler.nodes.spi.ValueProxy;
4039
import org.graalvm.compiler.nodes.spi.Virtualizable;
4140
import org.graalvm.compiler.nodes.spi.VirtualizerTool;
4241
import org.graalvm.compiler.nodes.util.GraphUtil;
@@ -90,39 +89,15 @@ public ValueNode canonical(CanonicalizerTool tool, ValueNode forValue) {
9089
return this;
9190
}
9291

93-
/**
94-
* Replicate the {@link ValueProxyNode}s from {@code originalValue} onto {@code value}.
95-
*
96-
* @param originalValue a possibly proxied value
97-
* @param value a value needing proxies
98-
* @return proxies wrapping {@code value}
99-
*/
100-
private static ValueNode reproxyValue(ValueNode originalValue, ValueNode value) {
101-
if (value.isConstant()) {
102-
// No proxy needed
103-
return value;
104-
}
105-
if (originalValue instanceof ValueProxyNode) {
106-
ValueProxyNode proxy = (ValueProxyNode) originalValue;
107-
return new ValueProxyNode(reproxyValue(proxy.getOriginalNode(), value), proxy.proxyPoint());
108-
} else if (originalValue instanceof ValueProxy) {
109-
ValueProxy proxy = (ValueProxy) originalValue;
110-
return reproxyValue(proxy.getOriginalNode(), value);
111-
} else {
112-
return value;
113-
}
114-
}
115-
11692
/**
11793
* Gets the length of an array if possible.
11894
*
11995
* @return a node representing the length of {@code array} or null if it is not available
12096
*/
12197
public static ValueNode readArrayLength(ValueNode originalArray, ConstantReflectionProvider constantReflection) {
122-
ValueNode length = GraphUtil.arrayLength(originalArray);
98+
ValueNode length = GraphUtil.arrayLength(originalArray, ArrayLengthProvider.FindLengthMode.CANONICALIZE_READ);
12399
if (length != null) {
124-
// Ensure that any proxies on the original value end up on the length value
125-
return reproxyValue(originalArray, length);
100+
return length;
126101
}
127102
return readArrayLengthConstant(originalArray, constantReflection);
128103
}

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/java/NewMultiArrayNode.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ public boolean canDeoptimize() {
8787
}
8888

8989
@Override
90-
public ValueNode length() {
90+
public ValueNode findLength(ArrayLengthProvider.FindLengthMode mode) {
9191
return dimension(0);
9292
}
9393
}

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/memory/ReadNode.java

+2-1
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
import org.graalvm.compiler.nodes.extended.GuardingNode;
4545
import org.graalvm.compiler.nodes.memory.address.AddressNode;
4646
import org.graalvm.compiler.nodes.memory.address.OffsetAddressNode;
47+
import org.graalvm.compiler.nodes.spi.ArrayLengthProvider;
4748
import org.graalvm.compiler.nodes.spi.NodeLIRBuilderTool;
4849
import org.graalvm.compiler.nodes.spi.Virtualizable;
4950
import org.graalvm.compiler.nodes.spi.VirtualizerTool;
@@ -122,7 +123,7 @@ public static ValueNode canonicalizeRead(ValueNode read, AddressNode address, Lo
122123
}
123124
}
124125
if (locationIdentity.equals(ARRAY_LENGTH_LOCATION)) {
125-
ValueNode length = GraphUtil.arrayLength(object);
126+
ValueNode length = GraphUtil.arrayLength(object, ArrayLengthProvider.FindLengthMode.CANONICALIZE_READ);
126127
if (length != null) {
127128
return length;
128129
}

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/spi/ArrayLengthProvider.java

+36-2
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,45 @@
2323
package org.graalvm.compiler.nodes.spi;
2424

2525
import org.graalvm.compiler.nodes.ValueNode;
26+
import org.graalvm.compiler.nodes.ValuePhiNode;
27+
import org.graalvm.compiler.nodes.ValueProxyNode;
28+
import org.graalvm.compiler.nodes.util.GraphUtil;
2629

2730
public interface ArrayLengthProvider {
2831

2932
/**
30-
* @return the length of the array described by this node, or null if it is not available
33+
* The different modes that determine what the results of {@link GraphUtil#arrayLength} and
34+
* {@link ArrayLengthProvider#findLength} can be used for.
3135
*/
32-
ValueNode length();
36+
enum FindLengthMode {
37+
/**
38+
* Use the result of {@link GraphUtil#arrayLength} and
39+
* {@link ArrayLengthProvider#findLength} to replace the explicit load of the array length
40+
* with a node that does not involve a memory access of the array length.
41+
*
42+
* Values that are defined inside a loop and flow out the loop need to be proxied by
43+
* {@link ValueProxyNode}. When this mode is used, new necessary proxy nodes are created
44+
* base on the proxies that were found while traversing the path to the length node. In
45+
* addition, new {@link ValuePhiNode phi nodes} can be created. The caller is responsible
46+
* for adding these nodes to the graph, i.e., the return value can be a node that is not yet
47+
* added to the graph.
48+
*/
49+
CANONICALIZE_READ,
50+
51+
/**
52+
* Use the result of {@link GraphUtil#arrayLength} and
53+
* {@link ArrayLengthProvider#findLength} only for decisions whether a certain optimization
54+
* is possible. No new nodes are created during the search, i.e., the result is either a
55+
* node that is already in the graph, or null.
56+
*/
57+
SEARCH_ONLY
58+
}
59+
60+
/**
61+
* Returns the length of the array described by this node, or null if it is not available.
62+
* Details of the different modes are documented in {@link FindLengthMode}.
63+
*
64+
* This method should not be called directly. Use {@link GraphUtil#arrayLength} instead.
65+
*/
66+
ValueNode findLength(FindLengthMode mode);
3367
}

0 commit comments

Comments
 (0)