Re: [PATCH, AArch64] Fix for PR67896 (C++ FE cannot distinguish __Poly{8,16,64,128}_t types)

2016-04-01 Thread James Greenhalgh 
On Mon, Jan 25, 2016 at 12:15:48PM +, James Greenhalgh wrote:
> On Wed, Jan 20, 2016 at 09:27:41PM +0100, Roger Ferrer Ibáñez wrote:
> > Hi James,
> > 
> > > This patch looks technically correct to me, though there is a small
> > > style issue to correct (in-line below), and your ChangeLogs don't fit
> > > our usual style.
> > 
> > thank you very much for the useful comments. I'm attaching a new
> > version of the patch with the style issues (hopefully) ironed out.
> 
> Thanks, this version of the patch looks correct to me.
> 
> > > > P.S.: I haven't signed the copyright assignment to the FSF. The change
> > > > is really small but I can do the paperwork if required.
> 
> I can't commit it on your behalf until we've heard back regarding whether
> this needs a copyright assignment to the FSF, but once I've heard I'd
> be happy to commit this for you. I'll expand the CC list a bit further
> to see if we can get an answer on that.
> 
> Thanks again for the analysis and patch.

This patch should also be backported to GCC 5, which has the same bug.

I've done that as r234665 as the merge was clean after a bootstrap and
test cycle on aarch64-none-linux-gnu.

Thanks,
James

Re: [PATCH, AArch64] Fix for PR67896 (C++ FE cannot distinguish __Poly{8,16,64,128}_t types)

2016-01-25 Thread Mike Stump 
On Jan 25, 2016, at 4:15 AM, James Greenhalgh  wrote:
 P.S.: I haven't signed the copyright assignment to the FSF. The change
 is really small but I can do the paperwork if required.
> 
> I can't commit it on your behalf until we've heard back regarding whether
> this needs a copyright assignment to the FSF, but once I've heard I'd
> be happy to commit this for you.

This is fine for the tree without paper work.  Though, if you work on gcc on a 
regular basis and are likely to contribute more work in the future, it is nice 
to get the paper work out of the way for next time.

Re: [PATCH, AArch64] Fix for PR67896 (C++ FE cannot distinguish __Poly{8,16,64,128}_t types)

2016-01-25 Thread James Greenhalgh 
On Wed, Jan 20, 2016 at 09:27:41PM +0100, Roger Ferrer Ibáñez wrote:
> Hi James,
> 
> > This patch looks technically correct to me, though there is a small
> > style issue to correct (in-line below), and your ChangeLogs don't fit
> > our usual style.
> 
> thank you very much for the useful comments. I'm attaching a new
> version of the patch with the style issues (hopefully) ironed out.

Thanks, this version of the patch looks correct to me.

> > > P.S.: I haven't signed the copyright assignment to the FSF. The change
> > > is really small but I can do the paperwork if required.

I can't commit it on your behalf until we've heard back regarding whether
this needs a copyright assignment to the FSF, but once I've heard I'd
be happy to commit this for you. I'll expand the CC list a bit further
to see if we can get an answer on that.

Thanks again for the analysis and patch.

James

> gcc/ChangeLog:
> 
> 2016-01-19  Roger Ferrer Ibáñez  
> 
> PR target/67896
> * config/aarch64/aarch64-builtins.c
> (aarch64_init_simd_builtin_types): Do not set structural
> equality to __Poly{8,16,64,128}_t types.
> 
> gcc/testsuite/ChangeLog:
> 
> 2016-01-19  Roger Ferrer Ibáñez  
> 
> PR target/67896
> * gcc.target/aarch64/simd/pr67896.C: New.
> 
> -- 
> Roger Ferrer Ibáñez

