On Fri, Feb 17, 2017 at 7:49 PM, Jakub Jelinek <ja...@redhat.com> wrote:
> 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)?
Thanks for the detailed analysis. After some thinking, I think that
the above is the correct approach.
> 2017-02-17 Jakub Jelinek <ja...@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.
OK for mainline and eventually branches.
Thanks,
Uros.
> --- 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
