[PATCH] Fix simd attribute handling on aarch64

classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Fix simd attribute handling on aarch64

Steve Ellcey-10
This patch fixes a bug with SIMD functions on Aarch64.  I found it
while trying to run SPEC with ToT GCC and a glibc that defines vector
math functions for aarch64.  When a function is declared with the simd
attribute GCC creates vector clones of that function with the return
and argument types changed to vector types.  On Aarch64 the vector
clones are also marked with the aarch64_vector_pcs attribute to signify
that they use an alternate calling convention.  Due to a bug in GCC the
non-vector version of the function being cloned was also being marked
with this attribute.

Because simd_clone_adjust and expand_simd_clones are calling
targetm.simd_clone.adjust (which attached the aarch64_vector_pcs
attribute to the function type) before calling
simd_clone_adjust_return_type (which created a new distinct type tree
for the cloned function) the attribute got attached to both the
'normal' scalar version of the SIMD function and any vector versions of
the function.  The attribute should only be on the vector versions.

My fix is to call simd_clone_adjust_return_type and create the new type
before calling targetm.simd_clone.adjust which adds the attribute.  The
only other platform that this patch could affect is x86 because that is
the only other platform to use targetm.simd_clone.adjust.  I did a
bootstrap and gcc test run on x86 (as well as Aarch64) and got no
regressions.

OK to checkin?

Steve Ellcey
[hidden email]


2019-07-17  Steve Ellcey  <[hidden email]>

        * omp-simd-clone.c (simd_clone_adjust):  Call targetm.simd_clone.adjust
        after calling simd_clone_adjust_return_type.
        (expand_simd_clones): Ditto.


diff --git a/gcc/omp-simd-clone.c b/gcc/omp-simd-clone.c
index caa8da3cba5..6a6b439d146 100644
--- a/gcc/omp-simd-clone.c
+++ b/gcc/omp-simd-clone.c
@@ -1164,9 +1164,8 @@ simd_clone_adjust (struct cgraph_node *node)
 {
   push_cfun (DECL_STRUCT_FUNCTION (node->decl));
 
-  targetm.simd_clone.adjust (node);
-
   tree retval = simd_clone_adjust_return_type (node);
+  targetm.simd_clone.adjust (node);
   ipa_parm_adjustment_vec adjustments
     = simd_clone_adjust_argument_types (node);
 
@@ -1737,8 +1736,8 @@ expand_simd_clones (struct cgraph_node *node)
     simd_clone_adjust (n);
   else
     {
-      targetm.simd_clone.adjust (n);
       simd_clone_adjust_return_type (n);
+      targetm.simd_clone.adjust (n);
       simd_clone_adjust_argument_types (n);
     }
  }


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix simd attribute handling on aarch64

Richard Sandiford-9
Steve Ellcey <[hidden email]> writes:

> This patch fixes a bug with SIMD functions on Aarch64.  I found it
> while trying to run SPEC with ToT GCC and a glibc that defines vector
> math functions for aarch64.  When a function is declared with the simd
> attribute GCC creates vector clones of that function with the return
> and argument types changed to vector types.  On Aarch64 the vector
> clones are also marked with the aarch64_vector_pcs attribute to signify
> that they use an alternate calling convention.  Due to a bug in GCC the
> non-vector version of the function being cloned was also being marked
> with this attribute.
>
> Because simd_clone_adjust and expand_simd_clones are calling
> targetm.simd_clone.adjust (which attached the aarch64_vector_pcs
> attribute to the function type) before calling
> simd_clone_adjust_return_type (which created a new distinct type tree
> for the cloned function) the attribute got attached to both the
> 'normal' scalar version of the SIMD function and any vector versions of
> the function.  The attribute should only be on the vector versions.
>
> My fix is to call simd_clone_adjust_return_type and create the new type
> before calling targetm.simd_clone.adjust which adds the attribute.  The
> only other platform that this patch could affect is x86 because that is
> the only other platform to use targetm.simd_clone.adjust.  I did a
> bootstrap and gcc test run on x86 (as well as Aarch64) and got no
> regressions.
>
> OK to checkin?
>
> Steve Ellcey
> [hidden email]
>
>
> 2019-07-17  Steve Ellcey  <[hidden email]>
>
> * omp-simd-clone.c (simd_clone_adjust):  Call targetm.simd_clone.adjust
> after calling simd_clone_adjust_return_type.
> (expand_simd_clones): Ditto.

