Skip to content

Commit

Permalink
strengthen reference stores to give release/consume semantics (#45484)
Browse files Browse the repository at this point in the history 
Followup to #36507; see the discussion there.

Also slightly weakens non-atomic pointer modification, since we
generally don't need DRF swap guarantees at all (even monotonic), only
the atomic-release property. This would correspond to atomic-unordered
failure order in many cases, but the LLVM LangRef says that this case is
"uninteresting", and thus declares it is invalid.

n.b. this still does not cover embedded references inside inlined structs
  • Loading branch information
@vtjnash
vtjnash authored Jun 9, 2022
1 parent f4d13cf commit 46135df
Show file tree
Hide file tree
Showing 9 changed files with 28 additions and 22 deletions.
8 changes: 4 additions & 4 deletions src/array.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -617,7 +617,7 @@ JL_DLLEXPORT void jl_arrayset(jl_array_t *a JL_ROOTING_ARGUMENT, jl_value_t *rhs
arrayassign_safe(hasptr, jl_array_owner(a), &((char*)a->data)[i * a->elsize], rhs, a->elsize);
}
else {
jl_atomic_store_relaxed(((_Atomic(jl_value_t*)*)a->data) + i, rhs);
jl_atomic_store_release(((_Atomic(jl_value_t*)*)a->data) + i, rhs);
jl_gc_wb(jl_array_owner(a), rhs);
}
}
Expand All @@ -627,7 +627,7 @@ JL_DLLEXPORT void jl_arrayunset(jl_array_t *a, size_t i)
if (i >= jl_array_len(a))
jl_bounds_error_int((jl_value_t*)a, i + 1);
if (a->flags.ptrarray)
jl_atomic_store_relaxed(((_Atomic(jl_value_t*)*)a->data) + i, NULL);
jl_atomic_store_release(((_Atomic(jl_value_t*)*)a->data) + i, NULL);
else if (a->flags.hasptr) {
size_t elsize = a->elsize;
jl_assume(elsize >= sizeof(void*) && elsize % sizeof(void*) == 0);
Expand Down Expand Up @@ -1198,7 +1198,7 @@ static NOINLINE ssize_t jl_array_ptr_copy_forward(jl_value_t *owner,
_Atomic(void*) *dest_pa = (_Atomic(void*)*)dest_p;
for (ssize_t i = 0; i < n; i++) {
void *val = jl_atomic_load_relaxed(src_pa + i);
jl_atomic_store_relaxed(dest_pa + i, val);
jl_atomic_store_release(dest_pa + i, val);
// `val` is young or old-unmarked
if (val && !(jl_astaggedvalue(val)->bits.gc & GC_MARKED)) {
jl_gc_queue_root(owner);
Expand All @@ -1216,7 +1216,7 @@ static NOINLINE ssize_t jl_array_ptr_copy_backward(jl_value_t *owner,
_Atomic(void*) *dest_pa = (_Atomic(void*)*)dest_p;
for (ssize_t i = 0; i < n; i++) {
void *val = jl_atomic_load_relaxed(src_pa + n - i - 1);
jl_atomic_store_relaxed(dest_pa + n - i - 1, val);
jl_atomic_store_release(dest_pa + n - i - 1, val);
// `val` is young or old-unmarked
if (val && !(jl_astaggedvalue(val)->bits.gc & GC_MARKED)) {
jl_gc_queue_root(owner);
Expand Down
12 changes: 9 additions & 3 deletions src/cgutils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1759,6 +1759,8 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
if (issetfield || (Order == AtomicOrdering::NotAtomic && isswapfield)) {
if (isswapfield) {
auto *load = ctx.builder.CreateAlignedLoad(elty, ptr, Align(alignment));
if (isboxed)
load->setOrdering(AtomicOrdering::Unordered);
if (aliasscope)
load->setMetadata("noalias", aliasscope);
if (tbaa)
Expand All @@ -1767,7 +1769,7 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
instr = load;
}
StoreInst *store = ctx.builder.CreateAlignedStore(r, ptr, Align(alignment));
store->setOrdering(Order);
store->setOrdering(Order == AtomicOrdering::NotAtomic && isboxed ? AtomicOrdering::Release : Order);
if (aliasscope)
store->setMetadata("noalias", aliasscope);
if (tbaa)
Expand Down Expand Up @@ -1807,7 +1809,7 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
ctx.builder.CreateCondBr(SameType, BB, SkipBB);
ctx.builder.SetInsertPoint(SkipBB);
LoadInst *load = ctx.builder.CreateAlignedLoad(elty, ptr, Align(alignment));
load->setOrdering(FailOrder);
load->setOrdering(FailOrder == AtomicOrdering::NotAtomic && isboxed ? AtomicOrdering::Monotonic : FailOrder);
if (aliasscope)
load->setMetadata("noalias", aliasscope);
if (tbaa)
Expand Down Expand Up @@ -1838,7 +1840,7 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
}
else { // swap or modify
LoadInst *Current = ctx.builder.CreateAlignedLoad(elty, ptr, Align(alignment));
Current->setOrdering(Order == AtomicOrdering::NotAtomic ? Order : AtomicOrdering::Monotonic);
Current->setOrdering(Order == AtomicOrdering::NotAtomic && !isboxed ? Order : AtomicOrdering::Monotonic);
if (aliasscope)
Current->setMetadata("noalias", aliasscope);
if (tbaa)
Expand Down Expand Up @@ -1893,6 +1895,8 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
// modifyfield or replacefield
assert(elty == realelty && !intcast);
auto *load = ctx.builder.CreateAlignedLoad(elty, ptr, Align(alignment));
if (isboxed)
load->setOrdering(AtomicOrdering::Monotonic);
if (aliasscope)
load->setMetadata("noalias", aliasscope);
if (tbaa)
Expand Down Expand Up @@ -1921,6 +1925,8 @@ static jl_cgval_t typed_store(jl_codectx_t &ctx,
else {
if (Order == AtomicOrdering::Unordered)
Order = AtomicOrdering::Monotonic;
if (Order == AtomicOrdering::Monotonic && isboxed)
Order = AtomicOrdering::Release;
if (!isreplacefield)
FailOrder = AtomicOrdering::Monotonic;
else if (FailOrder == AtomicOrdering::Unordered)
Expand Down
12 changes: 6 additions & 6 deletions src/codegen.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2940,11 +2940,11 @@ static bool emit_f_opfield(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f,
else {
*ret = emit_setfield(ctx, uty, obj, idx, val, cmp, true,
(needlock || order <= jl_memory_order_notatomic)
? (isboxed ? AtomicOrdering::Unordered : AtomicOrdering::NotAtomic) // TODO: we should do this for anything with CountTrackedPointers(elty).count > 0
: get_llvm_atomic_order(order),
? AtomicOrdering::NotAtomic
: get_llvm_atomic_order(order),
(needlock || fail_order <= jl_memory_order_notatomic)
? (isboxed ? AtomicOrdering::Unordered : AtomicOrdering::NotAtomic) // TODO: we should do this for anything with CountTrackedPointers(elty).count > 0
: get_llvm_atomic_order(fail_order),
? AtomicOrdering::NotAtomic
: get_llvm_atomic_order(fail_order),
needlock, issetfield, isreplacefield, isswapfield, ismodifyfield,
modifyop, fname);
}
Expand Down Expand Up @@ -3279,8 +3279,8 @@ static bool emit_builtin_call(jl_codectx_t &ctx, jl_cgval_t *ret, jl_value_t *f,
ctx.aliasscope,
data_owner,
isboxed,
isboxed ? AtomicOrdering::Unordered : AtomicOrdering::NotAtomic, // TODO: we should do this for anything with CountTrackedPointers(elty).count > 0
isboxed ? AtomicOrdering::Unordered : AtomicOrdering::NotAtomic, // TODO: we should do this for anything with CountTrackedPointers(elty).count > 0
isboxed ? AtomicOrdering::Release : AtomicOrdering::NotAtomic, // TODO: we should do this for anything with CountTrackedPointers(elty).count > 0
/*FailOrder*/AtomicOrdering::NotAtomic, // TODO: we should do this for anything with CountTrackedPointers(elty).count > 0
0,
false,
true,
Expand Down
2 changes: 1 addition & 1 deletion src/datatype.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1491,7 +1491,7 @@ void set_nth_field(jl_datatype_t *st, jl_value_t *v, size_t i, jl_value_t *rhs,
return;
}
if (jl_field_isptr(st, i)) {
jl_atomic_store_relaxed((_Atomic(jl_value_t*)*)((char*)v + offs), rhs);
jl_atomic_store_release((_Atomic(jl_value_t*)*)((char*)v + offs), rhs);
jl_gc_wb(v, rhs);
}
else {
Expand Down
4 changes: 2 additions & 2 deletions src/debuginfo.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ void JITDebugInfoRegistry::libc_frames_t::libc_register_frame(const char *Entry)
auto libc_register_frame_ = jl_atomic_load_relaxed(&this->libc_register_frame_);
if (!libc_register_frame_) {
libc_register_frame_ = (void(*)(void*))dlsym(RTLD_NEXT, "__register_frame");
jl_atomic_store_relaxed(&this->libc_register_frame_, libc_register_frame_);
jl_atomic_store_release(&this->libc_register_frame_, libc_register_frame_);
}
assert(libc_register_frame_);
jl_profile_atomic([&]() {
Expand All @@ -550,7 +550,7 @@ void JITDebugInfoRegistry::libc_frames_t::libc_deregister_frame(const char *Entr
auto libc_deregister_frame_ = jl_atomic_load_relaxed(&this->libc_deregister_frame_);
if (!libc_deregister_frame_) {
libc_deregister_frame_ = (void(*)(void*))dlsym(RTLD_NEXT, "__deregister_frame");
jl_atomic_store_relaxed(&this->libc_deregister_frame_, libc_deregister_frame_);
jl_atomic_store_release(&this->libc_deregister_frame_, libc_deregister_frame_);
}
assert(libc_deregister_frame_);
jl_profile_atomic([&]() {
Expand Down
4 changes: 2 additions & 2 deletions src/iddict.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,9 @@ static inline int jl_table_assign_bp(jl_array_t **pa, jl_value_t *key, jl_value_
} while (iter <= maxprobe && index != orig);

if (empty_slot != -1) {
jl_atomic_store_relaxed(&tab[empty_slot], key);
jl_atomic_store_release(&tab[empty_slot], key);
jl_gc_wb(a, key);
jl_atomic_store_relaxed(&tab[empty_slot + 1], val);
jl_atomic_store_release(&tab[empty_slot + 1], val);
jl_gc_wb(a, val);
return 1;
}
Expand Down
2 changes: 1 addition & 1 deletion src/julia.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1001,7 +1001,7 @@ STATIC_INLINE jl_value_t *jl_array_ptr_set(
{
assert(((jl_array_t*)a)->flags.ptrarray);
assert(i < jl_array_len(a));
jl_atomic_store_relaxed(((_Atomic(jl_value_t*)*)(jl_array_data(a))) + i, (jl_value_t*)x);
jl_atomic_store_release(((_Atomic(jl_value_t*)*)(jl_array_data(a))) + i, (jl_value_t*)x);
if (x) {
if (((jl_array_t*)a)->flags.how == 3) {
a = jl_array_data_owner(a);
Expand Down
4 changes: 2 additions & 2 deletions src/julia_internal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,12 @@ static inline void memmove_refs(void **dstp, void *const *srcp, size_t n) JL_NOT
_Atomic(void*) *dstpa = (_Atomic(void*)*)dstp;
if (dstp < srcp || dstp > srcp + n) {
for (i = 0; i < n; i++) {
jl_atomic_store_relaxed(dstpa + i, jl_atomic_load_relaxed(srcpa + i));
jl_atomic_store_release(dstpa + i, jl_atomic_load_relaxed(srcpa + i));
}
}
else {
for (i = 0; i < n; i++) {
jl_atomic_store_relaxed(dstpa + n - i - 1, jl_atomic_load_relaxed(srcpa + n - i - 1));
jl_atomic_store_release(dstpa + n - i - 1, jl_atomic_load_relaxed(srcpa + n - i - 1));
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/module.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -830,7 +830,7 @@ JL_DLLEXPORT void jl_checked_assignment(jl_binding_t *b, jl_value_t *rhs)
jl_safe_printf("WARNING: redefinition of constant %s. This may fail, cause incorrect answers, or produce other errors.\n",
jl_symbol_name(b->name));
}
jl_atomic_store_relaxed(&b->value, rhs);
jl_atomic_store_release(&b->value, rhs);
jl_gc_wb_binding(b, rhs);
}

Expand Down

0 comments on commit 46135df

Please sign in to comment.