This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Fix various issues with x86 builtins (PR target/79568)


Hi!

The masks for builtins seems to be quite messy.  Most of
the builtins have a single OPTION_MASK_ISA_* in there and that is
clear (i.e. that the builtin is only enabled/usable if that isa
bit is on).  Then we have 0 and that is meant for always enabled builtins.
Then there is OPTION_MASK_ISA_xxx | OPTION_MASK_ISA_64BIT and that
means (according to def_builtin code and comments) that we enable
only if TARGET_64BIT and the rest without the  | OPTION_MASK_ISA_64BIT
needs to be satisfied.  Then there is
OPTION_MASK_ISA_xxx | OPTION_MASK_ISA_AVX512VL and that is again
in def_builtin code and comments documented to be only enabled if
-mavx512vl and the rest without the | OPTION_MASK_ISA_AVX512VL
needs to be satisfied.  Then we have
OPTION_MASK_ISA_FMA | OPTION_MASK_ISA_FMA4
OPTION_MASK_ISA_SSE4_2 | OPTION_MASK_ISA_CRC32
OPTION_MASK_ISA_SSE | OPTION_MASK_ISA_3DNOW_A
def_builtin here as well as ix86_expand_builtin suggest here that it
is either SSE or 3dNOWa etc. (I believe at least the last one
is meant to be that, no idea about the other two).
And finally various builtins use ~OPTION_MASK_ISA_64BIT, which I have
no idea what people adding those really meant to express.  At least
for e.g. __builtin_ia32_pause I'd expect it wants to be enabled
everywhere (that is 0 though), while what both def_builtin and
ix86_expand_builtin actually implement for ~OPTION_MASK_ISA_64BIT
is that it is enabled whenever any ISA (other than OPTION_MASK_ISA_64BIT)
is set and disabled otherwise.  So, e.g. -m32 -march=i386 -mno-sahf -mno-mmx
-mno-sse disables __builtin_ia32_pause, __builtin_ia32_rdtsc,
__builtin_ia32_rolhi etc.

For OPTION_MASK_ISA_64BIT and OPTION_MASK_ISA_AVX512VL, while
def_builtin implements something documented (and what makes sense),
ix86_expand_builtin actually takes it as any of the ISAs enabled,
so if you manage to define the builtin earlier (e.g. through including
x86intrin.h), then one can use
OPTION_MASK_ISA_AVX512BW | OPTION_MASK_ISA_AVX512VL
builtin (meant to be enabled if both are on) in a function where just one
of them is on (-mavx512bw or -mavx512vl), if both aren't on, that will ICE.
Similarly, for OPTION_MASK_ISA_LWP | OPTION_MASK_ISA_64BIT, if the
builtin is defined earlier, it will be enabled even in -mno-lwp -m64
function and ICE (not for -m32 -mlwp, because -m64 is a global TU option
and so nothing will define the builtin).

This patch attempts to resolve it by changing all ~OPTION_MASK_ISA_64BIT
builtins to 0, handling xxx | OPTION_MASK_ISA_64BIT
as must satisfy TARGET_64BIT and xxx, handling yyy |
OPTION_MASK_ISA_AVX512VL as must satisfy -mavx512vl and yyy and for the
rest, if 0, enabling always, otherwise requiring at least one of the ISAs
enabled from the bitmask.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Or do we want a different behavior (then what and why)?

