review / fixed orderings

JuliaLang · Apr 2, 2021 · 298c8f5 · 298c8f5
1 parent 51c9af9
commit 298c8f5
Show file tree

Hide file tree

Showing 3 changed files with 9 additions and 17 deletions.
diff --git a/src/builtins.c b/src/builtins.c
@@ -925,11 +925,7 @@ JL_CALLABLE(jl_f_swapfield)
     if (isatomic == (order == jl_memory_order_notatomic))
         jl_atomic_error(isatomic ? "swapfield!: atomic field cannot be written non-atomically"
                                  : "swapfield!: non-atomic field cannot be written atomically");
-    if (order >= jl_memory_order_acq_rel || order == jl_memory_order_release)
-        jl_fence(); // `st->[idx]` will have at least relaxed ordering
-    v = swap_nth_field(st, v, idx, args[2], isatomic);
-    if (order >= jl_memory_order_acq_rel || order == jl_memory_order_acquire)
-        jl_fence(); // `v` already had at least consume ordering
+    v = swap_nth_field(st, v, idx, args[2], isatomic); // always seq_cst, if isatomic needed at all
     return v;
 }
 
@@ -948,7 +944,7 @@ JL_CALLABLE(jl_f_modifyfield)
     if (isatomic == (order == jl_memory_order_notatomic))
         jl_atomic_error(isatomic ? "modifyfield!: atomic field cannot be written non-atomically"
                                  : "modifyfield!: non-atomic field cannot be written atomically");
-    v = modify_nth_field(st, v, idx, args[2], args[3], isatomic);
+    v = modify_nth_field(st, v, idx, args[2], args[3], isatomic); // always seq_cst, if isatomic needed at all
     return v;
 }
 
@@ -965,7 +961,7 @@ JL_CALLABLE(jl_f_cmpswapfield)
         JL_TYPECHK(cmpswapfield!, symbol, args[5]);
         failure_order = jl_get_atomic_order_checked((jl_sym_t*)args[5], 1, 0);
     }
-    // TODO: filter invalid orderings
+    // TODO: filter more invalid ordering combinations
     jl_value_t *v = args[0];
     jl_datatype_t *st = (jl_datatype_t*)jl_typeof(v);
     size_t idx = get_checked_fieldindex("cmpswapfield!", st, v, args[1], 1);
@@ -975,7 +971,7 @@ JL_CALLABLE(jl_f_cmpswapfield)
                                  : "cmpswapfield!: non-atomic field cannot be written atomically");
     if (failure_order > success_order)
         jl_atomic_error("cmpswapfield!: invalid atomic ordering");
-    v = cmpswap_nth_field(st, v, idx, args[2], args[3], isatomic);
+    v = cmpswap_nth_field(st, v, idx, args[2], args[3], isatomic); // always seq_cst, if isatomic needed at all
     return v;
 }
 

diff --git a/src/datatype.c b/src/datatype.c
@@ -1523,6 +1523,8 @@ jl_value_t *modify_nth_field(jl_datatype_t *st, jl_value_t *v, size_t i, jl_valu
     size_t offs = jl_field_offset(st, i);
     jl_value_t *ty = jl_field_type_concrete(st, i);
     jl_value_t *r = jl_get_nth_field_checked(v, i);
+    if (isatomic && jl_field_isptr(st, i))
+        jl_fence(); // load was previously only relaxed
     jl_value_t **args;
     JL_GC_PUSHARGS(args, 2);
     args[0] = r;
@@ -1532,8 +1534,6 @@ jl_value_t *modify_nth_field(jl_datatype_t *st, jl_value_t *v, size_t i, jl_valu
         args[1] = y;
         if (!jl_isa(y, ty))
             jl_type_error("modifyfield!", ty, y);
-        // TODO: if (order >= jl_memory_order_acq_rel || order == jl_memory_order_release)
-        // TODO:     jl_fence(); // `st->[idx]` will have at least relaxed ordering
         if (jl_field_isptr(st, i)) {
             jl_value_t **p = (jl_value_t**)((char*)v + offs);
             if (isatomic ? jl_atomic_cmpswap(p, &r, y) : jl_atomic_cmpswap_relaxed(p, &r, y))
@@ -1597,8 +1597,6 @@ jl_value_t *modify_nth_field(jl_datatype_t *st, jl_value_t *v, size_t i, jl_valu
         jl_gc_safepoint();
     }
     JL_GC_POP();
-    //if (order >= jl_memory_order_acq_rel || order == jl_memory_order_acquire)
-    //    jl_fence(); // `v` already had at least consume ordering
     return r;
 }
 
@@ -1609,8 +1607,6 @@ jl_value_t *cmpswap_nth_field(jl_datatype_t *st, jl_value_t *v, size_t i, jl_val
         jl_type_error("cmpswapfield!", ty, rhs);
     size_t offs = jl_field_offset(st, i);
     jl_value_t *r = expected;
-    // TODO: if (order >= jl_memory_order_acq_rel || order == jl_memory_order_release)
-    // TODO:     jl_fence(); // `st->[idx]` will have at least relaxed ordering
     if (jl_field_isptr(st, i)) {
         jl_value_t **p = (jl_value_t**)((char*)v + offs);
         int success;
@@ -1620,7 +1616,7 @@ jl_value_t *cmpswap_nth_field(jl_datatype_t *st, jl_value_t *v, size_t i, jl_val
                 jl_gc_wb(v, rhs);
             if (__unlikely(r == NULL))
                 jl_throw(jl_undefref_exception);
-            if (success || !jl_egal(r, expected)) // TODO: may need barrier before jl_egal
+            if (success || !jl_egal(r, expected))
                 break;
         }
         jl_value_t **args;
@@ -1704,8 +1700,6 @@ jl_value_t *cmpswap_nth_field(jl_datatype_t *st, jl_value_t *v, size_t i, jl_val
         if (__unlikely(r == NULL))
             jl_throw(jl_undefref_exception);
     }
-    //if (order >= jl_memory_order_acq_rel || order == jl_memory_order_acquire)
-    //    jl_fence(); // `v` already had at least consume ordering
     return r;
 }
 

diff --git a/src/runtime_intrinsics.c b/src/runtime_intrinsics.c
@@ -144,6 +144,8 @@ JL_DLLEXPORT jl_value_t *jl_atomic_pointerswap(jl_value_t *p, jl_value_t *x, jl_
 
 JL_DLLEXPORT jl_value_t *jl_atomic_pointermodify(jl_value_t *p, jl_value_t *f, jl_value_t *x, jl_value_t *order_sym)
 {
+    // n.b. we use seq_cst always here, but need to verify the order sym
+    // against the weaker load-only that happens first
     if (order_sym == (jl_value_t*)acquire_release_sym)
         order_sym = (jl_value_t*)acquire_sym;
     jl_value_t *expected = jl_atomic_pointerref(p, order_sym);