Skip to content

Commit 6c5ee7f

Browse files
davleopodavleopo
committed
[GR-55884] Kill all finals on volatile.
PullRequest: graal/18392
2 parents 4b7b8d0 + 798af44 commit 6c5ee7f

File tree

2 files changed

+127
-34
lines changed

2 files changed

+127
-34
lines changed

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

Lines changed: 126 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,23 @@
2424
*/
2525
package jdk.graal.compiler.core.test.ea;
2626

27+
import java.util.ArrayList;
28+
import java.util.Collections;
29+
import java.util.List;
30+
2731
import org.junit.Assert;
2832
import org.junit.Test;
2933

34+
import jdk.graal.compiler.api.directives.GraalDirectives;
35+
import jdk.graal.compiler.core.common.GraalOptions;
3036
import jdk.graal.compiler.core.test.GraalCompilerTest;
37+
import jdk.graal.compiler.debug.GraalError;
3138
import jdk.graal.compiler.nodes.StructuredGraph;
3239
import jdk.graal.compiler.nodes.StructuredGraph.AllowAssumptions;
3340
import jdk.graal.compiler.nodes.java.LoadFieldNode;
41+
import jdk.graal.compiler.options.OptionValues;
3442
import jdk.graal.compiler.virtual.phases.ea.ReadEliminationPhase;
43+
import jdk.vm.ci.code.InstalledCode;
3544

3645
public class FinalReadEliminationTest extends GraalCompilerTest {
3746

@@ -59,7 +68,7 @@ public void testAccrossVolatile01() {
5968
StructuredGraph g = parseEager(getResolvedJavaMethod("snippetAccessVolatile1"), AllowAssumptions.NO);
6069
createCanonicalizerPhase().apply(g, getDefaultHighTierContext());
6170
new ReadEliminationPhase(createCanonicalizerPhase()).apply(g, getDefaultHighTierContext());
62-
Assert.assertEquals(1, g.getNodes().filter(LoadFieldNode.class).filter(x -> !((LoadFieldNode) x).field().isVolatile()).count());
71+
Assert.assertEquals(2, g.getNodes().filter(LoadFieldNode.class).filter(x -> !((LoadFieldNode) x).field().isVolatile()).count());
6372
}
6473

6574
public static int snippetAccessVolatile2(A a) {
@@ -76,4 +85,120 @@ public void testAccrossVolatile02() {
7685
new ReadEliminationPhase(createCanonicalizerPhase()).apply(g, getDefaultHighTierContext());
7786
Assert.assertEquals(2, g.getNodes().filter(LoadFieldNode.class).filter(x -> !((LoadFieldNode) x).field().isVolatile()).count());
7887
}
88+
89+
// Checkstyle: stop
90+
public static final boolean LOG = false;
91+
92+
static class ThisEscaper {
93+
final int x;
94+
static volatile ThisEscaper volatileField1;
95+
96+
ThisEscaper() {
97+
volatileField1 = this;
98+
while (!flag1) {
99+
}
100+
this.x = 42;
101+
flag = true;
102+
}
103+
}
104+
105+
static volatile boolean flag;
106+
107+
static volatile boolean flag1;
108+
109+
// compile this method
110+
static void readerAction() {
111+
if (GraalDirectives.inCompiledCode()) {
112+
if (LOG) {
113+
GraalDirectives.log("Executing compiled code\n");
114+
}
115+
}
116+
while (ThisEscaper.volatileField1 == null) {
117+
}
118+
ThisEscaper localA = ThisEscaper.volatileField1;
119+
int t1 = localA.x; // = 0
120+
while (!flag) {
121+
/* loop until it changed */
122+
}
123+
int t2 = localA.x; // = 42
124+
125+
if (t1 == t2) {
126+
throw new IllegalArgumentException("must be different values but is t1=" + t1 + " t2=" + t2);
127+
}
128+
if (t1 != 0 || t2 != 42) {
129+
throw new IllegalArgumentException("whaat t1=" + t1 + " t2=" + t2);
130+
}
131+
if (LOG) {
132+
System.out.printf("t1=%s t2=%s%n", t1, t2);
133+
}
134+
}
135+
136+
static Runnable readerRunnable = new Runnable() {
137+
138+
@Override
139+
public void run() {
140+
readerAction();
141+
}
142+
};
143+
static Runnable setRunnable = new Runnable() {
144+
145+
@Override
146+
public void run() {
147+
try {
148+
if (LOG) {
149+
System.out.println("Starting sleeping before setting flag1");
150+
}
151+
Thread.sleep(1000 * 2);
152+
if (LOG) {
153+
System.out.println("Done sleeping before setting flag1");
154+
}
155+
} catch (InterruptedException e) {
156+
throw GraalError.shouldNotReachHere(e);
157+
}
158+
flag1 = true;
159+
if (LOG) {
160+
System.out.println("Done setting flag1");
161+
}
162+
}
163+
};
164+
165+
@SuppressWarnings("unused")
166+
@Test
167+
public void testThreadsThisEscape() throws InterruptedException {
168+
// first do all compiles
169+
OptionValues opt = new OptionValues(getInitialOptions(), GraalOptions.OptConvertDeoptsToGuards, false);
170+
InstalledCode ic = getCode(getResolvedJavaMethod("readerAction"), null, true, true, opt);
171+
assert ic.isAlive();
172+
assert ic.isValid();
173+
174+
// then run the program
175+
Thread readerThread = new Thread(readerRunnable);
176+
Thread setThread = new Thread(setRunnable);
177+
178+
List<Throwable> t = Collections.synchronizedList(new ArrayList<Throwable>());
179+
180+
Thread.UncaughtExceptionHandler h = new Thread.UncaughtExceptionHandler() {
181+
@Override
182+
public void uncaughtException(Thread th, Throwable ex) {
183+
t.add(ex);
184+
}
185+
};
186+
187+
readerThread.setUncaughtExceptionHandler(h);
188+
setThread.setUncaughtExceptionHandler(h);
189+
190+
readerThread.start();
191+
setThread.start();
192+
193+
// start them then allocate
194+
new ThisEscaper();
195+
196+
readerThread.join();
197+
setThread.join();
198+
199+
for (Throwable throwable : t) {
200+
throw GraalError.shouldNotReachHere(throwable);
201+
}
202+
}
203+
// Checkstyle: resume
79204
}

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

Lines changed: 1 addition & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,9 @@
3030
import org.graalvm.collections.Equivalence;
3131
import org.graalvm.word.LocationIdentity;
3232

33-
import jdk.graal.compiler.debug.Assertions;
3433
import jdk.graal.compiler.graph.Node;
35-
import jdk.graal.compiler.nodes.FieldLocationIdentity;
3634
import jdk.graal.compiler.nodes.ValueNode;
37-
import jdk.graal.compiler.nodes.java.LoadFieldNode;
3835
import jdk.vm.ci.meta.JavaKind;
39-
import jdk.vm.ci.meta.ResolvedJavaField;
4036

4137
/**
4238
* This class maintains a set of known values, identified by base object, locations and offset.
@@ -202,36 +198,8 @@ public ValueNode getCacheEntry(CacheEntry<?> identifier) {
202198
* array access. This method must implement Java semantic for regular fields, array accesses,
203199
* volatile operations etc.
204200
*/
205-
public void killReadCache(Node kill, LocationIdentity identity, ValueNode index, ValueNode array) {
201+
public void killReadCache(@SuppressWarnings("unused") Node kill, LocationIdentity identity, ValueNode index, ValueNode array) {
206202
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-
}
235203
/**
236204
* Kill all mutable locations.
237205
*/

0 commit comments

Comments
 (0)