Skip to content

Commit 3087e14

Browse files
TheRealMDoerrTheRealMDoerr
committed
8320807: [PPC64][ZGC] C1 generates wrong code for atomics
Reviewed-by: lucy, rrich
1 parent 54957ac commit 3087e14

File tree

4 files changed

+51
-79
lines changed

4 files changed

+51
-79
lines changed

‎src/hotspot/cpu/ppc/c1_LIRAssembler_ppc.cpp‎

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2600,6 +2600,13 @@ void LIR_Assembler::emit_compare_and_swap(LIR_OpCompareAndSwap* op) {
26002600
Unimplemented();
26012601
}
26022602

2603+
// There might be a volatile load before this Unsafe CAS.
2604+
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
2605+
__ sync();
2606+
} else {
2607+
__ lwsync();
2608+
}
2609+
26032610
if (is_64bit) {
26042611
__ cmpxchgd(BOOL_RESULT, /*current_value=*/R0, cmp_value, new_value, addr,
26052612
MacroAssembler::MemBarNone,
@@ -2961,9 +2968,24 @@ void LIR_Assembler::atomic_op(LIR_Code code, LIR_Opr src, LIR_Opr data, LIR_Opr
29612968
assert(addr->disp() == 0 && addr->index()->is_illegal(), "use leal!");
29622969
const Register Rptr = addr->base()->as_pointer_register(),
29632970
Rtmp = tmp->as_register();
2964-
Register Rco = noreg;
2965-
if (UseCompressedOops && data->is_oop()) {
2966-
Rco = __ encode_heap_oop(Rtmp, data->as_register());
2971+
Register Robj = noreg;
2972+
if (data->is_oop()) {
2973+
if (UseCompressedOops) {
2974+
Robj = __ encode_heap_oop(Rtmp, data->as_register());
2975+
} else {
2976+
Robj = data->as_register();
2977+
if (Robj == dest->as_register()) { // May happen with ZGC.
2978+
__ mr(Rtmp, Robj);
2979+
Robj = Rtmp;
2980+
}
2981+
}
2982+
}
2983+
2984+
// There might be a volatile load before this Unsafe OP.
2985+
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
2986+
__ sync();
2987+
} else {
2988+
__ lwsync();
29672989
}
29682990

29692991
Label Lretry;
@@ -2983,18 +3005,11 @@ void LIR_Assembler::atomic_op(LIR_Code code, LIR_Opr src, LIR_Opr data, LIR_Opr
29833005
} else if (data->is_oop()) {
29843006
assert(code == lir_xchg, "xadd for oops");
29853007
const Register Rold = dest->as_register();
3008+
assert_different_registers(Rptr, Rold, Robj);
29863009
if (UseCompressedOops) {
2987-
assert_different_registers(Rptr, Rold, Rco);
29883010
__ lwarx(Rold, Rptr, MacroAssembler::cmpxchgx_hint_atomic_update());
2989-
__ stwcx_(Rco, Rptr);
3011+
__ stwcx_(Robj, Rptr);
29903012
} else {
2991-
Register Robj = data->as_register();
2992-
assert_different_registers(Rptr, Rold, Rtmp);
2993-
assert_different_registers(Rptr, Robj, Rtmp);
2994-
if (Robj == Rold) { // May happen with ZGC.
2995-
__ mr(Rtmp, Robj);
2996-
Robj = Rtmp;
2997-
}
29983013
__ ldarx(Rold, Rptr, MacroAssembler::cmpxchgx_hint_atomic_update());
29993014
__ stdcx_(Robj, Rptr);
30003015
}
@@ -3022,6 +3037,12 @@ void LIR_Assembler::atomic_op(LIR_Code code, LIR_Opr src, LIR_Opr data, LIR_Opr
30223037
if (UseCompressedOops && data->is_oop()) {
30233038
__ decode_heap_oop(dest->as_register());
30243039
}
3040+
3041+
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
3042+
__ isync();
3043+
} else {
3044+
__ sync();
3045+
}
30253046
}
30263047

30273048

‎src/hotspot/cpu/ppc/c1_LIRGenerator_ppc.cpp‎

Lines changed: 0 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -639,13 +639,6 @@ LIR_Opr LIRGenerator::atomic_cmpxchg(BasicType type, LIR_Opr addr, LIRItem& cmp_
639639
cmp_value.load_item();
640640
new_value.load_item();
641641

642-
// Volatile load may be followed by Unsafe CAS.
643-
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
644-
__ membar();
645-
} else {
646-
__ membar_release();
647-
}
648-
649642
if (is_reference_type(type)) {
650643
if (UseCompressedOops) {
651644
t1 = new_register(T_OBJECT);
@@ -670,21 +663,7 @@ LIR_Opr LIRGenerator::atomic_xchg(BasicType type, LIR_Opr addr, LIRItem& value)
670663
LIR_Opr tmp = FrameMap::R0_opr;
671664

672665
value.load_item();
673-
674-
// Volatile load may be followed by Unsafe CAS.
675-
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
676-
__ membar();
677-
} else {
678-
__ membar_release();
679-
}
680-
681666
__ xchg(addr, value.result(), result, tmp);
682-
683-
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
684-
__ membar_acquire();
685-
} else {
686-
__ membar();
687-
}
688667
return result;
689668
}
690669

@@ -694,21 +673,7 @@ LIR_Opr LIRGenerator::atomic_add(BasicType type, LIR_Opr addr, LIRItem& value) {
694673
LIR_Opr tmp = FrameMap::R0_opr;
695674

696675
value.load_item();
697-
698-
// Volatile load may be followed by Unsafe CAS.
699-
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
700-
__ membar(); // To be safe. Unsafe semantics are unclear.
701-
} else {
702-
__ membar_release();
703-
}
704-
705676
__ xadd(addr, value.result(), result, tmp);
706-
707-
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
708-
__ membar_acquire();
709-
} else {
710-
__ membar();
711-
}
712677
return result;
713678
}
714679

