Re: [AArch64][PATCH 1/5] Use atomic instructions for swap and fetch-update operations.

[James Greenhalgh <james dot greenhalgh at arm dot com>]

On Thu, Sep 17, 2015 at 05:37:55PM +0100, Matthew Wahab wrote:
> Hello,
> 
> ARMv8.1 adds atomic swap and atomic load-operate instructions with
> optional memory ordering specifiers. This patch series adds the
> instructions to GCC, making them available with -march=armv8.1-a or
> -march=armv8+lse, and uses them to implement the __sync and __atomic
> builtins.
> 
> The ARMv8.1 swap instruction swaps the value in a register with a value
> in memory. The load-operate instructions load a value from memory,
> update it with the result of an operation and store the result in
> memory.
> 
> This series uses the swap instruction to implement the atomic_exchange
> patterns and the load-operate instructions to implement the
> atomic_fetch_<op> and atomic_<op>_fetch patterns. For the
> atomic_<op>_fetch patterns, the value returned as the result of the
> operation has to be recalculated from the loaded data. The ARMv8 BIC
> instruction is added so that it can be used for this recalculation.
> 
> The patches in this series
> - add and use the atomic swap instruction.
> - add the Aarch64 BIC instruction,
> - add the ARMv8.1 load-operate instructions,
> - use the load-operate instructions to implement the atomic_fetch_<op>
>    patterns,
> - use the load-operate instructions to implement the patterns
>    atomic_<op>_fetch patterns,
> 
> The code-generation changes in this series are based around a new
> function, aarch64_gen_atomic_ldop, which takes the operation to be
> implemented and emits the appropriate code, making use of the atomic
> instruction. This follows the existing uses aarch64_split_atomic_op for
> the same purpose when atomic instructions aren't available.
> 
> This patch adds the ARMv8.1 SWAP instruction and function
> aarch64_gen_atomic_ldop and changes the implementation of the
> atomic_exchange pattern to the atomic instruction when it is available.
> 
> The general form of the code generated for an atomic_exchange, with
> destination D, source S, memory address A and memory order MO is:
> 
>     swp<mo><sz> S, D, [A]
> 
> where
>     <mo> is one of {'', 'a', 'l', 'al'} depending on memory order MO.
>     <sz> is one of {'', 'b', 'h'} depending on the data size.
> 
> This patch also adds tests for the changes. These reuse the support code
> introduced for the atomic CAS tests, adding macros to test functions
> taking one memory ordering argument. These are used to iteratively
> define functions using the __atomic_exchange builtins, which should be
> implemented using the atomic swap.
> 
> Tested the series for aarch64-none-linux-gnu with native bootstrap and
> make check. Also tested for aarch64-none-elf with cross-compiled
> check-gcc on an ARMv8.1 emulator with +lse enabled by default.
> 
> Ok for trunk?

OK, though I have a question on one patch hunk.

> gcc/
> 2015-09-17  Matthew Wahab  <matthew.wahab@arm.com>
> 
> 	* config/aarch64/aarch64-protos.h (aarch64_gen_atomic_ldop):
> 	Declare.
> 	* config/aarch64/aarch64.c (aarch64_emit_atomic_swp): New.
> 	(aarch64_gen_atomic_ldop): New.
> 	(aarch64_split_atomic_op): Fix whitespace and add a comment.
> 	* config/aarch64/atomics.md (UNSPECV_ATOMIC_SWP): New.
> 	(atomic_compare_and_swap<mode>_lse): Remove comments and fix
> 	whitespace.
> 	(atomic_exchange<mode>): Replace with an expander.
> 	(aarch64_atomic_exchange<mode>): New.
> 	(aarch64_atomic_exchange<mode>_lse): New.
> 	(aarch64_atomic_<atomic_optab><mode>): Fix some whitespace.
> 	(aarch64_atomic_swp<mode>): New.
> 
> 
> gcc/testsuite/
> 2015-09-17  Matthew Wahab  <matthew.wahab@arm.com>
> 
> 	* gcc.target/aarch64/atomic-inst-ops.inc: (TEST_MODEL): New.
> 	(TEST_ONE): New.
>           * gcc.target/aarch64/atomic-inst-swap.c: New.
> 

> diff --git a/gcc/config/aarch64/atomics.md b/gcc/config/aarch64/atomics.md
> index 65d2cc9..0e71002 100644
> --- a/gcc/config/aarch64/atomics.md
> +++ b/gcc/config/aarch64/atomics.md
> @@ -27,6 +27,7 @@
>      UNSPECV_ATOMIC_CMPSW		; Represent an atomic compare swap.
>      UNSPECV_ATOMIC_EXCHG		; Represent an atomic exchange.
>      UNSPECV_ATOMIC_CAS			; Represent an atomic CAS.
> +    UNSPECV_ATOMIC_SWP			; Represent an atomic SWP.
>      UNSPECV_ATOMIC_OP			; Represent an atomic operation.
>  ])
>  
> @@ -122,19 +123,19 @@
>  )
>  
>  (define_insn_and_split "aarch64_compare_and_swap<mode>_lse"
> -  [(set (reg:CC CC_REGNUM)					;; bool out
> +  [(set (reg:CC CC_REGNUM)
>      (unspec_volatile:CC [(const_int 0)] UNSPECV_ATOMIC_CMPSW))
> -   (set (match_operand:GPI 0 "register_operand" "=&r")		;; val out
> -    (match_operand:GPI 1 "aarch64_sync_memory_operand" "+Q"))   ;; memory
> +   (set (match_operand:GPI 0 "register_operand" "=&r")
> +    (match_operand:GPI 1 "aarch64_sync_memory_operand" "+Q"))
>     (set (match_dup 1)
>      (unspec_volatile:GPI
> -      [(match_operand:GPI 2 "aarch64_plus_operand" "rI")	;; expect
> -       (match_operand:GPI 3 "register_operand" "r")		;; desired
> -       (match_operand:SI 4 "const_int_operand")			;; is_weak
> -       (match_operand:SI 5 "const_int_operand")			;; mod_s
> -       (match_operand:SI 6 "const_int_operand")]		;; mod_f
> +      [(match_operand:GPI 2 "aarch64_plus_operand" "rI")
> +       (match_operand:GPI 3 "register_operand" "r")
> +       (match_operand:SI 4 "const_int_operand")
> +       (match_operand:SI 5 "const_int_operand")
> +       (match_operand:SI 6 "const_int_operand")]

I'm not sure I understand the change here, those comments still look helpful
enough for understanding the pattern, what have a I missed?

Thanks,
James