Skip to content

Commit 312c2b4

Browse files
tkrodrigueztkrodriguez
committed
[GR-68664] Disallow floating reads unless FloatingReadsPhase has been run
PullRequest: graal/21834
2 parents 7171a11 + b17a358 commit 312c2b4

File tree

56 files changed

+186
-209
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

56 files changed

+186
-209
lines changed

‎compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/TransplantLowLevelGraphTest.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Rec
9393
lateMacroInvoke.setTemplateProducer(new Supplier<SnippetTemplate>() {
9494
@Override
9595
public SnippetTemplate get() {
96-
Arguments args = new Arguments(transplantTestSnippets.producer, GuardsStage.AFTER_FSA, LoweringTool.StandardLoweringStage.LOW_TIER);
96+
Arguments args = new Arguments(transplantTestSnippets.producer, GuardsStage.AFTER_FSA, true, LoweringTool.StandardLoweringStage.LOW_TIER);
9797
// no args
9898
SnippetTemplate template = transplantTestSnippets.template(getProviders(), lateMacroInvoke, args);
9999
return template;
@@ -116,7 +116,7 @@ public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Rec
116116
lateMacroInvoke.setTemplateProducer(new Supplier<SnippetTemplate>() {
117117
@Override
118118
public SnippetTemplate get() {
119-
Arguments args = new Arguments(transplantTestSnippets.producerWithArgs, GuardsStage.AFTER_FSA, LoweringTool.StandardLoweringStage.LOW_TIER);
119+
Arguments args = new Arguments(transplantTestSnippets.producerWithArgs, GuardsStage.AFTER_FSA, true, LoweringTool.StandardLoweringStage.LOW_TIER);
120120
args.add("a", arg0);
121121
args.add("b", arg1);
122122
SnippetTemplate template = transplantTestSnippets.template(getProviders(), lateMacroInvoke, args);
@@ -140,7 +140,7 @@ public boolean apply(GraphBuilderContext b, ResolvedJavaMethod targetMethod, Rec
140140
lateMacroInvoke.setTemplateProducer(new Supplier<SnippetTemplate>() {
141141
@Override
142142
public SnippetTemplate get() {
143-
Arguments args = new Arguments(transplantTestSnippets.producerWithDeopt, GuardsStage.AFTER_FSA, LoweringTool.StandardLoweringStage.LOW_TIER);
143+
Arguments args = new Arguments(transplantTestSnippets.producerWithDeopt, GuardsStage.AFTER_FSA, true, LoweringTool.StandardLoweringStage.LOW_TIER);
144144
args.add("a", arg0);
145145
args.add("b", arg1);
146146
SnippetTemplate template = transplantTestSnippets.template(getProviders(), lateMacroInvoke, args);

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/core/amd64/AMD64NodeMatchRules.java‎

Lines changed: 0 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -390,25 +390,17 @@ public ComplexMatchResult testBitAndBranch(IfNode root, ValueNode value, Constan
390390
}
391391

392392
@MatchRule("(If (IntegerTest Read=access value))")
393-
@MatchRule("(If (IntegerTest FloatingRead=access value))")
394393
public ComplexMatchResult integerTestBranchMemory(IfNode root, LIRLowerableAccess access, ValueNode value) {
395394
return emitIntegerTestBranchMemory(root, value, access);
396395
}
397396

398397
@MatchRule("(If (IntegerEquals=compare value Read=access))")
399398
@MatchRule("(If (IntegerLessThan=compare value Read=access))")
400399
@MatchRule("(If (IntegerBelow=compare value Read=access))")
401-
@MatchRule("(If (IntegerEquals=compare value FloatingRead=access))")
402-
@MatchRule("(If (IntegerLessThan=compare value FloatingRead=access))")
403-
@MatchRule("(If (IntegerBelow=compare value FloatingRead=access))")
404400
@MatchRule("(If (FloatEquals=compare value Read=access))")
405-
@MatchRule("(If (FloatEquals=compare value FloatingRead=access))")
406401
@MatchRule("(If (FloatLessThan=compare value Read=access))")
407-
@MatchRule("(If (FloatLessThan=compare value FloatingRead=access))")
408402
@MatchRule("(If (PointerEquals=compare value Read=access))")
409-
@MatchRule("(If (PointerEquals=compare value FloatingRead=access))")
410403
@MatchRule("(If (ObjectEquals=compare value Read=access))")
411-
@MatchRule("(If (ObjectEquals=compare value FloatingRead=access))")
412404
public ComplexMatchResult ifCompareMemory(IfNode root, CompareNode compare, ValueNode value, LIRLowerableAccess access) {
413405
return emitCompareBranchMemory(root, compare, value, access);
414406
}
@@ -468,7 +460,6 @@ public ComplexMatchResult ifCompareLogicCas(IfNode root, CompareNode compare, Va
468460
return null;
469461
}
470462

471-
@MatchRule("(If (ObjectEquals=compare value FloatingRead=access))")
472463
public ComplexMatchResult ifLogicCas(IfNode root, CompareNode compare, ValueNode value, LIRLowerableAccess access) {
473464
return emitCompareBranchMemory(root, compare, value, access);
474465
}
@@ -516,7 +507,6 @@ private ComplexMatchResult binaryRead(AMD64Assembler.VexRVMOp op, OperandSize si
516507
}
517508

518509
@MatchRule("(Add value Read=access)")
519-
@MatchRule("(Add value FloatingRead=access)")
520510
public ComplexMatchResult addMemory(ValueNode value, LIRLowerableAccess access) {
521511
OperandSize size = getMemorySize(access);
522512
if (size.isXmmType()) {
@@ -531,7 +521,6 @@ public ComplexMatchResult addMemory(ValueNode value, LIRLowerableAccess access)
531521
}
532522

533523
@MatchRule("(Sub value Read=access)")
534-
@MatchRule("(Sub value FloatingRead=access)")
535524
public ComplexMatchResult subMemory(ValueNode value, LIRLowerableAccess access) {
536525
OperandSize size = getMemorySize(access);
537526
if (size.isXmmType()) {
@@ -546,7 +535,6 @@ public ComplexMatchResult subMemory(ValueNode value, LIRLowerableAccess access)
546535
}
547536

548537
@MatchRule("(Mul value Read=access)")
549-
@MatchRule("(Mul value FloatingRead=access)")
550538
public ComplexMatchResult mulMemory(ValueNode value, LIRLowerableAccess access) {
551539
OperandSize size = getMemorySize(access);
552540
if (size.isXmmType()) {
@@ -561,7 +549,6 @@ public ComplexMatchResult mulMemory(ValueNode value, LIRLowerableAccess access)
561549
}
562550

563551
@MatchRule("(And value Read=access)")
564-
@MatchRule("(And value FloatingRead=access)")
565552
public ComplexMatchResult andMemory(ValueNode value, LIRLowerableAccess access) {
566553
OperandSize size = getMemorySize(access);
567554
if (size.isXmmType()) {
@@ -572,7 +559,6 @@ public ComplexMatchResult andMemory(ValueNode value, LIRLowerableAccess access)
572559
}
573560

574561
@MatchRule("(Or value Read=access)")
575-
@MatchRule("(Or value FloatingRead=access)")
576562
public ComplexMatchResult orMemory(ValueNode value, LIRLowerableAccess access) {
577563
OperandSize size = getMemorySize(access);
578564
if (size.isXmmType()) {
@@ -583,7 +569,6 @@ public ComplexMatchResult orMemory(ValueNode value, LIRLowerableAccess access) {
583569
}
584570

585571
@MatchRule("(Xor value Read=access)")
586-
@MatchRule("(Xor value FloatingRead=access)")
587572
public ComplexMatchResult xorMemory(ValueNode value, LIRLowerableAccess access) {
588573
OperandSize size = getMemorySize(access);
589574
if (size.isXmmType()) {
@@ -660,20 +645,17 @@ public ComplexMatchResult writeNarrow(WriteNode root, NarrowNode narrow) {
660645
}
661646

662647
@MatchRule("(SignExtend Read=access)")
663-
@MatchRule("(SignExtend FloatingRead=access)")
664648
public ComplexMatchResult signExtend(SignExtendNode root, LIRLowerableAccess access) {
665649
return emitSignExtendMemory(access, root.getInputBits(), root.getResultBits(), null);
666650
}
667651

668652
@MatchRule("(ZeroExtend Read=access)")
669-
@MatchRule("(ZeroExtend FloatingRead=access)")
670653
public ComplexMatchResult zeroExtend(ZeroExtendNode root, LIRLowerableAccess access) {
671654
AMD64Kind memoryKind = getMemoryKind(access);
672655
return builder -> getArithmeticLIRGenerator().emitZeroExtendMemory(memoryKind, root.getResultBits(), (AMD64AddressValue) operand(access.getAddress()), getState(access));
673656
}
674657

675658
@MatchRule("(Narrow Read=access)")
676-
@MatchRule("(Narrow FloatingRead=access)")
677659
public ComplexMatchResult narrowRead(NarrowNode root, LIRLowerableAccess access) {
678660
return new ComplexMatchResult() {
679661
@Override
@@ -690,14 +672,12 @@ public Value evaluate(NodeLIRBuilder builder) {
690672
}
691673

692674
@MatchRule("(SignExtend (Narrow=narrow Read=access))")
693-
@MatchRule("(SignExtend (Narrow=narrow FloatingRead=access))")
694675
public ComplexMatchResult signExtendNarrowRead(SignExtendNode root, NarrowNode narrow, LIRLowerableAccess access) {
695676
LIRKind kind = getLIRGeneratorTool().getLIRKind(narrow.stamp(NodeView.DEFAULT));
696677
return emitSignExtendMemory(access, narrow.getResultBits(), root.getResultBits(), kind);
697678
}
698679

699680
@MatchRule("(FloatConvert Read=access)")
700-
@MatchRule("(FloatConvert FloatingRead=access)")
701681
public ComplexMatchResult floatConvert(FloatConvertNode root, LIRLowerableAccess access) {
702682
if (root.getFloatConvert().getCategory().equals(FloatConvertCategory.FloatingPointToInteger) && (root.inputCanBeNaN() || root.canOverflow())) {
703683
/* We need to fix up the result of the conversion, the input should be in a register. */
@@ -730,7 +710,6 @@ public ComplexMatchResult floatConvert(FloatConvertNode root, LIRLowerableAccess
730710
}
731711

732712
@MatchRule("(Reinterpret Read=access)")
733-
@MatchRule("(Reinterpret FloatingRead=access)")
734713
public ComplexMatchResult reinterpret(ReinterpretNode root, LIRLowerableAccess access) {
735714
return builder -> {
736715
LIRKind kind = getLIRGeneratorTool().getLIRKind(root.stamp(NodeView.DEFAULT));

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/graph/Node.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -825,7 +825,7 @@ protected void updatePredecessor(Node oldSuccessor, Node newSuccessor) {
825825
}
826826
}
827827

828-
void initialize(Graph newGraph) {
828+
final void initialize(Graph newGraph) {
829829
assertTrue(id == INITIAL_ID, "unexpected id: %d", id);
830830
this.graph = newGraph;
831831
newGraph.register(this);

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/amd64/AMD64X87MathSnippets.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ public void lower(UnaryMathIntrinsicNode mathIntrinsicNode, LoweringTool tool) {
107107
throw GraalError.shouldNotReachHere("Snippet not found for math intrinsic " + mathIntrinsicNode.getOperation().name()); // ExcludeFromJacocoGeneratedReport
108108
}
109109

110-
Arguments args = new Arguments(info, mathIntrinsicNode.graph().getGuardsStage(), tool.getLoweringStage());
110+
Arguments args = new Arguments(info, mathIntrinsicNode.graph(), tool.getLoweringStage());
111111
args.add("input", mathIntrinsicNode.getValue());
112112
template(tool, mathIntrinsicNode, args).instantiate(tool.getMetaAccess(), mathIntrinsicNode, DEFAULT_REPLACER, tool, args);
113113
mathIntrinsicNode.safeDelete();

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/meta/DefaultHotSpotLoweringProvider.java‎

Lines changed: 30 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -792,12 +792,27 @@ private void lowerKlassLayoutHelperNode(KlassLayoutHelperNode n, LoweringTool to
792792
}
793793
StructuredGraph graph = n.graph();
794794
assert !n.getHub().isConstant();
795-
AddressNode address = createOffsetAddress(graph, n.getHub(), runtime.getVMConfig().klassLayoutHelperOffset);
796-
ReadNode memoryRead = graph.add(new ReadNode(address, HotSpotReplacementsUtil.KLASS_LAYOUT_HELPER_LOCATION, null, n.stamp(NodeView.DEFAULT), null, BarrierType.NONE));
797-
graph.addAfterFixed(tool.lastFixedNode(), memoryRead);
795+
ValueNode memoryRead = createRead(graph, createOffsetAddress(graph, n.getHub(), runtime.getVMConfig().klassLayoutHelperOffset), HotSpotReplacementsUtil.KLASS_LAYOUT_HELPER_LOCATION,
796+
n.stamp(NodeView.DEFAULT), BarrierType.NONE, tool.lastFixedNode());
798797
n.replaceAtUsagesAndDelete(memoryRead);
799798
}
800799

800+
private static ValueNode createRead(StructuredGraph graph, AddressNode address, LocationIdentity location, Stamp stamp, BarrierType barrierType, FixedWithNextNode insertAfter) {
801+
return createRead(graph, address, location, stamp, null, barrierType, insertAfter);
802+
}
803+
804+
private static ValueNode createRead(StructuredGraph graph, AddressNode address, LocationIdentity location, Stamp stamp, GuardingNode guard, BarrierType barrierType,
805+
FixedWithNextNode insertAfter) {
806+
if (graph.getGraphState().allowsFloatingReads()) {
807+
return graph.unique(new FloatingReadNode(address, location, null, stamp, guard, barrierType));
808+
} else {
809+
ReadNode memoryRead = graph.add(new ReadNode(address, location, null, stamp, guard, barrierType));
810+
graph.addAfterFixed(insertAfter, memoryRead);
811+
assert memoryRead.canFloat() : "this path is only useable for floatable reads: " + memoryRead;
812+
return memoryRead;
813+
}
814+
}
815+
801816
private void lowerHubGetClassNode(HubGetClassNode n, LoweringTool tool) {
802817
if (tool.getLoweringStage() == LoweringTool.StandardLoweringStage.HIGH_TIER) {
803818
return;
@@ -809,13 +824,16 @@ private void lowerHubGetClassNode(HubGetClassNode n, LoweringTool tool) {
809824
assert !hub.isConstant();
810825
AddressNode mirrorAddress = createOffsetAddress(graph, hub, vmConfig.classMirrorOffset);
811826
Stamp loadStamp = n.stamp(NodeView.DEFAULT);
812-
FloatingReadNode read = graph.unique(
813-
new FloatingReadNode(mirrorAddress, HotSpotReplacementsUtil.CLASS_MIRROR_LOCATION, null, StampFactory.forKind(target.wordJavaKind),
814-
null, BarrierType.NONE));
827+
FixedWithNextNode insertAfter = tool.lastFixedNode();
828+
ValueNode read = createRead(graph, mirrorAddress, HotSpotReplacementsUtil.CLASS_MIRROR_LOCATION, StampFactory.forKind(target.wordJavaKind),
829+
BarrierType.NONE, insertAfter);
830+
if (read instanceof FixedWithNextNode fixed) {
831+
insertAfter = fixed;
832+
}
815833
// Read the Object from the OopHandle
816834
AddressNode address = createOffsetAddress(graph, read, 0);
817-
read = graph.unique(new FloatingReadNode(address, HotSpotReplacementsUtil.HOTSPOT_OOP_HANDLE_LOCATION, null, loadStamp, null,
818-
barrierSet.readBarrierType(HotSpotReplacementsUtil.HOTSPOT_OOP_HANDLE_LOCATION, address, loadStamp)));
835+
read = createRead(graph, address, HotSpotReplacementsUtil.HOTSPOT_OOP_HANDLE_LOCATION, loadStamp,
836+
barrierSet.readBarrierType(HotSpotReplacementsUtil.HOTSPOT_OOP_HANDLE_LOCATION, address, loadStamp), insertAfter);
819837
n.replaceAtUsagesAndDelete(read);
820838
}
821839

@@ -827,7 +845,7 @@ private void lowerClassGetHubNode(ClassGetHubNode n, LoweringTool tool) {
827845
StructuredGraph graph = n.graph();
828846
assert !n.getValue().isConstant();
829847
AddressNode address = createOffsetAddress(graph, n.getValue(), runtime.getVMConfig().klassOffset);
830-
FloatingReadNode read = graph.unique(new FloatingReadNode(address, HotSpotReplacementsUtil.CLASS_KLASS_LOCATION, null, n.stamp(NodeView.DEFAULT), null, BarrierType.NONE));
848+
ValueNode read = createRead(graph, address, HotSpotReplacementsUtil.CLASS_KLASS_LOCATION, n.stamp(NodeView.DEFAULT), BarrierType.NONE, tool.lastFixedNode());
831849
n.replaceAtUsagesAndDelete(read);
832850
}
833851

@@ -920,9 +938,7 @@ protected ValueNode createReadArrayComponentHub(StructuredGraph graph, ValueNode
920938
guard = AbstractBeginNode.prevBegin(anchor);
921939
}
922940
AddressNode address = createOffsetAddress(graph, arrayHub, runtime.getVMConfig().arrayClassElementOffset);
923-
ReadNode read = graph.add(new ReadNode(address, HotSpotReplacementsUtil.OBJ_ARRAY_KLASS_ELEMENT_KLASS_LOCATION, null, KlassPointerStamp.klassNonNull(), guard, BarrierType.NONE));
924-
graph.addAfterFixed(insertAfter, read);
925-
return read;
941+
return createRead(graph, address, HotSpotReplacementsUtil.OBJ_ARRAY_KLASS_ELEMENT_KLASS_LOCATION, KlassPointerStamp.klassNonNull(), guard, BarrierType.NONE, insertAfter);
926942
}
927943

928944
private void lowerLoadMethodNode(LoadMethodNode loadMethodNode) {
@@ -1157,22 +1173,17 @@ protected ValueNode createReadHub(StructuredGraph graph, ValueNode object, Lower
11571173
hubStamp = hubStamp.compressed(config.getKlassEncoding());
11581174
}
11591175

1160-
FixedWithNextNode effectiveInsertAfter = insertAfter;
1161-
11621176
if (config.useCompactObjectHeaders) {
11631177
AddressNode address = createOffsetAddress(graph, object, config.markOffset);
1164-
ReadNode memoryRead = graph.add(new ReadNode(address, COMPACT_HUB_LOCATION, null, StampFactory.forKind(JavaKind.Long), null, BarrierType.NONE));
1165-
graph.addAfterFixed(insertAfter, memoryRead);
1166-
effectiveInsertAfter = memoryRead;
1178+
ValueNode memoryRead = createRead(graph, address, COMPACT_HUB_LOCATION, StampFactory.forKind(JavaKind.Long), BarrierType.NONE, insertAfter);
11671179
ValueNode rawCompressedHubWordSize = graph.addOrUnique(UnsignedRightShiftNode.create(memoryRead, ConstantNode.forInt(config.markWordKlassShift, graph), NodeView.DEFAULT));
11681180
ValueNode rawCompressedHub = graph.addOrUnique(NarrowNode.create(rawCompressedHubWordSize, 32, NodeView.DEFAULT));
11691181
ValueNode compressedKlassPointer = graph.addOrUnique(PointerCastNode.create(hubStamp, rawCompressedHub));
11701182
return HotSpotCompressionNode.uncompress(graph, compressedKlassPointer, config.getKlassEncoding());
11711183
}
11721184
AddressNode address = createOffsetAddress(graph, object, config.hubOffset);
11731185
LocationIdentity hubLocation = config.useCompressedClassPointers ? COMPRESSED_HUB_LOCATION : HUB_LOCATION;
1174-
ReadNode memoryRead = graph.add(new ReadNode(address, hubLocation, null, hubStamp, null, BarrierType.NONE));
1175-
graph.addAfterFixed(effectiveInsertAfter, memoryRead);
1186+
ValueNode memoryRead = createRead(graph, address, hubLocation, hubStamp, BarrierType.NONE, insertAfter);
11761187
if (config.useCompressedClassPointers) {
11771188
return HotSpotCompressionNode.uncompress(graph, memoryRead, config.getKlassEncoding());
11781189
} else {

‎compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/replacements/AssertionSnippets.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -87,7 +87,7 @@ public Templates(OptionValues options, HotSpotProviders providers) {
8787

8888
public void lower(AssertionNode assertionNode, LoweringTool tool) {
8989
StructuredGraph graph = assertionNode.graph();
90-
Arguments args = new Arguments(graph.start() instanceof StubStartNode ? stubAssertion : assertion, graph.getGuardsStage(), tool.getLoweringStage());
90+
Arguments args = new Arguments(graph.start() instanceof StubStartNode ? stubAssertion : assertion, graph, tool.getLoweringStage());
9191
args.add("condition", assertionNode.condition());
9292
args.add("message",
9393
graph.unique(new ConstantNode(new CStringConstant("failed runtime assertion in snippet/stub: " + assertionNode.message() + " (" + graph.method() + ")"),

0 commit comments

Comments
 (0)