diff mbox series

[rs6000] Undefine vector, bool, pixel in xmmintrin.h

Message ID db4f94be-b2de-504c-b458-133a85248902@linux.ibm.com
State New
Headers show
Series [rs6000] Undefine vector, bool, pixel in xmmintrin.h | expand

Commit Message

Bill Schmidt April 2, 2018, 1:24 a.m. UTC 
Hi,

The xmmintrin.h and related header files make use of altivec.h to enable
vector intrinsics to be used for compatibility.  However, for C++ or C11
using strict ANSI compliance, this causes a collision with the "bool"
keyword.  We intend to later change altivec.h to not define these keywords
under those conditions, but this could cause a certain amount of community
churn, so we want to try a distro build with this before making such a
change in GCC 9.  Since these compatibility headers are new in GCC 8, we 
can safely undefine these following the include of altivec.h without
disturbing existing code.

Note that for strict-ANSI C11 code, stdbool.h must be included after
including xmmintrin.h in order to enable the bool keyword.  When we later
fix altivec.h to not define them in the first place, this ordering
requirement will no longer exist.

While testing, I discovered a couple stray instances of "vector" that
should have been "__vector" in these headers.  Fixes for those are
included in this patch.

I also updated the gcc.target/powerpc/powerpc.exp file to allow C++
tests to be placed in that directory (with a *.C suffix).

Bootstrapped and tested on powerpc64le-linux-gnu with no regressions.
Is this okay for trunk?

Thanks,
Bill


[gcc]

2018-04-02  Bill Schmidt  <wschmidt@linux.ibm.com>

	* config/rs6000/emmintrin.h (_mm_cvtpd_epi32): Use __vector rather
	than vector.
	(_mm_cvtpd_ps): Likewise.
	(_mm_cvttpd_epi32): Likewise.
	* config/rs6000/mmintrin.h (_mm_unpacklo_pi8): Likewise.
	* config/rs6000/xmmintrin.h: For strict-ANSI C++ or C11, undefine
	vector, pixel, and bool following altivec.h include.

[gcc/testsuite]

2018-04-02  Bill Schmidt  <wschmidt@linux.ibm.com>

	* gcc.target/powerpc/powerpc.exp: Add .C suffix for main loop.
	* gcc.target/powerpc/undef-bool.C: New file.
	* gcc.target/powerpc/undef-bool.c: New file.

Comments

Segher Boessenkool April 3, 2018, 9:49 a.m. UTC | #1 
Hi Bill,

On Sun, Apr 01, 2018 at 08:24:32PM -0500, Bill Schmidt wrote:
> 	* gcc.target/powerpc/undef-bool.C: New file.
> 	* gcc.target/powerpc/undef-bool.c: New file.

I'm not sure two files with the same basename will not cause problems.
In either case it is confusing.  Please rename one?

> --- gcc/config/rs6000/xmmintrin.h	(revision 258958)
> +++ gcc/config/rs6000/xmmintrin.h	(working copy)
> @@ -58,6 +58,18 @@
>  #define _XMMINTRIN_H_INCLUDED
>  
>  #include <altivec.h>
> +
> +/* Avoid collisions between altivec.h and strict adherence to C++ and
> +   C11 standards.  This should eventually be done inside altivec.h itself,
> +   but only after testing a full distro build.  */
> +#if defined(__STRICT_ANSI__) && (defined(__cplusplus) || \
> +				 (defined(__STDC_VERSION__) &&	\
> +				  __STDC_VERSION__ >= 201112L))
> +#undef vector
> +#undef pixel
> +#undef bool
> +#endif

This won't work if anything made a macro for bool (etc.) before including
this header.  I guess we can live with that for now (it will not work
without your patch either).

Okay for trunk (with the testcase name fix).  Thanks!


Segher
Jakub Jelinek April 4, 2018, 8:53 a.m. UTC | #2 
On Sun, Apr 01, 2018 at 08:24:32PM -0500, Bill Schmidt wrote:
> I also updated the gcc.target/powerpc/powerpc.exp file to allow C++
> tests to be placed in that directory (with a *.C suffix).

