Skip to content

Commit 63f1a04

Browse files
lukasstadlerlukasstadler
committed
[GR-37543] Restrict phi transformation to bit extends created by Truffle.
PullRequest: graal/11457
2 parents 3d209e8 + e29ed5b commit 63f1a04

File tree

8 files changed

+206
-21
lines changed

8 files changed

+206
-21
lines changed

‎compiler/src/org.graalvm.compiler.truffle.compiler/src/org/graalvm/compiler/truffle/compiler/TruffleCompilerImpl.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,9 @@
7878
import org.graalvm.compiler.lir.asm.CompilationResultBuilderFactory;
7979
import org.graalvm.compiler.lir.phases.LIRSuites;
8080
import org.graalvm.compiler.nodes.Cancellable;
81+
import org.graalvm.compiler.nodes.NodeView;
8182
import org.graalvm.compiler.nodes.StructuredGraph;
83+
import org.graalvm.compiler.nodes.calc.ZeroExtendNode;
8284
import org.graalvm.compiler.nodes.graphbuilderconf.GraphBuilderConfiguration;
8385
import org.graalvm.compiler.nodes.graphbuilderconf.GraphBuilderConfiguration.BytecodeExceptionMode;
8486
import org.graalvm.compiler.nodes.graphbuilderconf.GraphBuilderConfiguration.Plugins;
@@ -99,6 +101,7 @@
99101
import org.graalvm.compiler.truffle.common.TruffleDebugContext;
100102
import org.graalvm.compiler.truffle.common.TruffleDebugJavaMethod;
101103
import org.graalvm.compiler.truffle.common.TruffleInliningData;
104+
import org.graalvm.compiler.truffle.compiler.nodes.AnyExtendNode;
102105
import org.graalvm.compiler.truffle.compiler.nodes.TruffleAssumption;
103106
import org.graalvm.compiler.truffle.compiler.phases.InstrumentPhase;
104107
import org.graalvm.compiler.truffle.compiler.phases.InstrumentationSuite;
@@ -565,6 +568,13 @@ private StructuredGraph truffleTier(TruffleCompilationWrapper wrapper, DebugCont
565568
return graph;
566569
}
567570

