Skip to content

Commit 7b92aa5

Browse files
author
Michael Haas
committed
[GR-65172] Insert load and store conversions for unsafe accesses during PEA.
PullRequest: graal/20858
2 parents 93aee84 + ce97031 commit 7b92aa5

File tree

7 files changed

+216
-54
lines changed

7 files changed

+216
-54
lines changed

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/ea/EATestBase.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,13 @@ public int hashCode() {
114114
}
115115
}
116116

117+
public int getFirstField() {
118+
if (firstFieldIsX) {
119+
return x;
120+
}
121+
return y;
122+
}
123+
117124
public void setFirstField(int v) {
118125
if (firstFieldIsX) {
119126
x = v;

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/ea/UnsafeEATest.java

Lines changed: 65 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,9 @@
2626

2727
import java.nio.ByteBuffer;
2828

29+
import org.junit.Assert;
30+
import org.junit.Test;
31+
2932
import jdk.graal.compiler.api.directives.GraalDirectives;
3033
import jdk.graal.compiler.graph.Graph;
3134
import jdk.graal.compiler.graph.Node;
@@ -37,9 +40,6 @@
3740
import jdk.graal.compiler.nodes.extended.RawStoreNode;
3841
import jdk.graal.compiler.nodes.extended.UnsafeAccessNode;
3942
import jdk.graal.compiler.nodes.java.LoadFieldNode;
40-
import org.junit.Assert;
41-
import org.junit.Test;
42-
4343
import jdk.vm.ci.meta.JavaConstant;
4444
import jdk.vm.ci.meta.JavaKind;
4545
import jdk.vm.ci.meta.ResolvedJavaMethod;
@@ -348,4 +348,66 @@ public static TestClassInt testDeoptLongConstantSnippet() {
348348
return x;
349349
}
350350

351+
static final int VALUE = 0xF0F0F0F0;
352+
353+
public static boolean getBoolean() {
354+
TestClassInt data = new TestClassInt(VALUE, VALUE);
355+
return UNSAFE.getBoolean(data, TestClassInt.fieldOffset1);
356+
}
357+
358+
public static long getByte() {
359+
TestClassInt data = new TestClassInt(VALUE, VALUE);
360+
return UNSAFE.getByte(data, TestClassInt.fieldOffset1);
361+
}
362+
363+
public static long getShort() {
364+
TestClassInt data = new TestClassInt(VALUE, VALUE);
365+
return UNSAFE.getShort(data, TestClassInt.fieldOffset1);
366+
}
367+
368+
public static long getChar() {
369+
TestClassInt data = new TestClassInt(VALUE, VALUE);
370+
return UNSAFE.getShort(data, TestClassInt.fieldOffset1);
371+
}
372+
373+
@Test
374+
public void testUnsafeLoad() {
375+
test("getBoolean");
376+
test("getByte");
377+
test("getShort");
378+
test("getChar");
379+
}
380+
381+
public static long setBoolean() {
382+
TestClassInt data = new TestClassInt(VALUE, VALUE);
383+
UNSAFE.putBoolean(data, TestClassInt.fieldOffset1, true);
384+
return data.getFirstField();
385+
}
386+
387+
public static long setByte() {
388+
TestClassInt data = new TestClassInt(VALUE, VALUE);
389+
UNSAFE.putByte(data, TestClassInt.fieldOffset1, (byte) VALUE);
390+
return data.getFirstField();
391+
}
392+
393+
public static long setShort() {
394+
TestClassInt data = new TestClassInt(VALUE, VALUE);
395+
UNSAFE.putShort(data, TestClassInt.fieldOffset1, (short) VALUE);
396+
return data.getFirstField();
397+
}
398+
399+
public static long setChar() {
400+
TestClassInt data = new TestClassInt(VALUE, VALUE);
401+
UNSAFE.putChar(data, TestClassInt.fieldOffset1, (char) VALUE);
402+
return data.getFirstField();
403+
}
404+
405+
@Test
406+
public void testUnsafeStore() {
407+
test("setBoolean");
408+
test("setByte");
409+
test("setShort");
410+
test("setChar");
411+
}
412+
351413
}

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/extended/RawLoadNode.java

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@
5555
import jdk.graal.compiler.nodes.type.StampTool;
5656
import jdk.graal.compiler.nodes.virtual.VirtualArrayNode;
5757
import jdk.graal.compiler.nodes.virtual.VirtualObjectNode;
58+
import jdk.graal.compiler.replacements.DefaultJavaLoweringProvider;
5859
import jdk.vm.ci.meta.JavaKind;
5960
import jdk.vm.ci.meta.ResolvedJavaField;
6061

@@ -176,7 +177,26 @@ public void virtualize(VirtualizerTool tool) {
176177
return;
177178
}
178179
}
179-
tool.replaceWith(entry);
180+
JavaKind kind = accessKind();
181+
ValueNode replacement = tool.getAlias(entry);
182+
183+
if (kind.getStackKind() == JavaKind.Int) {
184+
/*
185+
* Get the value as it would be actually stored in memory or in other
186+
* words only the bytes we are interested in. The type is defined by the
187+
* access kind, e.g. for the access kind byte and the value 0xF0F0F0F0
188+
* we actually want to have 0xF0.
189+
*/
190+
ValueNode narrowed = DefaultJavaLoweringProvider.implicitPrimitiveStoreConvert(kind, replacement);
191+
/*
192+
* Expand the value to 32 bits again and perform boolean coercion if
193+
* necessary.
194+
*/
195+
replacement = DefaultJavaLoweringProvider.implicitUnsafePrimitiveLoadConvert(kind, narrowed);
196+
}
197+
198+
tool.ensureAdded(replacement);
199+
tool.replaceWith(replacement);
180200
}
181201
}
182202
}

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/replacements/DefaultJavaLoweringProvider.java