I think this is wrong.
Historically, we've been putting target C++ tests into g++.dg/*/*.C
with appropriate target guards, the gcc.target/ tests are compiled with the
gcc driver rather than g++, so in your case it just happens to work because
it is a compile time test only (and one that doesn't use any STL headers),
e.g. dg-do run test would fail miserably.

If we (for GCC9?) want to create a spot for target C++ tests, we should
just add g++.target/<cpu>/ directories and add all the needed infrastructure
for those.

Can you please revert the powerpc.exp change and move the test to
g++.dg/ext/ ?  Thanks.

	Jakub
Bill Schmidt April 4, 2018, 8:47 p.m. UTC | #3 
> On Apr 4, 2018, at 3:53 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Sun, Apr 01, 2018 at 08:24:32PM -0500, Bill Schmidt wrote:
>> I also updated the gcc.target/powerpc/powerpc.exp file to allow C++
>> tests to be placed in that directory (with a *.C suffix).
> 
> I think this is wrong.
> Historically, we've been putting target C++ tests into g++.dg/*/*.C
> with appropriate target guards, the gcc.target/ tests are compiled with the
> gcc driver rather than g++, so in your case it just happens to work because
> it is a compile time test only (and one that doesn't use any STL headers),
> e.g. dg-do run test would fail miserably.
> 
> If we (for GCC9?) want to create a spot for target C++ tests, we should
> just add g++.target/<cpu>/ directories and add all the needed infrastructure
> for those.
> 
> Can you please revert the powerpc.exp change and move the test to
> g++.dg/ext/ ?  Thanks.

Sure, I can -- I just want to point out that there is precedent here.  I noticed
that gcc.target/s390/s390.exp allows .C suffixes and there is one such
(compile-only) test in that directory, so I assumed the framework was okay
for this.

Thanks,
Bill

> 
> 	Jakub
>
Jakub Jelinek April 4, 2018, 8:51 p.m. UTC | #4 
On Wed, Apr 04, 2018 at 03:47:18PM -0500, Bill Schmidt wrote:
> > If we (for GCC9?) want to create a spot for target C++ tests, we should
> > just add g++.target/<cpu>/ directories and add all the needed infrastructure
> > for those.
> > 
> > Can you please revert the powerpc.exp change and move the test to
> > g++.dg/ext/ ?  Thanks.
> 
> Sure, I can -- I just want to point out that there is precedent here.  I noticed

Thanks.

> that gcc.target/s390/s390.exp allows .C suffixes and there is one such
> (compile-only) test in that directory, so I assumed the framework was okay
> for this.

Yes, and apparently aarch64 and arm have a couple of *.C tests too.
Still it doesn't feel right, running C++ tests under make check-gcc rather
than check-g++ is just weird.

I think we should just introduce g++.target/ in GCC9 and move those tests
there, plus any g++.dg/ tests guarded for single targets only.  g++.target
should do the -std=c++{98,11,14,17,2a} cycling etc.

	Jakub
Bill Schmidt April 4, 2018, 8:54 p.m. UTC | #5 
On Apr 4, 2018, at 3:51 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> On Wed, Apr 04, 2018 at 03:47:18PM -0500, Bill Schmidt wrote:
>>> If we (for GCC9?) want to create a spot for target C++ tests, we should
>>> just add g++.target/<cpu>/ directories and add all the needed infrastructure
>>> for those.
>>> 
>>> Can you please revert the powerpc.exp change and move the test to
>>> g++.dg/ext/ ?  Thanks.
>> 
>> Sure, I can -- I just want to point out that there is precedent here.  I noticed
> 
> Thanks.
> 
>> that gcc.target/s390/s390.exp allows .C suffixes and there is one such
>> (compile-only) test in that directory, so I assumed the framework was okay
>> for this.
> 
> Yes, and apparently aarch64 and arm have a couple of *.C tests too.
> Still it doesn't feel right, running C++ tests under make check-gcc rather
> than check-g++ is just weird.
> 
> I think we should just introduce g++.target/ in GCC9 and move those tests
> there, plus any g++.dg/ tests guarded for single targets only.  g++.target
> should do the -std=c++{98,11,14,17,2a} cycling etc.

Agreed.  I'll take a note about this for GCC9.

Thanks,
Bill
diff mbox series

Patch

Index: gcc/config/rs6000/emmintrin.h
===================================================================
--- gcc/config/rs6000/emmintrin.h	(revision 258958)
+++ gcc/config/rs6000/emmintrin.h	(working copy)
@@ -885,7 +885,7 @@  _mm_cvtpd_epi32 (__m128d __A)
 
 #ifdef _ARCH_PWR8
   temp = vec_mergeo (temp, temp);
-  result = (__v4si)vec_vpkudum ((vector long)temp, (vector long)vzero);
+  result = (__v4si)vec_vpkudum ((__vector long)temp, (__vector long)vzero);
 #else
   {
     const __v16qu pkperm = {0x00, 0x01, 0x02, 0x03, 0x08, 0x09, 0x0a, 0x0b,
@@ -919,7 +919,7 @@  _mm_cvtpd_ps (__m128d __A)
 
 #ifdef _ARCH_PWR8
   temp = vec_mergeo (temp, temp);
-  result = (__v4sf)vec_vpkudum ((vector long)temp, (vector long)vzero);
+  result = (__v4sf)vec_vpkudum ((__vector long)temp, (__vector long)vzero);
 #else
   {
     const __v16qu pkperm = {0x00, 0x01, 0x02, 0x03, 0x08, 0x09, 0x0a, 0x0b,
@@ -947,7 +947,7 @@  _mm_cvttpd_epi32 (__m128d __A)
 
 #ifdef _ARCH_PWR8
   temp = vec_mergeo (temp, temp);
-  result = (__v4si)vec_vpkudum ((vector long)temp, (vector long)vzero);
+  result = (__v4si)vec_vpkudum ((__vector long)temp, (__vector long)vzero);
 #else
   {
     const __v16qu pkperm = {0x00, 0x01, 0x02, 0x03, 0x08, 0x09, 0x0a, 0x0b,
Index: gcc/config/rs6000/mmintrin.h
===================================================================
--- gcc/config/rs6000/mmintrin.h	(revision 258958)
+++ gcc/config/rs6000/mmintrin.h	(working copy)
@@ -317,7 +317,7 @@  _mm_unpacklo_pi8 (__m64 __m1, __m64 __m2)
   a = (__vector unsigned char)vec_splats (__m1);
   b = (__vector unsigned char)vec_splats (__m2);
   c = vec_mergel (a, b);
-  return (__builtin_unpack_vector_int128 ((vector __int128_t)c, 1));
+  return (__builtin_unpack_vector_int128 ((__vector __int128_t)c, 1));
 #else
   __m64_union m1, m2, res;
 
Index: gcc/config/rs6000/xmmintrin.h
===================================================================
--- gcc/config/rs6000/xmmintrin.h	(revision 258958)
+++ gcc/config/rs6000/xmmintrin.h	(working copy)
@@ -58,6 +58,18 @@ 
 #define _XMMINTRIN_H_INCLUDED
 
 #include <altivec.h>
+
+/* Avoid collisions between altivec.h and strict adherence to C++ and
+   C11 standards.  This should eventually be done inside altivec.h itself,
+   but only after testing a full distro build.  */
+#if defined(__STRICT_ANSI__) && (defined(__cplusplus) || \
+				 (defined(__STDC_VERSION__) &&	\
+				  __STDC_VERSION__ >= 201112L))
+#undef vector
+#undef pixel
+#undef bool
+#endif
+
 #include <assert.h>
 
 /* We need type definitions from the MMX header file.  */
Index: gcc/testsuite/gcc.target/powerpc/powerpc.exp
===================================================================
--- gcc/testsuite/gcc.target/powerpc/powerpc.exp	(revision 258958)
+++ gcc/testsuite/gcc.target/powerpc/powerpc.exp	(working copy)
@@ -35,7 +35,7 @@  if ![info exists DEFAULT_CFLAGS] then {
 dg-init
 
 # Main loop.
-dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cS\]]] \
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cCS\]]] \
 	"" $DEFAULT_CFLAGS
 
 set SAVRES_TEST_OPTS [list -Os -O2 {-Os -mno-multiple} {-O2 -mno-multiple}]
Index: gcc/testsuite/gcc.target/powerpc/undef-bool.C
===================================================================
--- gcc/testsuite/gcc.target/powerpc/undef-bool.C	(nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/undef-bool.C	(working copy)
@@ -0,0 +1,13 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -std=c++11 -DNO_WARN_X86_INTRINSICS" } */
+
+/* Test to ensure that "bool" gets undef'd in xmmintrin.h when
+   we require strict ANSI.  */
+
+#include <xmmintrin.h>
+
+bool foo (int x)
+{
+  return x == 2;
+}
+
Index: gcc/testsuite/gcc.target/powerpc/undef-bool.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/undef-bool.c	(nonexistent)
+++ gcc/testsuite/gcc.target/powerpc/undef-bool.c	(working copy)
@@ -0,0 +1,15 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -std=c11 -DNO_WARN_X86_INTRINSICS" } */
+
+/* Test to ensure that "bool" gets undef'd in xmmintrin.h when
+   we require strict ANSI.  Subsequent use of bool needs stdbool.h.
+   altivec.h should eventually avoid defining bool, vector, and
+   pixel, following distro testing.  */
+
+#include <xmmintrin.h>
+
+bool foo (int x) /* { dg-error "unknown type name 'bool'; did you mean '_Bool'?" } */
+{
+  return x == 2;
+}
+