571+
private static void replaceAnyExtendNodes(StructuredGraph graph) {
572+
// replace all AnyExtendNodes with ZeroExtendNodes
573+
for (AnyExtendNode node : graph.getNodes(AnyExtendNode.TYPE)) {
574+
node.replaceAndDelete(graph.addOrUnique(ZeroExtendNode.create(node.getValue(), 64, NodeView.DEFAULT)));
575+
}
576+
}
577+
568578
/**
569579
* Hook for processing bailout exceptions.
570580
*
@@ -594,6 +604,8 @@ public CompilationResult compilePEGraph(StructuredGraph graph,
594604
CompilationRequest compilationRequest,
595605
TruffleCompilerListener listener,
596606
TruffleCompilationTask task) {
607+
608+
replaceAnyExtendNodes(graph);
597609
DebugContext debug = graph.getDebug();
598610
try (DebugContext.Scope s = debug.scope("TruffleFinal")) {
599611
debug.dump(DebugContext.BASIC_LEVEL, graph, "After TruffleTier");

‎compiler/src/org.graalvm.compiler.truffle.compiler/src/org/graalvm/compiler/truffle/compiler/nodes/AnyExtendNode.java

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
/*
2+
* Copyright (c) 2014, 2022, 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. Oracle designates this
8+
* particular file as subject to the "Classpath" exception as provided
9+
* by Oracle in the LICENSE file that accompanied this code.
10+
*
11+
* This code is distributed in the hope that it will be useful, but WITHOUT
12+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
13+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
14+
* version 2 for more details (a copy is included in the LICENSE file that
15+
* accompanied this code).
16+
*
17+
* You should have received a copy of the GNU General Public License version
18+
* 2 along with this work; if not, write to the Free Software Foundation,
19+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
20+
*
21+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
22+
* or visit www.oracle.com if you need additional information or have any
23+
* questions.
24+
*/
25+
package org.graalvm.compiler.truffle.compiler.nodes;
26+
27+
import static org.graalvm.compiler.nodeinfo.NodeCycles.CYCLES_1;
28+
29+
import org.graalvm.compiler.core.common.type.IntegerStamp;
30+
import org.graalvm.compiler.core.common.type.StampFactory;
31+
import org.graalvm.compiler.graph.IterableNodeType;
32+
import org.graalvm.compiler.graph.NodeClass;
33+
import org.graalvm.compiler.nodeinfo.NodeInfo;
34+
import org.graalvm.compiler.nodeinfo.NodeSize;
35+
import org.graalvm.compiler.nodes.NodeView;
36+
import org.graalvm.compiler.nodes.ValueNode;
37+
import org.graalvm.compiler.nodes.calc.FloatingNode;
38+
import org.graalvm.compiler.nodes.calc.SignExtendNode;
39+
import org.graalvm.compiler.nodes.calc.ZeroExtendNode;
40+
41+
import jdk.vm.ci.meta.JavaKind;
42+
43+
/**
44+
* The {@code AnyExtendNode} extends an i32 integer to an i64 integer, with the upper bits of the
45+
* result being undefined. This node itself cannot produce code, it needs to be converted to a
46+
* {@link ZeroExtendNode} or {@link SignExtendNode} explicitly.
47+
*/
48+
@NodeInfo(cycles = CYCLES_1, size = NodeSize.SIZE_1)
49+
public final class AnyExtendNode extends FloatingNode implements IterableNodeType {
50+
51+
public static final NodeClass<AnyExtendNode> TYPE = NodeClass.create(AnyExtendNode.class);
52+
53+
public static final int INPUT_BITS = 32;
54+
public static final int OUTPUT_BITS = 64;
55+
56+
@Input protected ValueNode value;
57+
58+
public AnyExtendNode(ValueNode value) {
59+
super(TYPE, StampFactory.forKind(JavaKind.Long));
60+
assert value.stamp(NodeView.DEFAULT) instanceof IntegerStamp;
61+
this.value = value;
62+
}
63+
64+
public ValueNode getValue() {
65+
return value;
66+
}
67+
}

‎compiler/src/org.graalvm.compiler.truffle.compiler/src/org/graalvm/compiler/truffle/compiler/phases/PhiTransformPhase.java

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -51,14 +51,16 @@
5151
import org.graalvm.compiler.phases.BasePhase;
5252
import org.graalvm.compiler.phases.common.CanonicalizerPhase;
5353
import org.graalvm.compiler.phases.common.util.EconomicSetNodeEventListener;
54+
import org.graalvm.compiler.truffle.compiler.nodes.AnyExtendNode;
5455

5556
import jdk.vm.ci.meta.ResolvedJavaType;
5657

