Re: [Aarch64][target/PR 67143] Use correct constraints on operands for atomic operations.

[James Greenhalgh <james.greenhalgh@arm.com>]

On Tue, Aug 11, 2015 at 02:05:37PM +0100, Matthew Wahab wrote:
> The Aarch64 backend implements the atomic operations on memory with the same
> patterns iterated over logical and arithmetic operation. It also specifies
> constraints on an operand for the operations but the constraints used are only
> valid for the logical operations. This can lead to an ICE, with the compiler
> unable to generate a valid instruction.
> 
> This patch reworks the atomic operation patterns to select the
> appropriate constraint for the operation. The logical operations take
> the constraints specified by the current lconst_atomic mode iterator
> while the arithmetic operations (plus, sub) take constraint "I".
> 
> This patch also adds the test-case provided by Matthia Klose for the bugzilla
> entry https://gcc.gnu.org/bugzilla/show_bug.cgi?id=67143, slightly modified to
> compile as plain C.
> 
> Tested for arm-none-eabi with cross-compiled check-gcc and for
> arm-none-linux-gnueabihf with native bootstrap and make check.
> 
> Ok for trunk?
>
> Matthew
> gcc/
> 2015-08-10  Matthew Wahab  <matthew.wahab@arm.com>
> 
> 	PR target/67143
> 	* config/aarch64/atomic.md (atomic_<optab><mode>): Replace
> 	'lconst_atomic' with 'const_atomic'.
> 	(atomic_fetch_<optab><mode>): Likewise.
> 	(atomic_<optab>_fetch<mode>): Likewise.
> 	* config/aarch64/iterators.md (lconst-atomic): Move below
> 	'const_atomic'.
> 	(const_atomic): New.
> 
> gcc/testsuite/
> 2015-08-10  Matthew Wahab  <matthew.wahab@arm.com>
> 	    Matthias Klose  <doko@debian.org>
> 
> 	PR target/67143
> 	* gcc.target/aarch64/pr67143.c: New

> diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md
> index 5d7966d..d3d6af7 100644
> --- a/gcc/config/aarch64/iterators.md
> +++ b/gcc/config/aarch64/iterators.md
> @@ -345,9 +345,6 @@
>  ;; Attribute to describe constants acceptable in logical operations
>  (define_mode_attr lconst [(SI "K") (DI "L")])
>  
> -;; Attribute to describe constants acceptable in atomic logical operations
> -(define_mode_attr lconst_atomic [(QI "K") (HI "K") (SI "K") (DI "L")])
> -
>  ;; Map a mode to a specific constraint character.
>  (define_mode_attr cmode [(QI "q") (HI "h") (SI "s") (DI "d")])
>  
> @@ -843,6 +840,16 @@
>     (plus "aarch64_plus_operand")
>     (minus "aarch64_plus_operand")])
>  
> +;; Constants acceptable for atomic operations.
> +;; This definition must appear in this file before the iterators it refers to.
> +(define_code_attr const_atomic
> + [(plus "I") (minus "I")

I'm worried this still gives us a mismatch between constraints and
predicates. The relevant predicates here are:

  (define_predicate "aarch64_plus_operand"
    (ior (match_operand 0 "register_operand")
         (match_operand 0 "aarch64_plus_immediate")))

  (define_predicate "aarch64_plus_immediate"
    (and (match_code "const_int")
         (ior (match_test "aarch64_uimm12_shift (INTVAL (op))")
	      (match_test "aarch64_uimm12_shift (-INTVAL (op))"))))

But our constraint only permits:

  (define_constraint "I"
   "A constant that can be used with an ADD operation."
   (and (match_code "const_int")
        (match_test "aarch64_uimm12_shift (ival)")))

Does this mean we are now loading constants we don't need to in to
registers? I don't think we could cause this to ICE - but are we
generating less good code than we would like?

Thanks,
James