It should be pretty easy to add a test for this, now that we use
.variant_pcs to mark symbols with the attribute.

> diff --git a/gcc/omp-simd-clone.c b/gcc/omp-simd-clone.c
> index caa8da3cba5..6a6b439d146 100644
> --- a/gcc/omp-simd-clone.c
> +++ b/gcc/omp-simd-clone.c
> @@ -1164,9 +1164,8 @@ simd_clone_adjust (struct cgraph_node *node)
>  {
>    push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>  
> -  targetm.simd_clone.adjust (node);
> -
>    tree retval = simd_clone_adjust_return_type (node);
> +  targetm.simd_clone.adjust (node);
>    ipa_parm_adjustment_vec adjustments
>      = simd_clone_adjust_argument_types (node);
>  
> @@ -1737,8 +1736,8 @@ expand_simd_clones (struct cgraph_node *node)
>      simd_clone_adjust (n);
>    else
>      {
> -      targetm.simd_clone.adjust (n);
>        simd_clone_adjust_return_type (n);
> +      targetm.simd_clone.adjust (n);
>        simd_clone_adjust_argument_types (n);
>      }
>   }

I don't think this is enough, since simd_clone_adjust_return_type
does nothing for functions that return void (e.g. sincos).
I think instead aarch64_simd_clone_adjust should do something like:

  TREE_TYPE (node->decl) = build_distinct_type_copy (TREE_TYPE (node->decl));

But maybe that has consequences that I've not thought about...

Thanks,
Richard
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix simd attribute handling on aarch64

Steve Ellcey-10
On Thu, 2019-07-18 at 08:37 +0100, Richard Sandiford wrote:
>
> > 2019-07-17  Steve Ellcey  <[hidden email]>
> >
> > * omp-simd-clone.c (simd_clone_adjust):  Call targetm.simd_clone.adjust
> > after calling simd_clone_adjust_return_type.
> > (expand_simd_clones): Ditto.
>
> It should be pretty easy to add a test for this, now that we use
> .variant_pcs to mark symbols with the attribute.

OK, I will add some tests that makes sure this mark is not on the
scalar version of a simd function.

> > diff --git a/gcc/omp-simd-clone.c b/gcc/omp-simd-clone.c
> > index caa8da3cba5..6a6b439d146 100644
> > --- a/gcc/omp-simd-clone.c
> > +++ b/gcc/omp-simd-clone.c
> > @@ -1164,9 +1164,8 @@ simd_clone_adjust (struct cgraph_node *node)
> >  {
> >    push_cfun (DECL_STRUCT_FUNCTION (node->decl));
> >  
> > -  targetm.simd_clone.adjust (node);
> > -
> >    tree retval = simd_clone_adjust_return_type (node);
> > +  targetm.simd_clone.adjust (node);
> >    ipa_parm_adjustment_vec adjustments
> >      = simd_clone_adjust_argument_types (node);
> >  
> > @@ -1737,8 +1736,8 @@ expand_simd_clones (struct cgraph_node *node)
> >      simd_clone_adjust (n);
> >    else
> >      {
> > -      targetm.simd_clone.adjust (n);
> >        simd_clone_adjust_return_type (n);
> > +      targetm.simd_clone.adjust (n);
> >        simd_clone_adjust_argument_types (n);
> >      }
> >   }
>
> I don't think this is enough, since simd_clone_adjust_return_type
> does nothing for functions that return void (e.g. sincos).
> I think instead aarch64_simd_clone_adjust should do something like:
>
>   TREE_TYPE (node->decl) = build_distinct_type_copy (TREE_TYPE (node-
> >decl));
>
> But maybe that has consequences that I've not thought about...