Lines changed: 109 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@
8080
import jdk.graal.compiler.nodes.ValueNode;
8181
import jdk.graal.compiler.nodes.ValuePhiNode;
8282
import jdk.graal.compiler.nodes.calc.AddNode;
83+
import jdk.graal.compiler.nodes.calc.AndNode;
8384
import jdk.graal.compiler.nodes.calc.ConditionalNode;
8485
import jdk.graal.compiler.nodes.calc.FloatingIntegerDivRemNode;
8586
import jdk.graal.compiler.nodes.calc.IntegerBelowNode;
@@ -89,6 +90,7 @@
8990
import jdk.graal.compiler.nodes.calc.IsNullNode;
9091
import jdk.graal.compiler.nodes.calc.LeftShiftNode;
9192
import jdk.graal.compiler.nodes.calc.NarrowNode;
93+
import jdk.graal.compiler.nodes.calc.OrNode;
9294
import jdk.graal.compiler.nodes.calc.ReinterpretNode;
9395
import jdk.graal.compiler.nodes.calc.RightShiftNode;
9496
import jdk.graal.compiler.nodes.calc.SignExtendNode;
@@ -834,7 +836,7 @@ protected ReadNode createUnsafeRead(StructuredGraph graph, RawLoadNode load, Gua
834836
} else {
835837
memoryRead.setGuard(guard);
836838
}
837-
ValueNode readValue = performBooleanCoercionIfNecessary(implicitLoadConvert(graph, readKind, memoryRead, compressible), readKind);
839+
ValueNode readValue = implicitUnsafeLoadConvert(graph, readKind, memoryRead, compressible);
838840
load.replaceAtUsages(readValue);
839841
return memoryRead;
840842
}
@@ -849,18 +851,18 @@ protected void lowerUnsafeMemoryLoadNode(UnsafeMemoryLoadNode load) {
849851
// An unsafe read must not float otherwise it may float above
850852
// a test guaranteeing the read is safe.
851853
memoryRead.setForceFixed(true);
852-
ValueNode readValue = performBooleanCoercionIfNecessary(implicitLoadConvert(graph, readKind, memoryRead, false), readKind);
854+
ValueNode readValue = implicitUnsafeLoadConvert(graph, readKind, memoryRead, false);
853855
load.replaceAtUsages(readValue);
854856
graph.replaceFixedWithFixed(load, memoryRead);
855857
}
856858