2017-02-17  Jakub Jelinek  <jakub@redhat.com>

	PR target/79568
	* config/i386/i386.c (ix86_expand_builtin): Handle
	OPTION_MASK_ISA_AVX512VL and OPTION_MASK_ISA_64BIT in
	ix86_builtins_isa[fcode].isa as a requirement of those
	flags and any other flag in the bitmask.
	(ix86_init_mmx_sse_builtins): Use 0 instead of
	~OPTION_MASK_ISA_64BIT as mask.
	* config/i386/i386-builtin.def (__builtin_ia32_rdtsc,
	__builtin_ia32_rdtscp, __builtin_ia32_pause, __builtin_ia32_bsrsi,
	__builtin_ia32_rdpmc, __builtin_ia32_rolqi, __builtin_ia32_rolhi,
	__builtin_ia32_rorqi, __builtin_ia32_rorhi): Likewise.

	* gcc.target/i386/pr79568-1.c: New test.
	* gcc.target/i386/pr79568-2.c: New test.
	* gcc.target/i386/pr79568-3.c: New test.

--- gcc/config/i386/i386.c.jj	2017-02-17 11:11:27.000000000 +0100
+++ gcc/config/i386/i386.c	2017-02-17 17:10:07.899194674 +0100
@@ -32075,11 +32075,11 @@ ix86_init_mmx_sse_builtins (void)
 	       IX86_BUILTIN_SBB64);
 
   /* Read/write FLAGS.  */