I think that would work, but it would build two distinct types for non-
void functions, one of which would be unused/uneeded.  I.e.
aarch64_simd_clone_adjust would create a distinct type and then
simd_clone_adjust_return_type would create another distinct type
and the previous one would no longer be used anywhere.

What do you think about moving the call to build_distinct_type_copy
out of simd_clone_adjust_return_type and doing it even for null
types.  Below is what I am thinking about (untested).  I suppose
we could also leave the call to build_distinct_type_copy in
simd_clone_adjust_return_type but just move it above the check
for a NULL type so that a distinct type is always created there.
That would still require that we change the order of the
targetm.simd_clone.adjust and simd_clone_adjust_return_type
calls as my original patch does.


diff --git a/gcc/omp-simd-clone.c b/gcc/omp-simd-clone.c
index caa8da3cba5..427d6f6f514 100644
--- a/gcc/omp-simd-clone.c
+++ b/gcc/omp-simd-clone.c
@@ -498,7 +498,6 @@ simd_clone_adjust_return_type (struct cgraph_node
*node)
   /* Adjust the function return type.  */
   if (orig_rettype == void_type_node)
     return NULL_TREE;
-  TREE_TYPE (fndecl) = build_distinct_type_copy (TREE_TYPE (fndecl));
   t = TREE_TYPE (TREE_TYPE (fndecl));
   if (INTEGRAL_TYPE_P (t) || POINTER_TYPE_P (t))
     veclen = node->simdclone->vecsize_int;