> From 72c065f6a3f9d168baf357de1b567faa6042c03b Mon Sep 17 00:00:00 2001
> From: Roger Ferrer Ibanez 
> Date: Wed, 20 Jan 2016 21:11:42 +0100
> Subject: [PATCH] Do not set structural equality on polynomial types
> 
> ---
>  gcc/config/aarch64/aarch64-builtins.c   | 10 ++
>  gcc/testsuite/gcc.target/aarch64/simd/pr67896.C |  7 +++
>  2 files changed, 13 insertions(+), 4 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/aarch64/simd/pr67896.C
> 
> diff --git a/gcc/config/aarch64/aarch64-builtins.c 
> b/gcc/config/aarch64/aarch64-builtins.c
> index bd7a8dd..40272ed 100644
> --- a/gcc/config/aarch64/aarch64-builtins.c
> +++ b/gcc/config/aarch64/aarch64-builtins.c
> @@ -610,14 +610,16 @@ aarch64_init_simd_builtin_types (void)
>enum machine_mode mode = aarch64_simd_types[i].mode;
>  
>if (aarch64_simd_types[i].itype == NULL)
> - aarch64_simd_types[i].itype =
> -   build_distinct_type_copy
> - (build_vector_type (eltype, GET_MODE_NUNITS (mode)));
> + {
> +   aarch64_simd_types[i].itype
> + = build_distinct_type_copy
> +   (build_vector_type (eltype, GET_MODE_NUNITS (mode)));
> +   SET_TYPE_STRUCTURAL_EQUALITY (aarch64_simd_types[i].itype);
> + }
>  
>tdecl = add_builtin_type (aarch64_simd_types[i].name,
>   aarch64_simd_types[i].itype);
>TYPE_NAME (aarch64_simd_types[i].itype) = tdecl;
> -  SET_TYPE_STRUCTURAL_EQUALITY (aarch64_simd_types[i].itype);
>  }
>  
>  #define AARCH64_BUILD_SIGNED_TYPE(mode)  \
> diff --git a/gcc/testsuite/gcc.target/aarch64/simd/pr67896.C 
> b/gcc/testsuite/gcc.target/aarch64/simd/pr67896.C
> new file mode 100644
> index 000..1f916e0
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/simd/pr67896.C
> @@ -0,0 +1,7 @@
> +typedef __Poly8_t A;
> +typedef __Poly16_t A; /* { dg-error "conflicting declaration" } */
> +typedef __Poly64_t A; /* { dg-error "conflicting declaration" } */
> +typedef __Poly128_t A; /* { dg-error "conflicting declaration" } */
> +
> +typedef __Poly8x8_t B;
> +typedef __Poly16x8_t B; /* { dg-error "conflicting declaration" } */ 
> -- 
> 2.1.4
>

Re: [PATCH, AArch64] Fix for PR67896 (C++ FE cannot distinguish __Poly{8,16,64,128}_t types)

2016-01-20 Thread Roger Ferrer Ibáñez 
Hi James,

> This patch looks technically correct to me, though there is a small
> style issue to correct (in-line below), and your ChangeLogs don't fit
> our usual style.

thank you very much for the useful comments. I'm attaching a new
version of the patch with the style issues (hopefully) ironed out.

Kind regards,

gcc/ChangeLog:

2016-01-19  Roger Ferrer Ibáñez  

PR target/67896
* config/aarch64/aarch64-builtins.c
(aarch64_init_simd_builtin_types): Do not set structural
equality to __Poly{8,16,64,128}_t types.

gcc/testsuite/ChangeLog:

2016-01-19  Roger Ferrer Ibáñez  

PR target/67896
* gcc.target/aarch64/simd/pr67896.C: New.

-- 
Roger Ferrer Ibáñez
From 72c065f6a3f9d168baf357de1b567faa6042c03b Mon Sep 17 00:00:00 2001
From: Roger Ferrer Ibanez 
Date: Wed, 20 Jan 2016 21:11:42 +0100
Subject: [PATCH] Do not set structural equality on polynomial types

