From 05f8ee19af4d0fd498f51b6024a2af2dae4fac8f Mon Sep 17 00:00:00 2001
From: Jameson Nash <vtjnash@gmail.com>
Date: Thu, 27 May 2021 12:03:50 -0400
Subject: [PATCH] move mutation to atomic! always

---
 base/expr.jl    | 110 +++++++++++++++++++++++++++---------------------
 test/atomics.jl |  14 +++---
 2 files changed, 69 insertions(+), 55 deletions(-)

diff --git a/base/expr.jl b/base/expr.jl
index f50cbc374df73..90367c3435307 100644
--- a/base/expr.jl
+++ b/base/expr.jl
@@ -461,14 +461,8 @@ julia> mutable struct Atomic{T}; @atomic x::T; end
 julia> a = Atomic(1)
 Atomic{Int64}(1)
 
-julia> @atomic :sequentially_consistent a.x = 2 # set field x of a, with sequential consistency
-2
-
 julia> @atomic a.x # fetch field x of a, with sequential consistency
-2
-
-julia> @atomic a.x += 1 # increment field x of a, with sequential consistency
-3
+1
 ```
 """
 macro atomic(ex)
@@ -483,47 +477,33 @@ macro atomic(order, ex)
 end
 function make_atomic(order, ex)
     @nospecialize
-    if ex isa Expr
-        if ex.head === :.
-            l, r = esc(ex.args[1]), esc(ex.args[2])
-            return :(getproperty($l, $r, $order))
-        elseif ex.head === :(=)
-            l, r = ex.args[1], ex.args[2]
-            if is_expr(l, :., 2)
-                ll, lr = esc(l.args[1]), esc(l.args[2])
-                return :(setproperty!($ll, $lr, $r, $order))
-            end
-        end
-        if length(ex.args) == 2
-            if ex.head === :(+=)
-                op = :+
-            elseif ex.head === :(-=)
-                op = :-
-            elseif @isdefined string
-                shead = string(ex.head)
-                if endswith(shead, '=')
-                    op = Symbol(shead[1:prevind(shead, end)])
-                end
-            end
-            if @isdefined(op)
-                l, r = ex.args[1], esc(ex.args[2])
-                is_expr(l, :.) || error("@atomic modify expression missing field access")
-                ll, lr, op = esc(l.args[1]), esc(l.args[2]), esc(op)
-                return :(modifyproperty!($ll, $lr, $op, $r, $order)[2])
-            end
-        end
+    if isexpr(ex, :., 2)
+        l, r = esc(ex.args[1]), esc(ex.args[2])
+        return :(getproperty($l, $r, $order))
     end
     error("could not parse @atomic expression $ex")
 end
 
 
 """
-    @atomic! a.b.x max new()
-    @atomic! a.b.x + new()
-    @atomic! max(a.b.x, new())
-    @atomic! :acquire_release max(a.b.x, new())
-    @atomic! :acquire_release a.b.x + new()
-    @atomic! :acquire_release a.b.x max new()
+    @atomic! a.b.x = new
+    @atomic! a.b.x += addend
+    @atomic! :acquire_release a.b.x = new
+    @atomic! :acquire_release a.b.x += addend
+
+Perform the store operation expressed on the right atomically and return the
+new value.
+
+With `=`, this operation translates to a `setproperty!(a.b, :x, new)` call.
+With any operator also, this operation translates to a `modifyproperty!(a.b,
+:x, +, addend)[2]` call.
+
+    @atomic! a.b.x max arg2
+    @atomic! a.b.x + arg2
+    @atomic! max(a.b.x, arg2)
+    @atomic! :acquire_release max(a.b.x, arg2)
+    @atomic! :acquire_release a.b.x + arg2
+    @atomic! :acquire_release a.b.x max arg2
 
 Perform the binary operation expressed on the right atomically. Store the
 result into the field in the first argument and return the values `(old, new)`.
@@ -538,14 +518,20 @@ julia> mutable struct Atomic{T}; @atomic x::T; end
 julia> a = Atomic(1)
 Atomic{Int64}(1)
 
+julia> @atomic! :sequentially_consistent a.x = 2 # set field x of a, with sequential consistency
+2
+
+julia> @atomic! a.x += 1 # increment field x of a, with sequential consistency
+3
+
 julia> @atomic! a.x + 1 # increment field x of a, with sequential consistency
-(1, 2)
+(3, 4)
 
 julia> @atomic a.x # fetch field x of a, with sequential consistency
-2
+4
 
 julia> @atomic! max(a.x, 10) # change field x of a to the max value, with sequential consistency
-(2, 10)
+(4, 10)
 
 julia> @atomic! a.x max 5 # again change field x of a to the max value, with sequential consistency
 (10, 10)
@@ -567,8 +553,36 @@ macro atomic!(ex)
 end
 function make_atomic!(order, ex)
     @nospecialize
-    isexpr(ex, :call, 3) || error("could not parse @atomic! modify expression $ex")
-    return make_atomic!(order, ex.args[2], ex.args[1], ex.args[3])
+    if ex isa Expr
+        if isexpr(ex, :call, 3)
+            return make_atomic!(order, ex.args[2], ex.args[1], ex.args[3])
+        elseif ex.head === :(=)
+            l, r = ex.args[1], ex.args[2]
+            if is_expr(l, :., 2)
+                ll, lr = esc(l.args[1]), esc(l.args[2])
+                return :(setproperty!($ll, $lr, $r, $order))
+            end
+        end
+        if length(ex.args) == 2
+            if ex.head === :(+=)
+                op = :+
+            elseif ex.head === :(-=)
+                op = :-
+            elseif @isdefined string
+                shead = string(ex.head)
+                if endswith(shead, '=')
+                    op = Symbol(shead[1:prevind(shead, end)])
+                end
+            end
+            if @isdefined(op)
+                l, r = ex.args[1], esc(ex.args[2])
+                is_expr(l, :.) || error("@atomic modify expression missing field access")
+                ll, lr, op = esc(l.args[1]), esc(l.args[2]), esc(op)
+                return :(modifyproperty!($ll, $lr, $op, $r, $order)[2])
+            end
+        end
+    end
+    error("could not parse @atomic! modify expression $ex")
 end
 function make_atomic!(order, a1, op, a2)
     @nospecialize
@@ -674,6 +688,6 @@ function make_atomic_replace!(success_order, fail_order, ex, old_new)
         return :(replaceproperty!($ll, $lr, $exp, $rep, $success_order, $fail_order))
     else
         old_new = esc(old_new)
-        return :(local old_new = $old_new::Pair; replaceproperty!($ll, $lr, old_new[1], old_new[2], $success_order, $fail_order))
+        return :(replaceproperty!($ll, $lr, $old_new::Pair..., $success_order, $fail_order))
     end
 end
diff --git a/test/atomics.jl b/test/atomics.jl
index 7927ebb0db26e..a4a44e10d446d 100644
--- a/test/atomics.jl
+++ b/test/atomics.jl
@@ -297,16 +297,16 @@ test_field_undef(ARefxy{UndefComplex{UndefComplex{Any}}})
 # test macroexpansions
 let a = ARefxy(1, -1)
     @test 1 === @atomic a.x
-    @test 2 === @atomic :sequentially_consistent a.x = 2
-    @test 3 === @atomic :monotonic a.x = 3
-    @test_throws ConcurrencyViolationError @atomic :not_atomic a.x = 2
+    @test 2 === @atomic! :sequentially_consistent a.x = 2
+    @test 3 === @atomic! :monotonic a.x = 3
+    @test_throws ConcurrencyViolationError @atomic! :not_atomic a.x = 2
     @test_throws ConcurrencyViolationError @atomic :not_atomic a.x
-    @test_throws ConcurrencyViolationError @atomic :not_atomic a.x += 1
+    @test_throws ConcurrencyViolationError @atomic! :not_atomic a.x += 1
 
     @test 3 === @atomic :monotonic a.x
-    @test 5 === @atomic a.x += 2
-    @test 4 === @atomic :monotonic a.x -= 1
-    @test 12 === @atomic :monotonic a.x *= 3
+    @test 5 === @atomic! a.x += 2
+    @test 4 === @atomic! :monotonic a.x -= 1
+    @test 12 === @atomic! :monotonic a.x *= 3
 
     @test 12 === @atomic a.x
     @test (12, 13) === @atomic! a.x + 1