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

[Matthew Wahab <matthew dot wahab at foss dot arm dot com>]

On 23/11/15 12:24, James Greenhalgh wrote:
> 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.

> 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.
>
> > 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?

I think it should by armv8.1-a only. +simd is enabled by all -march settings so it 
seems redundant to add it here. An alternative is to add +rdma but that's also 
enabled by armv8.1-a. (I've a patch at 
https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01973.html which gets rid for +rdma as 
part of an armv8.1-a command line clean up.)

> > +# 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).

I'll rework the patch to use the internal types names.

Matthew