5758
/**
5859
* This phase recognizes {@link ValuePhiNode phis} that are repeatedly converted back and forth with
5960
* lossless conversions. The main use case is i64 phis that actually contain i32/f32/f64 values and
60-
* that use {@link NarrowNode}, {@link ZeroExtendNode} and {@link ReinterpretNode} for conversion.
61-
* In loop nests and complex control flow, multiple phis may need to be transformed as a group.<br/>
61+
* that use {@link NarrowNode}, {@link ZeroExtendNode}, {@link SignExtendNode},
62+
* {@link AnyExtendNode} and {@link ReinterpretNode} for conversion. In loop nests and complex
63+
* control flow, multiple phis may need to be transformed as a group.<br/>
6264
*
6365
* In order to be considered, phis can only have constants, other phis in the group, or an
6466
* appropriate conversion as an input (see
@@ -188,7 +190,14 @@ private static boolean isValidInput(ValueNode value, UnaryNode transformation, b
188190
// not the exact reverse of the narrow
189191
return false;
190192
}
191-
return !usedInState || convert instanceof ZeroExtendNode;
193+
return !usedInState;
194+
}
195+
if (value instanceof AnyExtendNode) {
196+
if (narrow.getResultBits() != AnyExtendNode.INPUT_BITS || narrow.getInputBits() != AnyExtendNode.OUTPUT_BITS) {
197+
// not the exact reverse of the narrow
198+
return false;
199+
}
200+
return true;
192201
}
193202
} else {
194203
assert transformation instanceof ReinterpretNode;
@@ -204,6 +213,9 @@ private static ValueNode transformInputValue(StructuredGraph graph, ValueNode va
204213
} else {
205214
if (transformation instanceof NarrowNode) {
206215
NarrowNode narrow = (NarrowNode) transformation;
216+
if (value instanceof AnyExtendNode && narrow.getInputBits() == AnyExtendNode.OUTPUT_BITS && narrow.getResultBits() == AnyExtendNode.INPUT_BITS) {
217+
return ((AnyExtendNode) value).getValue();
218+
}
207219
newValue = graph.unique(new NarrowNode(value, narrow.getInputBits(), narrow.getResultBits()));
208220
} else {
209221
assert transformation instanceof ReinterpretNode;
@@ -241,14 +253,12 @@ private static boolean checkTransformedNode(StructuredGraph graph, EconomicSetNo
241253
// look at the inputs
242254
if (node.getClass() == ValuePhiNode.class) {
243255
ReinterpretNode reinterpret = ((ValuePhiNode) node).values().filter(ReinterpretNode.class).first();
244-
IntegerConvertNode<?> convert = ((ValuePhiNode) node).values().filter(IntegerConvertNode.class).first();
256+
AnyExtendNode convert = ((ValuePhiNode) node).values().filter(AnyExtendNode.class).first();
245257

246258
if (reinterpret != null && convert == null) {
247259
transformation = new ReinterpretNode(reinterpret.getValue().getStackKind(), (ValueNode) node);
248260
} else if (reinterpret == null && convert != null) {
249-
if (convert.getInputBits() == 64 && convert.getResultBits() == 64) {
250-
transformation = new NarrowNode((ValueNode) node, 64, 32);
251-
}
261+
transformation = new NarrowNode((ValueNode) node, AnyExtendNode.OUTPUT_BITS, AnyExtendNode.INPUT_BITS);
252262
}
253263
}
254264
if (transformation == null) {
@@ -311,14 +321,16 @@ private static boolean checkTransformedNode(StructuredGraph graph, EconomicSetNo
311321
target.replaceAndDelete(duplicate);
312322
}
313323
return true;
314-
} else if (node.getClass() == ZeroExtendNode.class) {
315-
ZeroExtendNode extend = (ZeroExtendNode) node;
316-
if (node.hasExactlyOneUsage() && extend.getInputBits() == 32 && extend.getResultBits() == 64 && isValidStateUsage(node.singleUsage(), longClass)) {
324+
} else if (node.getClass() == AnyExtendNode.class) {
325+
AnyExtendNode extend = (AnyExtendNode) node;
326+
if (node.hasExactlyOneUsage() && isValidStateUsage(node.singleUsage(), longClass)) {
317327
node.replaceAtUsagesAndDelete(extend.getValue());
328+
return true;
318329
}
319330
} else if (node.getClass() == ReinterpretNode.class) {
320331
if (node.hasExactlyOneUsage() && isValidStateUsage(node.singleUsage(), longClass)) {
321332
node.replaceAtUsagesAndDelete(((ReinterpretNode) node).getValue());
333+
return true;
322334
}
323335
}
324336
return false;

‎compiler/src/org.graalvm.compiler.truffle.compiler/src/org/graalvm/compiler/truffle/compiler/substitutions/TruffleGraphBuilderPlugins.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@
9494
import org.graalvm.compiler.truffle.common.TruffleCompilerRuntime;
9595
import org.graalvm.compiler.truffle.common.TruffleDebugJavaMethod;
9696
import org.graalvm.compiler.truffle.compiler.PerformanceInformationHandler;
97+
import org.graalvm.compiler.truffle.compiler.nodes.AnyExtendNode;
9798
import org.graalvm.compiler.truffle.compiler.nodes.IsCompilationConstantNode;
9899
import org.graalvm.compiler.truffle.compiler.nodes.ObjectLocationIdentity;
99100
import org.graalvm.compiler.truffle.compiler.nodes.TruffleAssumption;
@@ -797,6 +798,13 @@ public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Rec
797798
return false;
798799
}
799800
});
801+
r.register(new RequiredInvocationPlugin("extend", int.class) {
802+
@Override
803+
public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Receiver receiver, ValueNode value) {
804+
b.addPush(JavaKind.Long, new AnyExtendNode(value));
805+
return true;
806+
}
807+
});
800808
}
801809

802810
/**

‎compiler/src/org.graalvm.compiler.truffle.runtime/src/org/graalvm/compiler/truffle/runtime/BytecodeOSRRootNode.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,11 @@ public Object executeOSR(VirtualFrame frame) {
7676
}
7777
}
7878

79+
@Override
80+
public String getName() {
81+
return toString();
82+
}
83+
7984
@Override
8085
public String toString() {
8186
return loopNode.toString() + "<OSR@" + target + ">";

‎compiler/src/org.graalvm.compiler.truffle.test/src/org/graalvm/compiler/truffle/test/PhiTransformTest.java

Lines changed: 75 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,14 +25,23 @@
2525

2626
package org.graalvm.compiler.truffle.test;
2727

28+
import java.util.ListIterator;
29+
30+
import org.graalvm.compiler.api.directives.GraalDirectives;
2831
import org.graalvm.compiler.core.test.GraalCompilerTest;
32+
import org.graalvm.compiler.nodes.NodeView;
2933
import org.graalvm.compiler.nodes.StructuredGraph;
3034
import org.graalvm.compiler.nodes.StructuredGraph.AllowAssumptions;
3135
import org.graalvm.compiler.nodes.calc.NarrowNode;
3236
import org.graalvm.compiler.nodes.calc.ReinterpretNode;
3337
import org.graalvm.compiler.nodes.calc.SignExtendNode;
3438
import org.graalvm.compiler.nodes.calc.ZeroExtendNode;
3539
import org.graalvm.compiler.nodes.spi.CoreProviders;
40+
import org.graalvm.compiler.options.OptionValues;
41+
import org.graalvm.compiler.phases.BasePhase;
42+
import org.graalvm.compiler.phases.tiers.HighTierContext;
43+
import org.graalvm.compiler.phases.tiers.Suites;
44+
import org.graalvm.compiler.truffle.compiler.nodes.AnyExtendNode;
3645
import org.graalvm.compiler.truffle.compiler.phases.PhiTransformPhase;
3746
import org.graalvm.compiler.virtual.phases.ea.PartialEscapePhase;
3847
import org.junit.Assert;
@@ -96,6 +105,34 @@ public void succeed2() {
96105
Assert.assertTrue(graph.getNodes().filter(ReinterpretNode.class).count() + graph.getNodes().filter(NarrowNode.class).count() == 0);
97106
}
98107

108+
public static float deoptSnippet(int count) {
109+
/*
110+
* This test will fail with a wrong result if the phi transformation happens, since the
111+
* upper 32 bits of the int-in-long are observed after the deopt.
112+
*/
113+
long[] values = new long[2];
114+
float s = 0;
115+
do {
116+
values[0] = ((int) values[0] + 1) & 0xffffffffL;
117+
s++;
118+
GraalDirectives.sideEffect();
119+
if (s > 30) {
120+
// value is used as i64 after deopt:
121+
GraalDirectives.deoptimize();
122+
if (values[0] < 0 || values[0] >= count) {
123+
break;
124+
}
125+
}
126+
} while ((int) values[0] < count);
127+
return s;
128+
}
129+
130+
@Test
131+
public void deopt() {
132+
StructuredGraph graph = createGraph("deoptSnippet");
133+
Assert.assertTrue(graph.getNodes().filter(ReinterpretNode.class).count() + graph.getNodes().filter(NarrowNode.class).count() == 1);
134+
}
135+
99136
public static float fail1Snippet(int count) {
100137
long[] values = new long[1];
101138
float s = 0;
@@ -184,14 +221,49 @@ public void fail5() {
184221
Assert.assertTrue(graph.getNodes().filter(NarrowNode.class).count() > 0);
185222
}
186223

224+
@Override
225+
protected Suites createSuites(OptionValues opts) {
226+
Suites suites = super.createSuites(opts);
227+
ListIterator<BasePhase<? super HighTierContext>> iter = suites.getHighTier().findPhase(PartialEscapePhase.class);
228+
iter.add(new ToAnyExtendsPhase());
229+
iter.add(new PhiTransformPhase(createCanonicalizerPhase()));
230+
iter.add(new ToZeroExtendsPhase());
231+
return suites;
232+
}
233+
187234
private StructuredGraph createGraph(String name) {
188235
test(name, 100);
189236
StructuredGraph graph = parseEager(name, AllowAssumptions.YES);
190237

191-
CoreProviders context = getProviders();
238+
createCanonicalizerPhase().apply(graph, getProviders());
239+
HighTierContext context = getDefaultHighTierContext();
240+
new PartialEscapePhase(false, createCanonicalizerPhase(), getInitialOptions()).apply(graph, context);
241+
new ToAnyExtendsPhase().apply(graph, context);
242+
new PhiTransformPhase(createCanonicalizerPhase()).apply(graph, context);
243+
new ToZeroExtendsPhase().apply(graph, context);
192244
createCanonicalizerPhase().apply(graph, context);
193-
new PartialEscapePhase(false, createCanonicalizerPhase(), getInitialOptions()).apply(graph, getDefaultHighTierContext());
194-
new PhiTransformPhase(createCanonicalizerPhase()).apply(graph, getDefaultHighTierContext());
195245
return graph;
196246
}
247+
248+
static final class ToAnyExtendsPhase extends BasePhase<CoreProviders> {
249+
250+
@Override
251+
protected void run(StructuredGraph graph, CoreProviders context) {
252+
if (!graph.toString().contains("deoptSnippet")) {
253+
for (ZeroExtendNode node : graph.getNodes().filter(ZeroExtendNode.class)) {
254+
node.replaceAndDelete(graph.unique(new AnyExtendNode(node.getValue())));
255+
}
256+
}
257+
}
258+
}
259+
260+
static final class ToZeroExtendsPhase extends BasePhase<CoreProviders> {
261+
262+
@Override
263+
protected void run(StructuredGraph graph, CoreProviders context) {
264+
for (AnyExtendNode node : graph.getNodes().filter(AnyExtendNode.class)) {
265+
node.replaceAndDelete(graph.addOrUnique(ZeroExtendNode.create(node.getValue(), 64, NodeView.DEFAULT)));
266+
}
267+
}
268+
}
197269
}

‎sulong/projects/com.oracle.truffle.llvm.runtime/src/com/oracle/truffle/llvm/runtime/nodes/control/LLVMDispatchBasicBlockNode.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,11 @@ public LLVMDispatchBasicBlockNode(int exceptionValueSlot, LLVMBasicBlockNode[] b
7070
this.debugInfo = debugInfo;
7171
}
7272

73+
@Override
74+
public String toString() {
75+
return getRootNode() != null ? getRootNode().toString() : "<OSR>";
76+
}
77+
7378
public LocalVariableDebugInfo getDebugInfo() {
7479
return debugInfo;
7580
}

0 commit comments

Comments
 (0)