857-
private static ValueNode performBooleanCoercionIfNecessary(ValueNode readValue, JavaKind readKind) {
858-
if (readKind == JavaKind.Boolean) {
859-
StructuredGraph graph = readValue.graph();
860-
IntegerEqualsNode eq = graph.addOrUnique(new IntegerEqualsNode(readValue, ConstantNode.forInt(0, graph)));
861-
return graph.addOrUnique(new ConditionalNode(eq, ConstantNode.forBoolean(false, graph), ConstantNode.forBoolean(true, graph)));
862-
}
863-
return readValue;
859+
/**
860+
* Coerce integer values into a boolean 0 or 1 to match Java semantics. The returned nodes have
861+
* not been added to the graph.
862+
*/
863+
private static ValueNode performBooleanCoercion(ValueNode readValue) {
864+
IntegerEqualsNode eq = new IntegerEqualsNode(readValue, ConstantNode.forInt(0));
865+
return new ConditionalNode(eq, ConstantNode.forBoolean(false), ConstantNode.forBoolean(true));
864866
}
865867

866868
protected void lowerUnsafeStoreNode(RawStoreNode store) {
@@ -1273,45 +1275,66 @@ protected Stamp loadStamp(Stamp stamp, JavaKind kind, boolean compressible) {
12731275
return stamp;
12741276
}
12751277

1276-
public final ValueNode implicitLoadConvertWithBooleanCoercionIfNecessary(StructuredGraph graph, JavaKind kind, ValueNode value) {
1277-
return performBooleanCoercionIfNecessary(implicitLoadConvert(graph, kind, value), kind);
1278+
protected abstract ValueNode newCompressionNode(CompressionOp op, ValueNode value);
1279+
1280+
/**
1281+
* Perform sign or zero extensions for subword types, and convert potentially unsafe 8 bit
1282+
* boolean values into 0 or 1. The nodes have already been added to the graph.
1283+
*/
1284+
public final ValueNode implicitUnsafeLoadConvert(StructuredGraph graph, JavaKind kind, ValueNode value, boolean compressible) {
1285+
if (compressible && kind.isObject()) {
1286+
return implicitLoadConvert(graph, kind, value, compressible);
1287+
} else {
1288+
ValueNode ret = implicitUnsafePrimitiveLoadConvert(kind, value);
1289+
if (!ret.isAlive()) {
1290+
ret = graph.addOrUniqueWithInputs(ret);
1291+
}
1292+
return ret;
1293+
}
12781294
}
12791295

12801296
public final ValueNode implicitLoadConvert(StructuredGraph graph, JavaKind kind, ValueNode value) {
12811297
return implicitLoadConvert(graph, kind, value, true);
12821298
}
12831299

1284-
public ValueNode implicitLoadConvert(JavaKind kind, ValueNode value) {
1285-
return implicitLoadConvert(kind, value, true);
1286-
}
1287-
1300+
/**
1301+
* Perform sign or zero extensions for subword types and add the nodes to the graph.
1302+
*/
12881303
protected final ValueNode implicitLoadConvert(StructuredGraph graph, JavaKind kind, ValueNode value, boolean compressible) {
1289-
ValueNode ret = implicitLoadConvert(kind, value, compressible);
1304+
ValueNode ret;
1305+
if (useCompressedOops(kind, compressible)) {
1306+
ret = newCompressionNode(CompressionOp.Uncompress, value);
1307+
} else {
1308+
ret = implicitPrimitiveLoadConvert(kind, value);
1309+
}
1310+
12901311
if (!ret.isAlive()) {
1291-
ret = graph.addOrUnique(ret);
1312+
ret = graph.addOrUniqueWithInputs(ret);
12921313
}
12931314
return ret;
12941315
}
12951316

1296-
protected abstract ValueNode newCompressionNode(CompressionOp op, ValueNode value);
1297-
12981317
/**
1299-
* @param compressible whether the convert should be compressible
1318+
* Perform sign or zero extensions for subword types. The caller is expected to add an resulting
1319+
* nodes to the graph.
13001320
*/
1301-
protected ValueNode implicitLoadConvert(JavaKind kind, ValueNode value, boolean compressible) {
1302-
if (useCompressedOops(kind, compressible)) {
1303-
return newCompressionNode(CompressionOp.Uncompress, value);
1304-
}
1321+
public static ValueNode implicitPrimitiveLoadConvert(JavaKind kind, ValueNode value) {
1322+
return switch (kind) {
1323+
case Byte, Short -> new SignExtendNode(value, 32);
1324+
case Boolean, Char -> new ZeroExtendNode(value, 32);
1325+
default -> value;
1326+
};
1327+
}
13051328

1306-
switch (kind) {
1307-
case Byte:
1308-
case Short:
1309-
return new SignExtendNode(value, 32);
1310-
case Boolean:
1311-
case Char:
1312-
return new ZeroExtendNode(value, 32);
1329+
/**
1330+
* Perform sign or zero extensions for subword types, and convert potentially unsafe 8 bit
1331+
* boolean values into 0 or 1. The caller is expected to add an resulting * nodes to the graph.
1332+
*/
1333+
public static ValueNode implicitUnsafePrimitiveLoadConvert(JavaKind kind, ValueNode value) {
1334+
if (kind == JavaKind.Boolean) {
1335+
return performBooleanCoercion(new ZeroExtendNode(value, 32));
13131336
}
1314-
return value;
1337+
return implicitPrimitiveLoadConvert(kind, value);
13151338
}
13161339

13171340
public ValueNode arrayImplicitStoreConvert(StructuredGraph graph,
@@ -1368,15 +1391,60 @@ protected ValueNode implicitStoreConvert(JavaKind kind, ValueNode value, boolean
13681391
return newCompressionNode(CompressionOp.Compress, value);
13691392
}
13701393

1371-
switch (kind) {
1372-
case Boolean:
1373-
case Byte:
1374-
return new NarrowNode(value, 8);
1375-
case Char:
1376-
case Short:
1377-
return new NarrowNode(value, 16);
1378-
}
1379-
return value;
1394+
return implicitPrimitiveStoreConvert(kind, value);
1395+
}
1396+
1397+
public static ValueNode implicitPrimitiveStoreConvert(JavaKind kind, ValueNode value) {
1398+
return switch (kind) {
1399+
case Boolean, Byte -> new NarrowNode(value, 8);
1400+
case Char, Short -> new NarrowNode(value, 16);
1401+
default -> value;
1402+
};
1403+
}
1404+
1405+
/**
1406+
* Simulate a primitive store.
1407+
*
1408+
* So for code like:
1409+
*
1410+
* <pre>
1411+
* static class Data {
1412+
* int value = 0xF0F0F0F0;
1413+
* }
1414+
*
1415+
* Data data = new Data();
1416+
* UNSAFE.putByte(data, FIELD_OFFSET, 0x0F);
1417+
* </pre>
1418+
*
1419+
* The field value of the data object is 0xF0F0F0F0 before the unsafe write operation and
1420+
* 0xF0F0F00F after the unsafe write operation. We are not allowed to touch the upper 3 bytes.
1421+
* To simulate the write operation we extract the appropriate bytes and combine them.
1422+
* <p>
1423+
* Example for a byte operation, currently stored value 0xF0F0F0F0 and value to store
1424+
* 0x0000000F:
1425+
*
1426+
* <pre>
1427+
* lowerBytesMask = 00000000 00000000 00000000 11111111
1428+
* upperBytesMask = 11111111 11111111 11111111 00000000
1429+
* currentStored = 11110000 11110000 11110000 11110000
1430+
* valueToStore = 00000000 00000000 00000000 00001111
1431+
* newValue = (currentStored & upperBytesMask) | (valueToStore & lowerBytesMask)
1432+
* = 11110000 11110000 11110000 00001111
1433+
* </pre>
1434+
*
1435+
*/
1436+
public static ValueNode simulatePrimitiveStore(JavaKind kind, ValueNode currentValue, ValueNode valueToStore) {
1437+
// compute the masks
1438+
int bitCount = kind.getByteCount() * 8;
1439+
int lowerBytesMask = (int) CodeUtil.mask(bitCount);
1440+
int upperBytesMask = ~lowerBytesMask;
1441+
1442+
// extract the upper bytes from the current entry
1443+
ValueNode upperBytes = AndNode.create(ConstantNode.forInt(upperBytesMask), currentValue, NodeView.DEFAULT);
1444+
// extract the lower bytes from the value
1445+
ValueNode lowerBytes = AndNode.create(ConstantNode.forInt(lowerBytesMask), valueToStore, NodeView.DEFAULT);
1446+
// combine both
1447+
return OrNode.create(upperBytes, lowerBytes, NodeView.DEFAULT);
13801448
}
13811449

13821450
protected abstract ValueNode createReadHub(StructuredGraph graph, ValueNode object, LoweringTool tool);

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/virtual/phases/ea/VirtualizerToolImpl.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050
import jdk.graal.compiler.nodes.virtual.VirtualInstanceNode;
5151
import jdk.graal.compiler.nodes.virtual.VirtualObjectNode;
5252
import jdk.graal.compiler.options.OptionValues;
53+
import jdk.graal.compiler.replacements.DefaultJavaLoweringProvider;
5354
import jdk.vm.ci.meta.Assumptions;
5455
import jdk.vm.ci.meta.JavaConstant;
5556
import jdk.vm.ci.meta.JavaKind;
@@ -175,6 +176,14 @@ public boolean setVirtualEntry(VirtualObjectNode virtual, int index, ValueNode v
175176

176177
if (canVirtualize) {
177178
getDebug().log(DebugContext.DETAILED_LEVEL, "virtualizing %s for entryKind %s and access kind %s", current, entryKind, accessKind);
179+
if (theAccessKind != null && entryKind != theAccessKind &&
180+
(theAccessKind != JavaKind.Int && theAccessKind.getStackKind() == JavaKind.Int)) {
181+
ValueNode entry = getEntry(virtual, index);
182+
// We can't just set the given value as the new entry but need to simulate a store
183+
// operation
184+
newValue = DefaultJavaLoweringProvider.simulatePrimitiveStore(accessKind, entry, newValue);
185+
ensureAdded(newValue);
186+
}
178187
state.setEntry(virtual.getObjectId(), index, newValue);
179188
if (entryKind == JavaKind.Int) {
180189
if (accessKind.needsTwoSlots()) {

0 commit comments

Comments
 (0)