---
 gcc/config/aarch64/aarch64-builtins.c   | 10 ++
 gcc/testsuite/gcc.target/aarch64/simd/pr67896.C |  7 +++
 2 files changed, 13 insertions(+), 4 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/simd/pr67896.C

diff --git a/gcc/config/aarch64/aarch64-builtins.c b/gcc/config/aarch64/aarch64-builtins.c
index bd7a8dd..40272ed 100644
--- a/gcc/config/aarch64/aarch64-builtins.c
+++ b/gcc/config/aarch64/aarch64-builtins.c
@@ -610,14 +610,16 @@ aarch64_init_simd_builtin_types (void)
   enum machine_mode mode = aarch64_simd_types[i].mode;
 
   if (aarch64_simd_types[i].itype == NULL)
-	aarch64_simd_types[i].itype =
-	  build_distinct_type_copy
-	(build_vector_type (eltype, GET_MODE_NUNITS (mode)));
+	{
+	  aarch64_simd_types[i].itype
+	= build_distinct_type_copy
+	  (build_vector_type (eltype, GET_MODE_NUNITS (mode)));
+	  SET_TYPE_STRUCTURAL_EQUALITY (aarch64_simd_types[i].itype);
+	}
 
   tdecl = add_builtin_type (aarch64_simd_types[i].name,
 aarch64_simd_types[i].itype);
   TYPE_NAME (aarch64_simd_types[i].itype) = tdecl;
-  SET_TYPE_STRUCTURAL_EQUALITY (aarch64_simd_types[i].itype);
 }
 
 #define AARCH64_BUILD_SIGNED_TYPE(mode)  \
diff --git a/gcc/testsuite/gcc.target/aarch64/simd/pr67896.C b/gcc/testsuite/gcc.target/aarch64/simd/pr67896.C
new file mode 100644
index 000..1f916e0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/simd/pr67896.C
@@ -0,0 +1,7 @@
+typedef __Poly8_t A;
+typedef __Poly16_t A; /* { dg-error "conflicting declaration" } */
+typedef __Poly64_t A; /* { dg-error "conflicting declaration" } */
+typedef __Poly128_t A; /* { dg-error "conflicting declaration" } */
+
+typedef __Poly8x8_t B;
+typedef __Poly16x8_t B; /* { dg-error "conflicting declaration" } */ 
-- 
2.1.4

Re: [PATCH, AArch64] Fix for PR67896 (C++ FE cannot distinguish __Poly{8,16,64,128}_t types)

2016-01-19 Thread James Greenhalgh 
On Tue, Jan 19, 2016 at 06:28:18AM +0100, Roger Ferrer Ibáñez wrote:
> Hi,
> 
> aarch64-builtins.c defines several SIMD builtin types. Among these
> SIMD types there are the polynomials __Poly{8,16,64,128}_t. These are
> built by a call to build_distinct_type_copy
> (unsigned_int{Q,H,D,T}I_type_node), respectively, i.e. they are not
> VECTOR_TYPEs. A later loop, traverses an array containing all the SIMD
> types and skips those types for which a VECTOR_TYPE does not have to
> be built: this is, types __Poly{8,16,64,128}_t. That same loop does
> SET_TYPE_STRUCTURAL_EQUALITY on the newly created vector type, but it
> does this unconditionally, thus setting TYPE_CANONICAL of types
> __Poly{8,16,64,128}_t to NULL.
> 
> The net effect of this is that the C++ FE is unable to distinguish
> between all __Poly{8,16,64,128}_t and between vector types with the
> same number of elements but different polynomial type as element type
> (like __Poly8x8_t vs __Poly16x8_t). Note that sizeof (correctly)
> returns different values for all these types. This patch simply
> protects SET_TYPE_STRUCTURAL_EQUALITY inside the branch that creates
> the vector type.
> 
> I have bootstrapped and regression tested this on a small board
> aarch64-unknown-linux-gnu host without new regressions.

