Re: [AArch64][dejagnu][PATCH 5/7] Dejagnu support for ARMv8.1 Adv.SIMD.

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

On Tue, Oct 27, 2015 at 03:32:04PM +0000, Matthew Wahab wrote:
> On 24/10/15 08:16, Bernhard Reutner-Fischer wrote:
> >On October 23, 2015 2:24:26 PM GMT+02:00, Matthew Wahab <matthew.wahab@foss.arm.com> wrote:
> >>The ARMv8.1 architecture extension adds two Adv.SIMD instructions,.
> >>This
> >>patch adds support in Dejagnu for ARMv8.1 Adv.SIMD specifiers and
> >>checks.
> >>
> >>The new test options are
> >>- { dg-add-options arm_v8_1a_neon }: Add compiler options needed to
> >>   enable ARMv8.1 Adv.SIMD.
> >>- { dg-require-effective-target arm_v8_1a_neon_hw }: Require a target
> >>   capable of executing ARMv8.1 Adv.SIMD instructions.
> >>
> >
> >Please error with something more meaningful than FOO, !__ARM_FEATURE_QRDMX comes to mind.
> >
> >TIA,
> >
> 
> I've reworked the patch so that the error is "__ARM_FEATURE_QRDMX not
> defined" and also strengthened the check_effective_target tests.
> 
> Retested for aarch64-none-elf with cross-compiled check-gcc on an
> ARMv8.1 emulator. Also tested with a version of the compiler that
> doesn't define the ACLE feature macro.

Hi Matthew,

I have a couple of comments below. Neither need to block the patch, but
I'd appreciate a reply before I say OK.

> From b12969882298cb79737e882c48398c58a45161b9 Mon Sep 17 00:00:00 2001
> From: Matthew Wahab <matthew.wahab@arm.com>
> Date: Mon, 26 Oct 2015 14:58:36 +0000
> Subject: [PATCH 5/7] [Testsuite] Add dejagnu options for armv8.1 neon
> 
> Change-Id: Ib58b8c4930ad3971af3ea682eda043e14cd2e8b3
> ---
>  gcc/testsuite/lib/target-supports.exp | 56 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
> index 4d5b0a3d..0fb679d 100644
> --- a/gcc/testsuite/lib/target-supports.exp
> +++ b/gcc/testsuite/lib/target-supports.exp
> @@ -2700,6 +2700,16 @@ proc add_options_for_arm_v8_neon { flags } {
>      return "$flags $et_arm_v8_neon_flags -march=armv8-a"
>  }
>  
> +# Add the options needed for ARMv8.1 Adv.SIMD.
> +
> +proc add_options_for_arm_v8_1a_neon { flags } {
> +    if { [istarget aarch64*-*-*] } {
> +	return "$flags -march=armv8.1-a"

Should this be -march=armv8.1-a+simd or some other feature flag?

> +    } else {
> +	return "$flags"
> +    }
> +}
> +
>  proc add_options_for_arm_crc { flags } {
>      if { ! [check_effective_target_arm_crc_ok] } {
>          return "$flags"
> @@ -2984,7 +2994,8 @@ foreach { armfunc armflag armdef } { v4 "-march=armv4 -marm" __ARM_ARCH_4__
>  				     v7r "-march=armv7-r" __ARM_ARCH_7R__
>  				     v7m "-march=armv7-m -mthumb" __ARM_ARCH_7M__
>  				     v7em "-march=armv7e-m -mthumb" __ARM_ARCH_7EM__
> -				     v8a "-march=armv8-a" __ARM_ARCH_8A__ } {
> +				     v8a "-march=armv8-a" __ARM_ARCH_8A__
> +				     v8_1a "-march=armv8.1a" __ARM_ARCH_8A__ } {
>      eval [string map [list FUNC $armfunc FLAG $armflag DEF $armdef ] {
>  	proc check_effective_target_arm_arch_FUNC_ok { } {
>  	    if { [ string match "*-marm*" "FLAG" ] &&
> @@ -3141,6 +3152,25 @@ proc check_effective_target_arm_neonv2_hw { } {
>      } [add_options_for_arm_neonv2 ""]]
>  }
>  
> +# Return 1 if the target supports the ARMv8.1 Adv.SIMD extension, 0
> +# otherwise.  The test is valid for AArch64.
> +
> +proc check_effective_target_arm_v8_1a_neon_ok_nocache { } {
> +    if { ![istarget aarch64*-*-*] } {
> +	return 0
> +    }
> +    return [check_no_compiler_messages_nocache arm_v8_1a_neon_ok assembly {
> +	#if !defined (__ARM_FEATURE_QRDMX)
> +	#error "__ARM_FEATURE_QRDMX not defined"
> +	#endif
> +    } [add_options_for_arm_v8_1a_neon ""]]
> +}
> +
> +proc check_effective_target_arm_v8_1a_neon_ok { } {
> +    return [check_cached_effective_target arm_v8_1a_neon_ok \
> +		check_effective_target_arm_v8_1a_neon_ok_nocache]
> +}
> +
>  # Return 1 if the target supports executing ARMv8 NEON instructions, 0
>  # otherwise.
>  
> @@ -3159,6 +3189,30 @@ proc check_effective_target_arm_v8_neon_hw { } {
>      } [add_options_for_arm_v8_neon ""]]
>  }
>  
> +# Return 1 if the target supports executing the ARMv8.1 Adv.SIMD extension, 0
> +# otherwise.  The test is valid for AArch64.
> +
> +proc check_effective_target_arm_v8_1a_neon_hw { } {
> +    if { ![check_effective_target_arm_v8_1a_neon_ok] } {
> +	return 0;
> +    }
> +    return [check_runtime_nocache arm_v8_1a_neon_hw_available {
> +	int
> +	main (void)
> +	{
> +	  long long a = 0, b = 1;
> +	  long long result = 0;
> +
> +	  asm ("sqrdmlah %s0,%s1,%s2"
> +	       : "=w"(result)
> +	       : "w"(a), "w"(b)
> +	       : /* No clobbers.  */);

Hm, those types look wrong, I guess this works but it is an unusual way
to write it. I presume this is to avoid including arm_neon.h each time, but
you could just directly use the internal type names for the arm_neon types.
That is to say __Int32x4_t (or whichever mode you intend to use).

> +
> +	  return result;
> +	}
> +    }  [add_options_for_arm_v8_1a_neon ""]]
> +}
> +
>  # Return 1 if this is a ARM target with NEON enabled.
>  
>  proc check_effective_target_arm_neon { } {

Thanks,
James