@@ -1164,6 +1163,7 @@ simd_clone_adjust (struct cgraph_node *node)
 {
   push_cfun (DECL_STRUCT_FUNCTION (node->decl));
 
+  TREE_TYPE (node->decl) = build_distinct_type_copy (TREE_TYPE (node-
>decl));
   targetm.simd_clone.adjust (node);
 
   tree retval = simd_clone_adjust_return_type (node);
@@ -1737,6 +1737,8 @@ expand_simd_clones (struct cgraph_node *node)
            simd_clone_adjust (n);
          else
            {
+             TREE_TYPE (n->decl)
+               = build_distinct_type_copy (TREE_TYPE (n->decl));
              targetm.simd_clone.adjust (n);
              simd_clone_adjust_return_type (n);
              simd_clone_adjust_argument_types (n);


Steve Ellcey
[hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix simd attribute handling on aarch64 (version 2)

Steve Ellcey-10
In reply to this post by Richard Sandiford-9
Here is version two of my patch to fix simd attribute handling on
aarch64.  Unlike the first patch where I swapped the order of the
calls to targetm.simd_clone.adjust and simd_clone_adjust_return_type,
in this one I remove the (conditional) call to build_distinct_type_copy
from simd_clone_adjust_return_type and do it unconditionally before
calling either routine.  The only downside to this that I can see is
that on non-aarch64 platforms where the return type of a vector
function is VOID (and not changed), we will create a distinct type
where we did not before.

I also added some tests to ensure that, on aarch64, the vector
functions created by cloning a simd function have the .variant_pcs
directive and that the original non-vector version of the function
does not have the directive.  Without this patch the non-vector
version is putting out the directive, that is what this patch
fixes.

Retested on x86 and aarch64 with no regressions.

OK to checkin?

Steve Ellcey
[hidden email]


2019-07-19  Steve Ellcey  <[hidden email]>

        * omp-simd-clone.c (simd_clone_adjust_return_type): Remove call to
        build_distinct_type_copy.
        (simd_clone_adjust): Call build_distinct_type_copy.
        (expand_simd_clones): Ditto.


2019-07-19  Steve Ellcey  <[hidden email]>

        * gcc.target/aarch64/simd_pcs_attribute.c: New test.
        * gcc.target/aarch64/simd_pcs_attribute-2.c: Ditto.
        * gcc.target/aarch64/simd_pcs_attribute-3.c: Ditto.



gcc-simd.patch (1K) Download Attachment
gcc-simd-test.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix simd attribute handling on aarch64 (version 2)

Richard Sandiford-9
Steve Ellcey <[hidden email]> writes:

> Here is version two of my patch to fix simd attribute handling on
> aarch64.  Unlike the first patch where I swapped the order of the
> calls to targetm.simd_clone.adjust and simd_clone_adjust_return_type,
> in this one I remove the (conditional) call to build_distinct_type_copy
> from simd_clone_adjust_return_type and do it unconditionally before
> calling either routine.  The only downside to this that I can see is
> that on non-aarch64 platforms where the return type of a vector
> function is VOID (and not changed), we will create a distinct type
> where we did not before.
>
> I also added some tests to ensure that, on aarch64, the vector
> functions created by cloning a simd function have the .variant_pcs
> directive and that the original non-vector version of the function
> does not have the directive.  Without this patch the non-vector
> version is putting out the directive, that is what this patch
> fixes.
>
> Retested on x86 and aarch64 with no regressions.
>
> OK to checkin?
>
> Steve Ellcey
> [hidden email]
>
>
> 2019-07-19  Steve Ellcey  <[hidden email]>
>
> * omp-simd-clone.c (simd_clone_adjust_return_type): Remove call to
> build_distinct_type_copy.
> (simd_clone_adjust): Call build_distinct_type_copy.
> (expand_simd_clones): Ditto.
>
>
> 2019-07-19  Steve Ellcey  <[hidden email]>
>
> * gcc.target/aarch64/simd_pcs_attribute.c: New test.
> * gcc.target/aarch64/simd_pcs_attribute-2.c: Ditto.
> * gcc.target/aarch64/simd_pcs_attribute-3.c: Ditto.
>
>
> diff --git a/gcc/omp-simd-clone.c b/gcc/omp-simd-clone.c
> index caa8da3cba5..427d6f6f514 100644
> --- a/gcc/omp-simd-clone.c
> +++ b/gcc/omp-simd-clone.c
> @@ -498,7 +498,6 @@ simd_clone_adjust_return_type (struct cgraph_node *node)
>    /* Adjust the function return type.  */
>    if (orig_rettype == void_type_node)
>      return NULL_TREE;
> -  TREE_TYPE (fndecl) = build_distinct_type_copy (TREE_TYPE (fndecl));
>    t = TREE_TYPE (TREE_TYPE (fndecl));
>    if (INTEGRAL_TYPE_P (t) || POINTER_TYPE_P (t))
>      veclen = node->simdclone->vecsize_int;
> @@ -1164,6 +1163,7 @@ simd_clone_adjust (struct cgraph_node *node)
>  {
>    push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>  
> +  TREE_TYPE (node->decl) = build_distinct_type_copy (TREE_TYPE (node->decl));
>    targetm.simd_clone.adjust (node);
>  
>    tree retval = simd_clone_adjust_return_type (node);
> @@ -1737,6 +1737,8 @@ expand_simd_clones (struct cgraph_node *node)
>      simd_clone_adjust (n);
>    else
>      {
> +      TREE_TYPE (n->decl)
> + = build_distinct_type_copy (TREE_TYPE (n->decl));
>        targetm.simd_clone.adjust (n);
>        simd_clone_adjust_return_type (n);
>        simd_clone_adjust_argument_types (n);

You can probably also remove:

      tree new_type = build_distinct_type_copy (TREE_TYPE (node->decl));
      ...
      TREE_TYPE (node->decl) = new_type;

in simd_clone_adjust_argument_types.

I'm happy doing it this way or doing the copy in the AArch64 hook.
It's really Jakub's call.

I don't think the tests need:

/* { dg-require-effective-target aarch64_variant_pcs } */

since they're only dg-do compile.  Leaving the line out would get more
coverage for people using older binutils.

The tests are OK with that change, thanks.

Richard
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix simd attribute handling on aarch64 (version 2)

Steve Ellcey-10
On Fri, 2019-07-19 at 19:24 +0100, Richard Sandiford wrote:

>
> You can probably also remove:
>
>       tree new_type = build_distinct_type_copy (TREE_TYPE (node->decl));
>       ...
>       TREE_TYPE (node->decl) = new_type;
>
> in simd_clone_adjust_argument_types.
>
> I'm happy doing it this way or doing the copy in the AArch64 hook.
> It's really Jakub's call.
You are right, that is no longer needed with the current patch.  I
removed it and retested with no regressions.  Jakub, do you have
any preference?  I have attached a new version of the patch to this
email.

> I don't think the tests need:
>
> /* { dg-require-effective-target aarch64_variant_pcs } */
>
> since they're only dg-do compile.  Leaving the line out would get more
> coverage for people using older binutils.
>
> The tests are OK with that change, thanks.

OK, I made that change to the tests.


Latest version of the patch:

2019-07-22  Steve Ellcey  <[hidden email]>

        * omp-simd-clone.c (simd_clone_adjust_return_type): Remove call to
        build_distinct_type_copy.
        (simd_clone_adjust_argument_types): Ditto.
        (simd_clone_adjust): Call build_distinct_type_copy here.
        (expand_simd_clones): Ditto.


2019-07-22  Steve Ellcey  <[hidden email]>

        * gcc.target/aarch64/simd_pcs_attribute.c: New test.
        * gcc.target/aarch64/simd_pcs_attribute-2.c: Ditto.
        * gcc.target/aarch64/simd_pcs_attribute-3.c: Ditto.



gcc-simd.patch (2K) Download Attachment
gcc-simd-test.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix simd attribute handling on aarch64 (version 2)

Steve Ellcey-10
Ping.

Steve Ellcey
[hidden email]

On Mon, 2019-07-22 at 11:25 -0700, Steve Ellcey wrote:

> On Fri, 2019-07-19 at 19:24 +0100, Richard Sandiford wrote:
> >
> > You can probably also remove:
> >
> >       tree new_type = build_distinct_type_copy (TREE_TYPE (node-
> > >decl));
> >       ...
> >       TREE_TYPE (node->decl) = new_type;
> >
> > in simd_clone_adjust_argument_types.
> >
> > I'm happy doing it this way or doing the copy in the AArch64 hook.
> > It's really Jakub's call.
>
> You are right, that is no longer needed with the current patch.  I
> removed it and retested with no regressions.  Jakub, do you have
> any preference?  I have attached a new version of the patch to this
> email.
>
> > I don't think the tests need:
> >
> > /* { dg-require-effective-target aarch64_variant_pcs } */
> >
> > since they're only dg-do compile.  Leaving the line out would get
> > more
> > coverage for people using older binutils.
> >
> > The tests are OK with that change, thanks.
>
> OK, I made that change to the tests.
>
>
> Latest version of the patch:
>
> 2019-07-22  Steve Ellcey  <[hidden email]>
>
> * omp-simd-clone.c (simd_clone_adjust_return_type): Remove call
> to
> build_distinct_type_copy.
> (simd_clone_adjust_argument_types): Ditto.
> (simd_clone_adjust): Call build_distinct_type_copy here.
> (expand_simd_clones): Ditto.
>
>
> 2019-07-22  Steve Ellcey  <[hidden email]>
>
> * gcc.target/aarch64/simd_pcs_attribute.c: New test.
> * gcc.target/aarch64/simd_pcs_attribute-2.c: Ditto.
> * gcc.target/aarch64/simd_pcs_attribute-3.c: Ditto.
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix simd attribute handling on aarch64 (version 2)

Richard Sandiford-9
In reply to this post by Steve Ellcey-10
Steve Ellcey <[hidden email]> writes:

> On Fri, 2019-07-19 at 19:24 +0100, Richard Sandiford wrote:
>>
>> You can probably also remove:
>>
>>       tree new_type = build_distinct_type_copy (TREE_TYPE (node->decl));
>>       ...
>>       TREE_TYPE (node->decl) = new_type;
>>
>> in simd_clone_adjust_argument_types.
>>
>> I'm happy doing it this way or doing the copy in the AArch64 hook.
>> It's really Jakub's call.
>
> You are right, that is no longer needed with the current patch.  I
> removed it and retested with no regressions.  Jakub, do you have
> any preference?  I have attached a new version of the patch to this
> email.
>
>> I don't think the tests need:
>>
>> /* { dg-require-effective-target aarch64_variant_pcs } */
>>
>> since they're only dg-do compile.  Leaving the line out would get more
>> coverage for people using older binutils.
>>
>> The tests are OK with that change, thanks.
>
> OK, I made that change to the tests.
>
>
> Latest version of the patch:
>
> 2019-07-22  Steve Ellcey  <[hidden email]>
>
> * omp-simd-clone.c (simd_clone_adjust_return_type): Remove call to
> build_distinct_type_copy.
> (simd_clone_adjust_argument_types): Ditto.
> (simd_clone_adjust): Call build_distinct_type_copy here.
> (expand_simd_clones): Ditto.
>
>
> 2019-07-22  Steve Ellcey  <[hidden email]>
>
> * gcc.target/aarch64/simd_pcs_attribute.c: New test.
> * gcc.target/aarch64/simd_pcs_attribute-2.c: Ditto.
> * gcc.target/aarch64/simd_pcs_attribute-3.c: Ditto.
>
>
> diff --git a/gcc/omp-simd-clone.c b/gcc/omp-simd-clone.c
> index caa8da3cba5..da01d9b8e6c 100644
> --- a/gcc/omp-simd-clone.c
> +++ b/gcc/omp-simd-clone.c
> @@ -498,7 +498,6 @@ simd_clone_adjust_return_type (struct cgraph_node *node)
>    /* Adjust the function return type.  */
>    if (orig_rettype == void_type_node)
>      return NULL_TREE;
> -  TREE_TYPE (fndecl) = build_distinct_type_copy (TREE_TYPE (fndecl));
>    t = TREE_TYPE (TREE_TYPE (fndecl));
>    if (INTEGRAL_TYPE_P (t) || POINTER_TYPE_P (t))
>      veclen = node->simdclone->vecsize_int;
> @@ -724,11 +723,6 @@ simd_clone_adjust_argument_types (struct cgraph_node *node)
>    else
>      new_reversed = void_list_node;
>   }
> -
> -      tree new_type = build_distinct_type_copy (TREE_TYPE (node->decl));
> -      TYPE_ARG_TYPES (new_type) = new_reversed;

I think you still need this line, just applied to the existing type
rather than a new one.

> -      TREE_TYPE (node->decl) = new_type;
> -
>        adjustments.release ();
>      }
>    args.release ();
> @@ -1164,6 +1158,7 @@ simd_clone_adjust (struct cgraph_node *node)
>  {
>    push_cfun (DECL_STRUCT_FUNCTION (node->decl));
>  
> +  TREE_TYPE (node->decl) = build_distinct_type_copy (TREE_TYPE (node->decl));
>    targetm.simd_clone.adjust (node);
>  
>    tree retval = simd_clone_adjust_return_type (node);
> @@ -1737,6 +1732,8 @@ expand_simd_clones (struct cgraph_node *node)
>      simd_clone_adjust (n);
>    else
>      {
> +      TREE_TYPE (n->decl)
> + = build_distinct_type_copy (TREE_TYPE (n->decl));
>        targetm.simd_clone.adjust (n);
>        simd_clone_adjust_return_type (n);
>        simd_clone_adjust_argument_types (n);
>
> diff --git a/gcc/testsuite/gcc.target/aarch64/simd_pcs_attribute-2.c b/gcc/testsuite/gcc.target/aarch64/simd_pcs_attribute-2.c
> index e69de29bb2d..8eec6824f37 100644
> --- a/gcc/testsuite/gcc.target/aarch64/simd_pcs_attribute-2.c
> +++ b/gcc/testsuite/gcc.target/aarch64/simd_pcs_attribute-2.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Ofast" } */
> +
> +__attribute__ ((__simd__ ("notinbranch")))
> +__attribute__ ((__nothrow__ , __leaf__ , __const__))
> +extern double foo (double x);
> +
> +void bar(double * f, int n)
> +{
> + int i;
> + for (i = 0; i < n; i++)
> + f[i] = foo(f[i]);
> +}
> +
> +/* { dg-final { scan-assembler-not {\.variant_pcs\tfoo} } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\t_ZGVnN2v_foo} 1 } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/simd_pcs_attribute-3.c b/gcc/testsuite/gcc.target/aarch64/simd_pcs_attribute-3.c
> index e69de29bb2d..95f6a6803e8 100644
> --- a/gcc/testsuite/gcc.target/aarch64/simd_pcs_attribute-3.c
> +++ b/gcc/testsuite/gcc.target/aarch64/simd_pcs_attribute-3.c
> @@ -0,0 +1,24 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Ofast" } */
> +
> +__attribute__ ((__simd__))
> +__attribute__ ((__nothrow__ , __leaf__ , __const__))
> +double foo (double x);
> +
> +void bar(double *f, int n)
> +{
> + int i;
> + for (i = 0; i < n; i++)
> + f[i] = foo(f[i]);
> +}
> +
> +double foo(double x)
> +{
> + return x * x / 3.0;
> +}
> +
> +/* { dg-final { scan-assembler-not {\.variant_pcs\tfoo} } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\t_ZGVnM1v_foo} 1 } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\t_ZGVnM2v_foo} 1 } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\t_ZGVnN1v_foo} 1 } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\t_ZGVnN2v_foo} 1 } } */
> diff --git a/gcc/testsuite/gcc.target/aarch64/simd_pcs_attribute.c b/gcc/testsuite/gcc.target/aarch64/simd_pcs_attribute.c
> index e69de29bb2d..eddcef3597c 100644
> --- a/gcc/testsuite/gcc.target/aarch64/simd_pcs_attribute.c
> +++ b/gcc/testsuite/gcc.target/aarch64/simd_pcs_attribute.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Ofast" } */
> +
> +__attribute__ ((__simd__ ("notinbranch")))
> +__attribute__ ((__nothrow__ , __leaf__ , __const__))
> +extern double log (double __x);
> +
> +void foo(double *f, int n)
> +{
> + int i;
> + for (i = 0; i < n; i++)
> + f[i] = log(f[i]);
> +}
> +
> +/* { dg-final { scan-assembler-not {\.variant_pcs\tlog} } } */
> +/* { dg-final { scan-assembler-times {\.variant_pcs\t_ZGVnN2v_log} 1 } } */
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix simd attribute handling on aarch64 (version 2)

Steve Ellcey-10
On Tue, 2019-07-30 at 14:31 +0100, Richard Sandiford wrote:

>
> > -
> > -      tree new_type = build_distinct_type_copy (TREE_TYPE (node-
> > >decl));
> > -      TYPE_ARG_TYPES (new_type) = new_reversed;
>
> I think you still need this line, just applied to the existing type
> rather than a new one.
>
> > -      TREE_TYPE (node->decl) = new_type;
> > -
> >        adjustments.release ();
> >      }
OK, I fixed that and retested with no regressions.

Steve Ellcey
[hidden email]


2019-07-30  Steve Ellcey  <[hidden email]>

        * omp-simd-clone.c (simd_clone_adjust_return_type): Remove call to
        build_distinct_type_copy.
        (simd_clone_adjust_argument_types): Ditto.
        (simd_clone_adjust): Call build_distinct_type_copy here.
        (expand_simd_clones): Ditto.

2019-07-30  Steve Ellcey  <[hidden email]>

        * gcc.target/aarch64/simd_pcs_attribute.c: New test.
        * gcc.target/aarch64/simd_pcs_attribute-2.c: Ditto.
        * gcc.target/aarch64/simd_pcs_attribute-3.c: Ditto.

gcc-simd.patch (2K) Download Attachment
gcc-simd-test.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix simd attribute handling on aarch64 (version 2)

Richard Sandiford-9
Steve Ellcey <[hidden email]> writes:

> On Tue, 2019-07-30 at 14:31 +0100, Richard Sandiford wrote:
>>
>> > -
>> > -      tree new_type = build_distinct_type_copy (TREE_TYPE (node-
>> > >decl));
>> > -      TYPE_ARG_TYPES (new_type) = new_reversed;
>>
>> I think you still need this line, just applied to the existing type
>> rather than a new one.
>>
>> > -      TREE_TYPE (node->decl) = new_type;
>> > -
>> >        adjustments.release ();
>> >      }
>
> OK, I fixed that and retested with no regressions.
>
> Steve Ellcey
> [hidden email]
>
>
> 2019-07-30  Steve Ellcey  <[hidden email]>
>
> * omp-simd-clone.c (simd_clone_adjust_return_type): Remove call to
> build_distinct_type_copy.
> (simd_clone_adjust_argument_types): Ditto.
> (simd_clone_adjust): Call build_distinct_type_copy here.
> (expand_simd_clones): Ditto.
>
> 2019-07-30  Steve Ellcey  <[hidden email]>
>
> * gcc.target/aarch64/simd_pcs_attribute.c: New test.
> * gcc.target/aarch64/simd_pcs_attribute-2.c: Ditto.
> * gcc.target/aarch64/simd_pcs_attribute-3.c: Ditto.

OK if there are no objections in 48 hours.

Thanks,
Richard
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix simd attribute handling on aarch64 (version 2)

Szabolcs Nagy-2
On 31/07/2019 08:25, Richard Sandiford wrote:

> Steve Ellcey <[hidden email]> writes:
>>
>> 2019-07-30  Steve Ellcey  <[hidden email]>
>>
>> * omp-simd-clone.c (simd_clone_adjust_return_type): Remove call to
>> build_distinct_type_copy.
>> (simd_clone_adjust_argument_types): Ditto.
>> (simd_clone_adjust): Call build_distinct_type_copy here.
>> (expand_simd_clones): Ditto.
>>
>> 2019-07-30  Steve Ellcey  <[hidden email]>
>>
>> * gcc.target/aarch64/simd_pcs_attribute.c: New test.
>> * gcc.target/aarch64/simd_pcs_attribute-2.c: Ditto.
>> * gcc.target/aarch64/simd_pcs_attribute-3.c: Ditto.
>
> OK if there are no objections in 48 hours.

i think this should be backported to gcc-9 too.
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Fix simd attribute handling on aarch64 (version 2)

Steve Ellcey-10
On Wed, 2019-08-07 at 10:40 +0000, Szabolcs Nagy wrote:

> -------------------------------------------
> ---
> On 31/07/2019 08:25, Richard Sandiford wrote:
> > Steve Ellcey <[hidden email]> writes:
> > >
> > > 2019-07-30  Steve Ellcey  <[hidden email]>
> > >
> > > * omp-simd-clone.c (simd_clone_adjust_return_type): Remove call
> > > to
> > > build_distinct_type_copy.
> > > (simd_clone_adjust_argument_types): Ditto.
> > > (simd_clone_adjust): Call build_distinct_type_copy here.
> > > (expand_simd_clones): Ditto.
> > >
> > > 2019-07-30  Steve Ellcey  <[hidden email]>
> > >
> > > * gcc.target/aarch64/simd_pcs_attribute.c: New test.
> > > * gcc.target/aarch64/simd_pcs_attribute-2.c: Ditto.
> > > * gcc.target/aarch64/simd_pcs_attribute-3.c: Ditto.
> >
> > OK if there are no objections in 48 hours.
>
> i think this should be backported to gcc-9 too.

Yes, I agree.  The 9.X branch is frozen right now for the 9.2 release,
I will backport it to the branch after it reopens assuming there are no
objections.

Steve Ellcey
[hidden email]