-  def_builtin (~OPTION_MASK_ISA_64BIT, "__builtin_ia32_readeflags_u32",
+  def_builtin (0, "__builtin_ia32_readeflags_u32",
                UNSIGNED_FTYPE_VOID, IX86_BUILTIN_READ_FLAGS);
   def_builtin (OPTION_MASK_ISA_64BIT, "__builtin_ia32_readeflags_u64",
                UINT64_FTYPE_VOID, IX86_BUILTIN_READ_FLAGS);
-  def_builtin (~OPTION_MASK_ISA_64BIT, "__builtin_ia32_writeeflags_u32",
+  def_builtin (0, "__builtin_ia32_writeeflags_u32",
                VOID_FTYPE_UNSIGNED, IX86_BUILTIN_WRITE_FLAGS);
   def_builtin (OPTION_MASK_ISA_64BIT, "__builtin_ia32_writeeflags_u64",
                VOID_FTYPE_UINT64, IX86_BUILTIN_WRITE_FLAGS);
@@ -36723,9 +36723,18 @@ ix86_expand_builtin (tree exp, rtx targe
      Originally the builtin was not created if it wasn't applicable to the
      current ISA based on the command line switches.  With function specific
      options, we need to check in the context of the function making the call
-     whether it is supported.  */
-  if ((ix86_builtins_isa[fcode].isa
-       && !(ix86_builtins_isa[fcode].isa & ix86_isa_flags))
+     whether it is supported.  Treat AVX512VL specially.  For other flags,
+     if isa includes more than one ISA bit, treat those are requiring any
+     of them.  For AVX512VL, require both AVX512VL and the non-AVX512VL
+     ISAs.  Similarly for 64BIT, but we shouldn't be building such builtins
+     at all, -m64 is a whole TU option.  */
+  if (((ix86_builtins_isa[fcode].isa
+	& ~(OPTION_MASK_ISA_AVX512VL | OPTION_MASK_ISA_64BIT))
+       && !(ix86_builtins_isa[fcode].isa
+	    & ~(OPTION_MASK_ISA_AVX512VL | OPTION_MASK_ISA_64BIT)
+	    & ix86_isa_flags))
+      || ((ix86_builtins_isa[fcode].isa & OPTION_MASK_ISA_AVX512VL)
+	  && !(ix86_isa_flags & OPTION_MASK_ISA_AVX512VL))
       || (ix86_builtins_isa[fcode].isa2
 	  && !(ix86_builtins_isa[fcode].isa2 & ix86_isa_flags2)))
     {
--- gcc/config/i386/i386-builtin.def.jj	2017-01-26 13:22:55.000000000 +0100
+++ gcc/config/i386/i386-builtin.def	2017-02-17 17:07:20.771407936 +0100
@@ -88,9 +88,9 @@ BDESC_END (PCMPISTR, SPECIAL_ARGS)
 
 /* Special builtins with variable number of arguments.  */
 BDESC_FIRST (special_args, SPECIAL_ARGS,
-       ~OPTION_MASK_ISA_64BIT, CODE_FOR_nothing, "__builtin_ia32_rdtsc", IX86_BUILTIN_RDTSC, UNKNOWN, (int) UINT64_FTYPE_VOID)
-BDESC (~OPTION_MASK_ISA_64BIT, CODE_FOR_nothing, "__builtin_ia32_rdtscp", IX86_BUILTIN_RDTSCP, UNKNOWN, (int) UINT64_FTYPE_PUNSIGNED)
-BDESC (~OPTION_MASK_ISA_64BIT, CODE_FOR_pause, "__builtin_ia32_pause", IX86_BUILTIN_PAUSE, UNKNOWN, (int) VOID_FTYPE_VOID)
+       0, CODE_FOR_nothing, "__builtin_ia32_rdtsc", IX86_BUILTIN_RDTSC, UNKNOWN, (int) UINT64_FTYPE_VOID)
+BDESC (0, CODE_FOR_nothing, "__builtin_ia32_rdtscp", IX86_BUILTIN_RDTSCP, UNKNOWN, (int) UINT64_FTYPE_PUNSIGNED)
+BDESC (0, CODE_FOR_pause, "__builtin_ia32_pause", IX86_BUILTIN_PAUSE, UNKNOWN, (int) VOID_FTYPE_VOID)
 
 /* 80387 (for use internally for atomic compound assignment).  */
 BDESC (0, CODE_FOR_fnstenv, "__builtin_ia32_fnstenv", IX86_BUILTIN_FNSTENV, UNKNOWN, (int) VOID_FTYPE_PVOID)
@@ -385,13 +385,13 @@ BDESC_END (SPECIAL_ARGS, ARGS)
 
 /* Builtins with variable number of arguments.  */
 BDESC_FIRST (args, ARGS,
-       ~OPTION_MASK_ISA_64BIT, CODE_FOR_bsr, "__builtin_ia32_bsrsi", IX86_BUILTIN_BSRSI, UNKNOWN, (int) INT_FTYPE_INT)
+       0, CODE_FOR_bsr, "__builtin_ia32_bsrsi", IX86_BUILTIN_BSRSI, UNKNOWN, (int) INT_FTYPE_INT)
 BDESC (OPTION_MASK_ISA_64BIT, CODE_FOR_bsr_rex64, "__builtin_ia32_bsrdi", IX86_BUILTIN_BSRDI, UNKNOWN, (int) INT64_FTYPE_INT64)
-BDESC (~OPTION_MASK_ISA_64BIT, CODE_FOR_nothing, "__builtin_ia32_rdpmc", IX86_BUILTIN_RDPMC, UNKNOWN, (int) UINT64_FTYPE_INT)
-BDESC (~OPTION_MASK_ISA_64BIT, CODE_FOR_rotlqi3, "__builtin_ia32_rolqi", IX86_BUILTIN_ROLQI, UNKNOWN, (int) UINT8_FTYPE_UINT8_INT)
-BDESC (~OPTION_MASK_ISA_64BIT, CODE_FOR_rotlhi3, "__builtin_ia32_rolhi", IX86_BUILTIN_ROLHI, UNKNOWN, (int) UINT16_FTYPE_UINT16_INT)
-BDESC (~OPTION_MASK_ISA_64BIT, CODE_FOR_rotrqi3, "__builtin_ia32_rorqi", IX86_BUILTIN_RORQI, UNKNOWN, (int) UINT8_FTYPE_UINT8_INT)
-BDESC (~OPTION_MASK_ISA_64BIT, CODE_FOR_rotrhi3, "__builtin_ia32_rorhi", IX86_BUILTIN_RORHI, UNKNOWN, (int) UINT16_FTYPE_UINT16_INT)
+BDESC (0, CODE_FOR_nothing, "__builtin_ia32_rdpmc", IX86_BUILTIN_RDPMC, UNKNOWN, (int) UINT64_FTYPE_INT)
+BDESC (0, CODE_FOR_rotlqi3, "__builtin_ia32_rolqi", IX86_BUILTIN_ROLQI, UNKNOWN, (int) UINT8_FTYPE_UINT8_INT)
+BDESC (0, CODE_FOR_rotlhi3, "__builtin_ia32_rolhi", IX86_BUILTIN_ROLHI, UNKNOWN, (int) UINT16_FTYPE_UINT16_INT)
+BDESC (0, CODE_FOR_rotrqi3, "__builtin_ia32_rorqi", IX86_BUILTIN_RORQI, UNKNOWN, (int) UINT8_FTYPE_UINT8_INT)
+BDESC (0, CODE_FOR_rotrhi3, "__builtin_ia32_rorhi", IX86_BUILTIN_RORHI, UNKNOWN, (int) UINT16_FTYPE_UINT16_INT)
 
 /* MMX */
 BDESC (OPTION_MASK_ISA_MMX, CODE_FOR_mmx_addv8qi3, "__builtin_ia32_paddb", IX86_BUILTIN_PADDB, UNKNOWN, (int) V8QI_FTYPE_V8QI_V8QI)
--- gcc/testsuite/gcc.target/i386/pr79568-1.c.jj	2017-02-17 13:15:54.801490187 +0100
+++ gcc/testsuite/gcc.target/i386/pr79568-1.c	2017-02-17 13:12:05.000000000 +0100
@@ -0,0 +1,18 @@
+/* PR target/79568 */
+/* { dg-do compile } */
+/* { dg-options "-mno-avx512vl -mavx512bw -O2" } */
+
+#pragma GCC push_options
+#pragma GCC target ("avx512vl,avx512bw")
+void
+foo (char *x, char __attribute__ ((__vector_size__(32))) *y, int z)
+{
+  __builtin_ia32_storedquqi256_mask (x, *y, z);
+}
+#pragma GCC pop_options
+
+void
+bar (char *x, char __attribute__ ((__vector_size__(32))) *y, int z)
+{
+  __builtin_ia32_storedquqi256_mask (x, *y, z); /* { dg-error "needs isa option" } */
+}
--- gcc/testsuite/gcc.target/i386/pr79568-2.c.jj	2017-02-17 13:15:58.252443786 +0100
+++ gcc/testsuite/gcc.target/i386/pr79568-2.c	2017-02-17 13:12:31.000000000 +0100
@@ -0,0 +1,18 @@
+/* PR target/79568 */
+/* { dg-do compile { target lp64 } } */
+/* { dg-options "-mno-lwp" } */
+
+#pragma GCC push_options
+#pragma GCC target ("lwp")
+void
+foo (unsigned long x, unsigned int y)
+{
+  __builtin_ia32_lwpval64 (x, y, 1);
+}
+#pragma GCC pop_options
+
+void
+bar (unsigned long x, unsigned int y)
+{
+  __builtin_ia32_lwpval64 (x, y, 1);	 /* { dg-error "needs isa option" } */
+}
--- gcc/testsuite/gcc.target/i386/pr79568-3.c.jj	2017-02-17 17:13:49.066265772 +0100
+++ gcc/testsuite/gcc.target/i386/pr79568-3.c	2017-02-17 17:05:49.000000000 +0100
@@ -0,0 +1,19 @@
+/* PR target/79568 */
+/* { dg-do compile } */
+/* { dg-options "-mno-sahf -mno-mmx -mno-sse" } */
+/* { dg-additional-options "-march=i386" { target ia32 } } */
+
+#pragma GCC push_options
+#pragma GCC target ("sse")
+void
+foo (void)
+{
+  __builtin_ia32_pause ();
+}
+#pragma GCC pop_options
+
+void
+bar (void)
+{
+  __builtin_ia32_pause ();
+}

	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]