Skip to content

Commit c871185

Browse files
davleopodavleopo
committed
[GR-28037] Do not kill final field locations for volatile operations during read elimination.
PullRequest: graal/18015
2 parents bb7f14e + fa1adc4 commit c871185

File tree

4 files changed

+257
-8
lines changed

4 files changed

+257
-8
lines changed

‎compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/ea/FinalReadEliminationTest.java

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,79 @@
1+
/*
2+
* Copyright (c) 2024, 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 jdk.graal.compiler.core.test.ea;
26+
27+
import org.junit.Assert;
28+
import org.junit.Test;
29+
30+
import jdk.graal.compiler.core.test.GraalCompilerTest;
31+
import jdk.graal.compiler.nodes.StructuredGraph;
32+
import jdk.graal.compiler.nodes.StructuredGraph.AllowAssumptions;
33+
import jdk.graal.compiler.nodes.java.LoadFieldNode;
34+
import jdk.graal.compiler.virtual.phases.ea.ReadEliminationPhase;
35+
36+
public class FinalReadEliminationTest extends GraalCompilerTest {
37+
38+
static class A {
39+
final int x;
40+
41+
A(int x) {
42+
this.x = x;
43+
}
44+
}
45+
46+
static volatile int accross;
47+
48+
static int S;
49+
50+
public static int snippetAccessVolatile1(A a) {
51+
int load1 = a.x;
52+
S = accross;
53+
int load2 = a.x;
54+
return load1 + load2;
55+
}
56+
57+
@Test
58+
public void testAccrossVolatile01() {
59+
StructuredGraph g = parseEager(getResolvedJavaMethod("snippetAccessVolatile1"), AllowAssumptions.NO);
60+
createCanonicalizerPhase().apply(g, getDefaultHighTierContext());
61+
new ReadEliminationPhase(createCanonicalizerPhase()).apply(g, getDefaultHighTierContext());
62+
Assert.assertEquals(1, g.getNodes().filter(LoadFieldNode.class).filter(x -> !((LoadFieldNode) x).field().isVolatile()).count());
63+
}
64+
65+
public static int snippetAccessVolatile2(A a) {
66+
int load1 = a.x;
67+
accross = load1;
68+
int load2 = a.x;
69+
return load1 + load2;
70+
}
71+
72+
@Test
73+
public void testAccrossVolatile02() {
74+
StructuredGraph g = parseEager(getResolvedJavaMethod("snippetAccessVolatile2"), AllowAssumptions.NO);
75+
createCanonicalizerPhase().apply(g, getDefaultHighTierContext());
76+
new ReadEliminationPhase(createCanonicalizerPhase()).apply(g, getDefaultHighTierContext());
77+
Assert.assertEquals(2, g.getNodes().filter(LoadFieldNode.class).filter(x -> !((LoadFieldNode) x).field().isVolatile()).count());
78+
}
79+
}

‎compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/ea/ReadEliminationCodeEmissionTest.java

Lines changed: 131 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,131 @@
1+
/*
2+
* Copyright (c) 2024, 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 jdk.graal.compiler.core.test.ea;
26+
27+
import java.io.IOException;
28+
import java.lang.reflect.InvocationTargetException;
29+
import java.lang.reflect.Method;
30+
31+
import org.junit.Test;
32+
33+
import jdk.graal.compiler.core.test.SubprocessTest;
34+
import jdk.graal.compiler.hotspot.GraalHotSpotVMConfig;
35+
import jdk.graal.compiler.hotspot.HotSpotBackend;
36+
import jdk.graal.compiler.test.SubprocessUtil;
37+
import jdk.vm.ci.code.InstalledCode;
38+
import jdk.vm.ci.code.InvalidInstalledCodeException;
39+
40+
public class ReadEliminationCodeEmissionTest extends SubprocessTest {
41+
42+
static class A {
43+
final int x;
44+
45+
A(int x) {
46+
this.x = x;
47+
}
48+
}
49+
50+
static volatile int accross;
51+
52+
static int S;
53+
54+
public static int accessVolatile1Snippet(A a) {
55+
int load1 = a.x;
56+
S = accross;
57+
int load2 = a.x;
58+
return load1 + load2;
59+
}
60+
61+
@Test
62+
public void accessVolatile1() {
63+
runTest("accessVolatile1", new A(12));
64+
}
65+
66+
public void runTest(String baseName, Object... args) {
67+
String snippetName = baseName + "Snippet";
68+
String methodSpec = getClass().getName() + "::" + snippetName;
69+
Method m = getMethod(snippetName);
70+
71+
Runnable run = () -> {
72+
// Force compilation with HotSpot
73+
for (int i = 0; i < 100000; i++) {
74+
try {
75+
Object[] finalArgs = applyArgSuppliers(args);
76+
m.invoke(null, finalArgs);
77+
} catch (IllegalAccessException | InvocationTargetException e) {
78+
throw new RuntimeException(e);
79+
}
80+
}
81+
if (args.length != 0) {
82+
Object[] nullArgs = args.clone();
83+
for (int i = 0; i < nullArgs.length; i++) {
84+
nullArgs[i] = null;
85+
}
86+
test(snippetName, nullArgs);
87+
}
88+
// Now generate JVMCI code
89+
InstalledCode code = getCode(getResolvedJavaMethod(snippetName));
90+
for (int i = 0; i < 100000; i++) {
91+
try {
92+
Object[] finalArgs = applyArgSuppliers(args);
93+
code.executeVarargs(finalArgs);
94+
if (i % 1000 == 0) {
95+
System.gc();
96+
}
97+
} catch (InvalidInstalledCodeException e) {
98+
throw new RuntimeException(e);
99+
}
100+
}
101+
102+
};
103+
104+
GraalHotSpotVMConfig config = ((HotSpotBackend) getBackend()).getRuntime().getVMConfig();
105+
SubprocessUtil.Subprocess subprocess = null;
106+
String logName = null;
107+
String[] vmArgs = new String[0];
108+
boolean print = Boolean.getBoolean("debug." + this.getClass().getName() + ".print");
109+
if (print) {
110+
logName = config.gc.name() + "_" + baseName + ".log";
111+
vmArgs = new String[]{"-XX:CompileCommand=print," + methodSpec,
112+
"-XX:CompileCommand=dontinline," + methodSpec,
113+
"-XX:+UnlockDiagnosticVMOptions",
114+
"-XX:-DisplayVMOutput",
115+
"-XX:-TieredCompilation",
116+
"-XX:+LogVMOutput",
117+
"-XX:+PreserveFramePointer",
118+
"-Xbatch",
119+
"-XX:LogFile=" + logName,
120+
"-Dgraal.Dump=:5"};
121+
}
122+
try {
123+
subprocess = launchSubprocess(baseName, run, vmArgs);
124+
} catch (InterruptedException | IOException e) {
125+
throw new RuntimeException(e);
126+
}
127+
if (subprocess != null && logName != null) {
128+
System.out.println("HotSpot output saved in " + logName);
129+
}
130+
}
131+
}

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/virtual/phases/ea/ReadEliminationBlockState.java

Lines changed: 41 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,15 @@
2828

2929
import org.graalvm.collections.EconomicMap;
3030
import org.graalvm.collections.Equivalence;
31-
import jdk.graal.compiler.nodes.ValueNode;
3231
import org.graalvm.word.LocationIdentity;
3332

33+
import jdk.graal.compiler.debug.Assertions;
34+
import jdk.graal.compiler.graph.Node;
35+
import jdk.graal.compiler.nodes.FieldLocationIdentity;
36+
import jdk.graal.compiler.nodes.ValueNode;
37+
import jdk.graal.compiler.nodes.java.LoadFieldNode;
3438
import jdk.vm.ci.meta.JavaKind;
39+
import jdk.vm.ci.meta.ResolvedJavaField;
3540

3641
/**
3742
* This class maintains a set of known values, identified by base object, locations and offset.
@@ -191,8 +196,42 @@ public ValueNode getCacheEntry(CacheEntry<?> identifier) {
191196
return readCache.get(identifier);
192197
}
193198

194-
public void killReadCache(LocationIdentity identity, ValueNode index, ValueNode array) {
199+
/**
200+
* Kill the cache for memory accesses established so far down the control flow graph.
201+
* {@code kill} represents a memory kill to location {@code identity}, potentially expressing an
202+
* array access. This method must implement Java semantic for regular fields, array accesses,
203+
* volatile operations etc.
204+
*/
205+
public void killReadCache(Node kill, LocationIdentity identity, ValueNode index, ValueNode array) {
195206
if (identity.isAny()) {
207+
if (kill instanceof LoadFieldNode af && af.ordersMemoryAccesses()) {
208+
ResolvedJavaField field = af.field();
209+
if (field.isVolatile()) {
210+
/**
211+
* See <a href="https://gee.cs.oswego.edu/dl/jmm/cookbook.html"> JSR-133
212+
* Cookbook</a> for details
213+
*
214+
* Normal load operations can be reordered with volatile load operations.
215+
*/
216+
Iterator<CacheEntry<?>> iterator = readCache.getKeys().iterator();
217+
while (iterator.hasNext()) {
218+
CacheEntry<?> entry = iterator.next();
219+
LocationIdentity location = entry.getIdentity();
220+
if (location instanceof FieldLocationIdentity fl && fl.getField().isFinal()) {
221+
assert !fl.getField().equals(field) : Assertions.errorMessage("Field cannot be final and volatile at the same time", fl, fl.getField(), identity, af);
222+
/**
223+
* Access to a different final field - this does not overlap with the
224+
* volatile memory effect.
225+
*/
226+
} else {
227+
if (entry.getIdentity().isMutable()) {
228+
iterator.remove();
229+
}
230+
}
231+
}
232+
return;
233+
}
234+
}
196235
/**
197236
* Kill all mutable locations.
198237
*/

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/virtual/phases/ea/ReadEliminationClosure.java

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ protected boolean processNode(Node node, ReadEliminationBlockState state, GraphE
121121
deleted = true;
122122
}
123123
// will be a field location identity not killing array accesses
124-
killReadCacheByIdentity(state, identifier.identity);
124+
killReadCacheByIdentity(state, identifier.identity, node);
125125
state.addCacheEntry(identifier, value);
126126
} else if (node instanceof RawStoreNode) {
127127
RawStoreNode write = (RawStoreNode) node;
@@ -134,15 +134,15 @@ protected boolean processNode(Node node, ReadEliminationBlockState state, GraphE
134134
effects.deleteNode(write);
135135
deleted = true;
136136
}
137-
killReadCacheByIdentity(state, write.getKilledLocationIdentity());
137+
killReadCacheByIdentity(state, write.getKilledLocationIdentity(), node);
138138
state.addCacheEntry(identifier, value);
139139
}
140140
} else {
141-
killReadCacheByIdentity(state, identity);
141+
killReadCacheByIdentity(state, identity, node);
142142
}
143143
} else if (MemoryKill.isMultiMemoryKill(node)) {
144144
for (LocationIdentity identity : ((MultiMemoryKill) node).getKilledLocationIdentities()) {
145-
killReadCacheByIdentity(state, identity);
145+
killReadCacheByIdentity(state, identity, node);
146146
}
147147
} else {
148148
throw GraalError.shouldNotReachHere("Unknown memory kill " + node); // ExcludeFromJacocoGeneratedReport
@@ -227,8 +227,8 @@ private static GuardingNode getGuard(ValueNode node) {
227227
return null;
228228
}
229229

230-
private static void killReadCacheByIdentity(ReadEliminationBlockState state, LocationIdentity identity) {
231-
state.killReadCache(identity, null, null);
230+
private static void killReadCacheByIdentity(ReadEliminationBlockState state, LocationIdentity identity, Node kill) {
231+
state.killReadCache(kill, identity, null, null);
232232
}
233233

234234
@Override

0 commit comments

Comments
 (0)