‎src/hotspot/cpu/ppc/gc/shenandoah/c1/shenandoahBarrierSetC1_ppc.cpp‎

Lines changed: 15 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/*
2-
* Copyright (c) 2018, 2021, Red Hat, Inc. All rights reserved.
3-
* Copyright (c) 2012, 2021 SAP SE. All rights reserved.
2+
* Copyright (c) 2018, 2023, Red Hat, Inc. All rights reserved.
3+
* Copyright (c) 2012, 2023 SAP SE. All rights reserved.
44
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
55
*
66
* This code is free software; you can redistribute it and/or modify it
@@ -53,8 +53,13 @@ void LIR_OpShenandoahCompareAndSwap::emit_code(LIR_Assembler *masm) {
5353
__ encode_heap_oop(new_val, new_val);
5454
}
5555

56-
// Due to the memory barriers emitted in ShenandoahBarrierSetC1::atomic_cmpxchg_at_resolved,
57-
// there is no need to specify stronger memory semantics.
56+
// There might be a volatile load before this Unsafe CAS.
57+
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
58+
__ sync();
59+
} else {
60+
__ lwsync();
61+
}
62+
5863
ShenandoahBarrierSet::assembler()->cmpxchg_oop(masm->masm(), addr, cmp_val, new_val, tmp1, tmp2,
5964
false, result);
6065

@@ -63,6 +68,12 @@ void LIR_OpShenandoahCompareAndSwap::emit_code(LIR_Assembler *masm) {
6368
__ decode_heap_oop(new_val);
6469
}
6570

71+
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
72+
__ isync();
73+
} else {
74+
__ sync();
75+
}
76+
6677
__ block_comment("} LIR_OpShenandoahCompareAndSwap (shenandaohgc)");
6778
}
6879

@@ -80,14 +91,6 @@ LIR_Opr ShenandoahBarrierSetC1::atomic_cmpxchg_at_resolved(LIRAccess &access, LI
8091
if (access.is_oop()) {
8192
LIRGenerator* gen = access.gen();
8293

83-
if (ShenandoahCASBarrier) {
84-
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
85-
__ membar();
86-
} else {
87-
__ membar_release();
88-
}
89-
}
90-
9194
if (ShenandoahSATBBarrier) {
9295
pre_barrier(gen, access.access_emit_info(), access.decorators(), access.resolved_addr(),
9396
LIR_OprFact::illegalOpr);
@@ -104,12 +107,6 @@ LIR_Opr ShenandoahBarrierSetC1::atomic_cmpxchg_at_resolved(LIRAccess &access, LI
104107

105108
__ append(new LIR_OpShenandoahCompareAndSwap(addr, cmp_value.result(), new_value.result(), t1, t2, result));
106109

107-
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
108-
__ membar_acquire();
109-
} else {
110-
__ membar();
111-
}
112-
113110
return result;
114111
}
115112
}
@@ -125,12 +122,6 @@ LIR_Opr ShenandoahBarrierSetC1::atomic_xchg_at_resolved(LIRAccess &access, LIRIt
125122
value.load_item();
126123
LIR_Opr value_opr = value.result();
127124

128-
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
129-
__ membar();
130-
} else {
131-
__ membar_release();
132-
}
133-
134125
if (access.is_oop()) {
135126
value_opr = iu_barrier(access.gen(), value_opr, access.access_emit_info(), access.decorators());
136127
}
@@ -152,11 +143,5 @@ LIR_Opr ShenandoahBarrierSetC1::atomic_xchg_at_resolved(LIRAccess &access, LIRIt
152143
}
153144
}
154145

155-
if (support_IRIW_for_not_multiple_copy_atomic_cpu) {
156-
__ membar_acquire();
157-
} else {
158-
__ membar();
159-
}
160-
161146
return result;
162147
}

‎src/hotspot/cpu/ppc/gc/z/zBarrierSetAssembler_ppc.cpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -340,11 +340,12 @@ void ZBarrierSetAssembler::store_barrier_medium(MacroAssembler* masm,
340340
}
341341
__ ld(R0, in_bytes(ZThreadLocalData::store_good_mask_offset()), R16_thread);
342342
__ cmpxchgd(CCR0, tmp, (intptr_t)0, R0, ref_base,
343-
MacroAssembler::MemBarNone, MacroAssembler::cmpxchgx_hint_atomic_update());
343+
MacroAssembler::MemBarNone, MacroAssembler::cmpxchgx_hint_atomic_update(),
344+
noreg, need_restore ? nullptr : &slow_path);
344345
if (need_restore) {
345346
__ subf(ref_base, ind_or_offs, ref_base);
347+
__ bne(CCR0, slow_path);
346348
}
347-
__ bne(CCR0, slow_path);
348349
} else {
349350
// A non-atomic relocatable object won't get to the medium fast path due to a
350351
// raw null in the young generation. We only get here because the field is bad.

0 commit comments

Comments
 (0)