Skip to content

Commit 6d5adbf

Browse files
committed
Review comments
1 parent 6311f52 commit 6d5adbf

File tree

6 files changed

+41
-31
lines changed

6 files changed

+41
-31
lines changed

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/aarch64/shenandoah/AArch64HotSpotShenandoahCardBarrierOp.java

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@
2424
*/
2525
package jdk.graal.compiler.hotspot.aarch64.shenandoah;
2626

27+
import static jdk.graal.compiler.lir.LIRInstruction.OperandFlag.COMPOSITE;
28+
import static jdk.graal.compiler.lir.LIRInstruction.OperandFlag.REG;
29+
import static jdk.vm.ci.aarch64.AArch64.zr;
30+
import static jdk.vm.ci.code.ValueUtil.asRegister;
31+
2732
import jdk.graal.compiler.asm.Label;
2833
import jdk.graal.compiler.asm.aarch64.AArch64Address;
2934
import jdk.graal.compiler.asm.aarch64.AArch64MacroAssembler;
@@ -38,11 +43,6 @@
3843
import jdk.vm.ci.code.Register;
3944
import jdk.vm.ci.meta.AllocatableValue;
4045

41-
import static jdk.graal.compiler.lir.LIRInstruction.OperandFlag.COMPOSITE;
42-
import static jdk.graal.compiler.lir.LIRInstruction.OperandFlag.REG;
43-
import static jdk.vm.ci.aarch64.AArch64.zr;
44-
import static jdk.vm.ci.code.ValueUtil.asRegister;
45-
4646
/**
4747
* AArch64 backend for the Shenandoah card barrier.
4848
*/
@@ -73,11 +73,11 @@ protected AArch64HotSpotShenandoahCardBarrierOp(GraalHotSpotVMConfig config, Hot
7373
sha1 = "1c3e544b6fdec2f4ca0f07b2a1d5261d55754cb9")
7474
// @formatter:on
7575
protected void emitCode(CompilationResultBuilder crb, AArch64MacroAssembler masm) {
76-
try (AArch64MacroAssembler.ScratchRegister tmp2 = masm.getScratchRegister();
77-
AArch64MacroAssembler.ScratchRegister tmp3 = masm.getScratchRegister()) {
76+
try (AArch64MacroAssembler.ScratchRegister scratch1 = masm.getScratchRegister();
77+
AArch64MacroAssembler.ScratchRegister scratch2 = masm.getScratchRegister()) {
7878
Register rtmp1 = asRegister(tmp);
79-
Register rtmp2 = tmp2.getRegister();
80-
Register rtmp3 = tmp3.getRegister();
79+
Register rscratch1 = scratch1.getRegister();
80+
Register rscratch2 = scratch2.getRegister();
8181
AArch64Address storeAddr = address.toAddress();
8282
Register rthread = providers.getRegisters().getThreadRegister();
8383

@@ -94,13 +94,13 @@ protected void emitCode(CompilationResultBuilder crb, AArch64MacroAssembler masm
9494

9595
AArch64Address currCTHolderAddr = AArch64Address.createImmediateAddress(64, AArch64Address.AddressingMode.IMMEDIATE_SIGNED_UNSCALED, rthread,
9696
HotSpotReplacementsUtil.shenandoahCardTableOffset(config));
97-
masm.ldr(64, rtmp2, currCTHolderAddr);
97+
masm.ldr(64, rscratch1, currCTHolderAddr);
9898

99-
AArch64Address cardAddr = AArch64Address.createRegisterOffsetAddress(8, rAddr, rtmp2, false);
99+
AArch64Address cardAddr = AArch64Address.createRegisterOffsetAddress(8, rAddr, rscratch1, false);
100100
if (HotSpotReplacementsUtil.useCondCardMark(config)) {
101101
Label alreadyDirty = new Label();
102-
masm.ldr(8, rtmp3, cardAddr);
103-
masm.cbz(8, rtmp3, alreadyDirty);
102+
masm.ldr(8, rscratch2, cardAddr);
103+
masm.cbz(8, rscratch2, alreadyDirty);
104104
masm.str(8, zr, cardAddr);
105105
masm.bind(alreadyDirty);
106106
} else {

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/hotspot/aarch64/shenandoah/AArch64HotSpotShenandoahCompareAndSwapOp.java

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@
2424
*/
2525
package jdk.graal.compiler.hotspot.aarch64.shenandoah;
2626

27+
import static jdk.vm.ci.aarch64.AArch64.zr;
28+
import static jdk.vm.ci.code.ValueUtil.asRegister;
29+
2730
import jdk.graal.compiler.asm.Label;
2831
import jdk.graal.compiler.asm.aarch64.AArch64Address;
2932
import jdk.graal.compiler.asm.aarch64.AArch64Assembler;
@@ -42,9 +45,6 @@
4245
import jdk.vm.ci.code.Register;
4346
import jdk.vm.ci.meta.AllocatableValue;
4447

45-
import static jdk.vm.ci.aarch64.AArch64.zr;
46-
import static jdk.vm.ci.code.ValueUtil.asRegister;
47-
4848
/**
4949
* Special Shenandoah CAS implementation that handles false negatives due to concurrent evacuation.
5050
* The service is more complex than a traditional CAS operation because the CAS operation is
@@ -81,6 +81,7 @@ public AArch64HotSpotShenandoahCompareAndSwapOp(GraalHotSpotVMConfig config, Hot
8181
sha1 = "553a2fb0d37f39016eda85331e8cd2421153cbfe")
8282
// @formatter:on
8383
public void emitCode(CompilationResultBuilder crb, AArch64MacroAssembler masm) {
84+
// The original code uses the name tmp2 for the expected value
8485
Register address = asRegister(addressValue);
8586
Register result = asRegister(resultValue);
8687
Register expected = asRegister(expectedValue);
@@ -92,6 +93,9 @@ public void emitCode(CompilationResultBuilder crb, AArch64MacroAssembler masm) {
9293
GraalError.guarantee(accessKind == AArch64Kind.QWORD || accessKind == AArch64Kind.DWORD, "must be 64 or 32 bit access");
9394
int size = (accessKind == AArch64Kind.QWORD) ? 64 : 32;
9495

96+
// The control flow below is slightly different than the original HotSpot code with the step
97+
// 4 case move into the out of line path.
98+
9599
// Step 1. Fast-path.
96100
//
97101
// Try to CAS with given arguments. If successful, then we are done.
@@ -101,7 +105,7 @@ public void emitCode(CompilationResultBuilder crb, AArch64MacroAssembler masm) {
101105

102106
// If expected equals null but result does not equal null, the
103107
// step2 branches to done to report failure of CAS. If both
104-
// expected and tmp2 equal null, the following branches to done to
108+
// expected and result equal null, the following branches to done to
105109
// report success of CAS. There's no need for a special test of
106110
// expected equal to null.
107111

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/GraphState.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ public final class GraphState {
6262
StageFlag.GUARD_LOWERING,
6363
StageFlag.MID_TIER_LOWERING,
6464
StageFlag.FSA,
65-
StageFlag.BARRIER_ADDITION);
65+
StageFlag.MID_TIER_BARRIER_ADDITION);
6666
private static final EnumSet<StageFlag> LOW_TIER_MANDATORY_STAGES = EnumSet.of(
6767
StageFlag.LOW_TIER_LOWERING,
6868
StageFlag.EXPAND_LOGIC,
@@ -78,7 +78,7 @@ public final class GraphState {
7878
StageFlag.MID_TIER_LOWERING,
7979
StageFlag.FSA,
8080
StageFlag.NODE_VECTORIZATION,
81-
StageFlag.BARRIER_ADDITION);
81+
StageFlag.MID_TIER_BARRIER_ADDITION);
8282

8383
/**
8484
* This set of {@link StageFlag}s represents the stages a {@link StructuredGraph} initially
@@ -638,7 +638,7 @@ public enum StageFlag {
638638
NODE_VECTORIZATION,
639639
VECTOR_MATERIALIZATION,
640640
OPTIMISTIC_GUARDS,
641-
BARRIER_ADDITION,
641+
MID_TIER_BARRIER_ADDITION,
642642
BARRIER_ELIMINATION,
643643
/* Stages applied by low tier. */
644644
LOW_TIER_LOWERING,

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/gc/BarrierSet.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,11 +25,11 @@
2525
*/
2626
package jdk.graal.compiler.nodes.gc;
2727

28-
import jdk.graal.compiler.nodes.GraphState;
2928
import org.graalvm.word.LocationIdentity;
3029

3130
import jdk.graal.compiler.core.common.memory.BarrierType;
3231
import jdk.graal.compiler.core.common.type.Stamp;
32+
import jdk.graal.compiler.nodes.GraphState;
3333
import jdk.graal.compiler.nodes.StructuredGraph;
3434
import jdk.graal.compiler.nodes.ValueNode;
3535
import jdk.graal.compiler.nodes.extended.RawStoreNode;
@@ -93,6 +93,6 @@ default void verifyBarriers(StructuredGraph graph) {
9393
default boolean shouldAddBarriersInStage(GraphState.StageFlag stage) {
9494
// Most barrier sets should be added in mid-tier, some might also
9595
// wish to add in low-tier (e.g. Shenandoah GC).
96-
return stage == GraphState.StageFlag.BARRIER_ADDITION;
96+
return stage == GraphState.StageFlag.MID_TIER_BARRIER_ADDITION;
9797
}
9898
}

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/gc/shenandoah/ShenandoahBarrierSet.java

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,33 +26,34 @@
2626

2727
import static jdk.graal.compiler.nodes.NamedLocationIdentity.OFF_HEAP_LOCATION;
2828

29-
import jdk.graal.compiler.core.common.type.AbstractObjectStamp;
30-
import jdk.graal.compiler.nodes.GraphState;
31-
import jdk.graal.compiler.nodes.extended.ArrayRangeWrite;
32-
import jdk.graal.compiler.nodes.gc.BarrierSet;
33-
import jdk.graal.compiler.nodes.java.ValueCompareAndSwapNode;
34-
import jdk.graal.compiler.nodes.spi.CoreProviders;
35-
import jdk.graal.compiler.nodes.type.NarrowOopStamp;
3629
import org.graalvm.word.LocationIdentity;
3730

3831
import jdk.graal.compiler.core.common.memory.BarrierType;
32+
import jdk.graal.compiler.core.common.type.AbstractObjectStamp;
3933
import jdk.graal.compiler.core.common.type.Stamp;
4034
import jdk.graal.compiler.debug.GraalError;
35+
import jdk.graal.compiler.graph.Graph;
4136
import jdk.graal.compiler.graph.Node;
4237
import jdk.graal.compiler.nodes.FieldLocationIdentity;
38+
import jdk.graal.compiler.nodes.GraphState;
4339
import jdk.graal.compiler.nodes.NamedLocationIdentity;
4440
import jdk.graal.compiler.nodes.NodeView;
4541
import jdk.graal.compiler.nodes.StructuredGraph;
4642
import jdk.graal.compiler.nodes.ValueNode;
43+
import jdk.graal.compiler.nodes.extended.ArrayRangeWrite;
4744
import jdk.graal.compiler.nodes.extended.RawStoreNode;
45+
import jdk.graal.compiler.nodes.gc.BarrierSet;
4846
import jdk.graal.compiler.nodes.java.AbstractCompareAndSwapNode;
4947
import jdk.graal.compiler.nodes.java.LoweredAtomicReadAndWriteNode;
48+
import jdk.graal.compiler.nodes.java.ValueCompareAndSwapNode;
5049
import jdk.graal.compiler.nodes.memory.AddressableMemoryAccess;
5150
import jdk.graal.compiler.nodes.memory.FixedAccessNode;
5251
import jdk.graal.compiler.nodes.memory.LIRLowerableAccess;
5352
import jdk.graal.compiler.nodes.memory.ReadNode;
5453
import jdk.graal.compiler.nodes.memory.WriteNode;
5554
import jdk.graal.compiler.nodes.memory.address.AddressNode;
55+
import jdk.graal.compiler.nodes.spi.CoreProviders;
56+
import jdk.graal.compiler.nodes.type.NarrowOopStamp;
5657
import jdk.graal.compiler.nodes.type.StampTool;
5758
import jdk.vm.ci.meta.JavaKind;
5859
import jdk.vm.ci.meta.ResolvedJavaField;
@@ -242,7 +243,11 @@ private void addLoadReferenceBarrier(FixedAccessNode node, AddressNode address,
242243
GraalError.guarantee(node != null, "input value must not be null");
243244
StructuredGraph graph = node.graph();
244245
boolean narrow = node.stamp(NodeView.DEFAULT) instanceof NarrowOopStamp;
246+
Graph.Mark beforeUncompression = graph.getMark();
245247
ValueNode uncompressed = maybeUncompressReference(node, narrow);
248+
if (uncompressed != node) {
249+
GraalError.guarantee(graph.isNew(beforeUncompression, uncompressed), "we must not reuse an existing uncompress node");
250+
}
246251
ShenandoahLoadRefBarrierNode lrb = graph.add(new ShenandoahLoadRefBarrierNode(uncompressed, address, barrierType, narrow));
247252
ValueNode compValue = maybeCompressReference(lrb, narrow);
248253
ValueNode newUsage = uncompressed != node ? uncompressed : lrb;

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/WriteBarrierAdditionPhase.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,18 +40,19 @@ public class WriteBarrierAdditionPhase extends BasePhase<CoreProviders> {
4040
private final StageFlag stage;
4141

4242
public WriteBarrierAdditionPhase() {
43-
this(StageFlag.BARRIER_ADDITION);
43+
this(StageFlag.MID_TIER_BARRIER_ADDITION);
4444
}
4545

4646
public WriteBarrierAdditionPhase(StageFlag stage) {
47+
assert stage == StageFlag.LOW_TIER_BARRIER_ADDITION || stage == StageFlag.MID_TIER_BARRIER_ADDITION : stage;
4748
this.stage = stage;
4849
}
4950

5051
@Override
5152
public Optional<NotApplicable> notApplicableTo(GraphState graphState) {
5253
return NotApplicable.ifAny(
5354
NotApplicable.ifApplied(this, stage, graphState),
54-
NotApplicable.unlessRunAfter(this, stage == StageFlag.BARRIER_ADDITION ? StageFlag.MID_TIER_LOWERING : StageFlag.LOW_TIER_LOWERING, graphState),
55+
NotApplicable.unlessRunAfter(this, stage == StageFlag.MID_TIER_BARRIER_ADDITION ? StageFlag.MID_TIER_LOWERING : StageFlag.LOW_TIER_LOWERING, graphState),
5556
NotApplicable.unlessRunAfter(this, StageFlag.FSA, graphState));
5657
}
5758

0 commit comments

Comments
 (0)