This patch looks technically correct to me, though there is a small
style issue to correct (in-line below), and your ChangeLogs don't fit
our usual style.

> P.S.: I haven't signed the copyright assignment to the FSF. The change
> is really small but I can do the paperwork if required.

I have no experience with making the call as to which sort of change
is "small enough" to include in GCC without copyright assignment, so
I've CC'ed the AArch64 maintainers, who may be able to give more advice.

> gcc/ChangeLog:
> 
> 2016-01-19 Roger Ferrer Ibáñez 

Two spaces between the date and your name, and between your name and your
email, as so:

2016-01-19  Roger Ferrer Ibáñez  


> 
> PR aarch64/67896

PR target/67896


> * aarch64-builtins.c (aarch64_init_simd_builtin_types): Do not set

This should be the path from the directory in which the ChangeLog resides:

* config/aarch64/aarch64-builtins.c

And a full-stop at the end of the line. So this whole entry would look like:

2016-01-19  Roger Ferrer Ibáñez  

PR target/67896
* config/aarch64/aarch64-builtins.c
(aarch64_init_simd_builtin_types): Do not set structural
equality to __Poly{8,16,64,128}_t types.

> structural equality to __Poly{8,16,64,128}_t types
> 
> gcc/testsuite/ChangeLog:
> 
> 2016-01-19 Roger Ferrer Ibáñez 
> 
> PR aarch64/67896
> * pr67896.C: New test

Same comments apply here:

2016-01-19 Roger Ferrer Ibáñez 
 
PR target/67896
* gcc.target/aarch64/simd/pr67896.C: New.

> diff --git a/gcc/config/aarch64/aarch64-builtins.c 
> b/gcc/config/aarch64/aarch64-builtins.c
> index bd7a8dd..edacf10 100644
> --- a/gcc/config/aarch64/aarch64-builtins.c
> +++ b/gcc/config/aarch64/aarch64-builtins.c
> @@ -610,14 +610,16 @@ aarch64_init_simd_builtin_types (void)
>enum machine_mode mode = aarch64_simd_types[i].mode;
>  
>if (aarch64_simd_types[i].itype == NULL)
> - aarch64_simd_types[i].itype =
> -   build_distinct_type_copy
> + {
> +   aarch64_simd_types[i].itype =
> + build_distinct_type_copy
>   (build_vector_type (eltype, GET_MODE_NUNITS (mode)));

I'd rewrap this as:

  aarch64_simd_types[i].itype
= build_distinct_type_copy
  (build_vector_type (eltype, GET_MODE_NUNITS (mode)));

Thanks for the patch!
James

> +   SET_TYPE_STRUCTURAL_EQUALITY (aarch64_simd_types[i].itype);
> + }
>  
>tdecl = add_builtin_type (aarch64_simd_types[i].name,
>   aarch64_simd_types[i].itype);
>TYPE_NAME (aarch64_simd_types[i].itype) = tdecl;
> -  SET_TYPE_STRUCTURAL_EQUALITY (aarch64_simd_types[i].itype);
>  }
>  
>  #define AARCH64_BUILD_SIGNED_TYPE(mode)  \
> diff --git a/gcc/testsuite/gcc.target/aarch64/simd/pr67896.C 
> b/gcc/testsuite/gcc.target/aarch64/simd/pr67896.C
> new file mode 100644
> index 000..1f916e0
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/simd/pr67896.C
> @@ -0,0 +1,7 @@
> +typedef __Poly8_t A;
> +typedef __Poly16_t A; /* { dg-error "conflicting declaration" } */
> +typedef __Poly64_t A; /* { dg-error "conflicting declaration" } */
> +typedef __Poly128_t A; /* { dg-error "conflicting declaration" } */
> +
> +typedef __Poly8x8_t B;
> +typedef __Poly16x8_t B; /* { dg-error "conflicting declaration" } */ 
> -- 
> 2.1.4
>