Fix up arch= handling in x86 target attribute (PR target/90867)
diff mbox series

Message ID 20191119235347.GR4650@tucnak
State New
Headers show
Series
  • Fix up arch= handling in x86 target attribute (PR target/90867)
Related show

Commit Message

Jakub Jelinek Nov. 19, 2019, 11:53 p.m. UTC
Hi!

The arch= handling in target attribute right now clears almost all isa_flags
and all isa_flags2, so that later call to ix86_option_override_internal
can set just the isa options for the specific arch and nothing else.
Unfortunately, it doesn't work, because next to the ix86_isa_flags{,2}
we have also ix86_isa_flags{,2}_explicit bitmask and in
ix86_option_override_internal and say for arch=x86_64 will not try to
set sse2 isa when we cleared it in ix86_isa_flags but kept it set in
ix86_isa_flags_explicit.
So, the testcase works fine with -O2, but doesn't work with -O2 -msse2,
in the former case ix86_isa_flags_explicit doesn't have MASK_SSE2 bit set,
but in the latter case it does, so in the former case we end up with
MASK_SSE2 in ix86_isa_flags, in the latter not in the function with target
attribute.

The following patch thus clears both ix86_isa_flags{,2} and corresponding
ix86_isa_flags{,2}_explicit.  Also, so that say
target ("arch=x86_64", "avx") works, the clearing is done when actually
seeing the arch=, not at the end.  target ("avx", "arch=x86_64") will still
not enable avx, like before, though.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2019-11-19  Jakub Jelinek  <jakub@redhat.com>

	PR target/90867
	* config/i386/i386-options.c (ix86_valid_target_attribute_tree): Don't
	clear opts->x_ix86_isa_flags{,2} here...
	(ix86_valid_target_attribute_inner_p): ... but here when seeing
	arch=.  Also clear opts->x_ix86_isa_flags{,2}_explicit.

	* gcc.target/i386/pr90867.c: New test.


	Jakub

Comments

Uros Bizjak Nov. 20, 2019, 6:56 a.m. UTC | #1
On Wed, Nov 20, 2019 at 12:53 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> The arch= handling in target attribute right now clears almost all isa_flags
> and all isa_flags2, so that later call to ix86_option_override_internal
> can set just the isa options for the specific arch and nothing else.
> Unfortunately, it doesn't work, because next to the ix86_isa_flags{,2}
> we have also ix86_isa_flags{,2}_explicit bitmask and in
> ix86_option_override_internal and say for arch=x86_64 will not try to
> set sse2 isa when we cleared it in ix86_isa_flags but kept it set in
> ix86_isa_flags_explicit.
> So, the testcase works fine with -O2, but doesn't work with -O2 -msse2,
> in the former case ix86_isa_flags_explicit doesn't have MASK_SSE2 bit set,
> but in the latter case it does, so in the former case we end up with
> MASK_SSE2 in ix86_isa_flags, in the latter not in the function with target
> attribute.
>
> The following patch thus clears both ix86_isa_flags{,2} and corresponding
> ix86_isa_flags{,2}_explicit.  Also, so that say
> target ("arch=x86_64", "avx") works, the clearing is done when actually
> seeing the arch=, not at the end.  target ("avx", "arch=x86_64") will still
> not enable avx, like before, though.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2019-11-19  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/90867
>         * config/i386/i386-options.c (ix86_valid_target_attribute_tree): Don't
>         clear opts->x_ix86_isa_flags{,2} here...
>         (ix86_valid_target_attribute_inner_p): ... but here when seeing
>         arch=.  Also clear opts->x_ix86_isa_flags{,2}_explicit.
>
>         * gcc.target/i386/pr90867.c: New test.

OK, also for backports.

Thanks,
Uros.

> --- gcc/config/i386/i386-options.c.jj   2019-11-18 12:07:54.672405129 +0100
> +++ gcc/config/i386/i386-options.c      2019-11-19 18:43:36.426606458 +0100
> @@ -1147,7 +1147,25 @@ ix86_valid_target_attribute_inner_p (tre
>               ret = false;
>             }
>           else
> -           p_strings[opt] = xstrdup (p + opt_len);
> +           {
> +             p_strings[opt] = xstrdup (p + opt_len);
> +             if (opt == IX86_FUNCTION_SPECIFIC_ARCH)
> +               {
> +                 /* If arch= is set,  clear all bits in x_ix86_isa_flags,
> +                    except for ISA_64BIT, ABI_64, ABI_X32, and CODE16
> +                    and all bits in x_ix86_isa_flags2.  */
> +                 opts->x_ix86_isa_flags &= (OPTION_MASK_ISA_64BIT
> +                                            | OPTION_MASK_ABI_64
> +                                            | OPTION_MASK_ABI_X32
> +                                            | OPTION_MASK_CODE16);
> +                 opts->x_ix86_isa_flags_explicit &= (OPTION_MASK_ISA_64BIT
> +                                                     | OPTION_MASK_ABI_64
> +                                                     | OPTION_MASK_ABI_X32
> +                                                     | OPTION_MASK_CODE16);
> +                 opts->x_ix86_isa_flags2 = 0;
> +                 opts->x_ix86_isa_flags2_explicit = 0;
> +               }
> +           }
>         }
>
>        else if (type == ix86_opt_enum)
> @@ -1225,18 +1243,8 @@ ix86_valid_target_attribute_tree (tree f
>        /* If we are using the default tune= or arch=, undo the string assigned,
>          and use the default.  */
>        if (option_strings[IX86_FUNCTION_SPECIFIC_ARCH])
> -       {
> -         opts->x_ix86_arch_string
> -           = ggc_strdup (option_strings[IX86_FUNCTION_SPECIFIC_ARCH]);
> -
> -         /* If arch= is set,  clear all bits in x_ix86_isa_flags,
> -            except for ISA_64BIT, ABI_64, ABI_X32, and CODE16.  */
> -         opts->x_ix86_isa_flags &= (OPTION_MASK_ISA_64BIT
> -                                    | OPTION_MASK_ABI_64
> -                                    | OPTION_MASK_ABI_X32
> -                                    | OPTION_MASK_CODE16);
> -         opts->x_ix86_isa_flags2 = 0;
> -       }
> +       opts->x_ix86_arch_string
> +         = ggc_strdup (option_strings[IX86_FUNCTION_SPECIFIC_ARCH]);
>        else if (!orig_arch_specified)
>         opts->x_ix86_arch_string = NULL;
>
> --- gcc/testsuite/gcc.target/i386/pr90867.c.jj  2019-11-19 18:49:11.746592189 +0100
> +++ gcc/testsuite/gcc.target/i386/pr90867.c     2019-11-19 18:48:17.271406789 +0100
> @@ -0,0 +1,30 @@
> +/* PR target/90867 */
> +/* { dg-do run { target lp64 } } */
> +/* { dg-options "-O2 -msse2" } */
> +
> +unsigned long long freq = 3600000000UL;   /* 3.6 GHz = 3600.0 MHz */
> +
> +__attribute__((noipa)) void
> +bar (double x)
> +{
> +  static double d = 3600000000.0;
> +  if (x != d)
> +    __builtin_abort ();
> +  d /= 1000.0;
> +}
> +
> +__attribute__ ((target ("arch=x86-64"))) int
> +foo ()
> +{
> +  bar ((double) freq);
> +  bar (1e-3 * freq);
> +  bar (1e-6 * freq);
> +  bar (1e-9 * freq);
> +  return 0;
> +}
> +
> +int
> +main ()
> +{
> +  return foo ();
> +}
>
>         Jakub
>

Patch
diff mbox series

--- gcc/config/i386/i386-options.c.jj	2019-11-18 12:07:54.672405129 +0100
+++ gcc/config/i386/i386-options.c	2019-11-19 18:43:36.426606458 +0100
@@ -1147,7 +1147,25 @@  ix86_valid_target_attribute_inner_p (tre
 	      ret = false;
 	    }
 	  else
-	    p_strings[opt] = xstrdup (p + opt_len);
+	    {
+	      p_strings[opt] = xstrdup (p + opt_len);
+	      if (opt == IX86_FUNCTION_SPECIFIC_ARCH)
+		{
+		  /* If arch= is set,  clear all bits in x_ix86_isa_flags,
+		     except for ISA_64BIT, ABI_64, ABI_X32, and CODE16
+		     and all bits in x_ix86_isa_flags2.  */
+		  opts->x_ix86_isa_flags &= (OPTION_MASK_ISA_64BIT
+					     | OPTION_MASK_ABI_64
+					     | OPTION_MASK_ABI_X32
+					     | OPTION_MASK_CODE16);
+		  opts->x_ix86_isa_flags_explicit &= (OPTION_MASK_ISA_64BIT
+						      | OPTION_MASK_ABI_64
+						      | OPTION_MASK_ABI_X32
+						      | OPTION_MASK_CODE16);
+		  opts->x_ix86_isa_flags2 = 0;
+		  opts->x_ix86_isa_flags2_explicit = 0;
+		}
+	    }
 	}
 
       else if (type == ix86_opt_enum)
@@ -1225,18 +1243,8 @@  ix86_valid_target_attribute_tree (tree f
       /* If we are using the default tune= or arch=, undo the string assigned,
 	 and use the default.  */
       if (option_strings[IX86_FUNCTION_SPECIFIC_ARCH])
-	{
-	  opts->x_ix86_arch_string
-	    = ggc_strdup (option_strings[IX86_FUNCTION_SPECIFIC_ARCH]);
-
-	  /* If arch= is set,  clear all bits in x_ix86_isa_flags,
-	     except for ISA_64BIT, ABI_64, ABI_X32, and CODE16.  */
-	  opts->x_ix86_isa_flags &= (OPTION_MASK_ISA_64BIT
-				     | OPTION_MASK_ABI_64
-				     | OPTION_MASK_ABI_X32
-				     | OPTION_MASK_CODE16);
-	  opts->x_ix86_isa_flags2 = 0;
-	}
+	opts->x_ix86_arch_string
+	  = ggc_strdup (option_strings[IX86_FUNCTION_SPECIFIC_ARCH]);
       else if (!orig_arch_specified)
 	opts->x_ix86_arch_string = NULL;
 
--- gcc/testsuite/gcc.target/i386/pr90867.c.jj	2019-11-19 18:49:11.746592189 +0100
+++ gcc/testsuite/gcc.target/i386/pr90867.c	2019-11-19 18:48:17.271406789 +0100
@@ -0,0 +1,30 @@ 
+/* PR target/90867 */
+/* { dg-do run { target lp64 } } */
+/* { dg-options "-O2 -msse2" } */
+
+unsigned long long freq = 3600000000UL;   /* 3.6 GHz = 3600.0 MHz */
+
+__attribute__((noipa)) void
+bar (double x)
+{
+  static double d = 3600000000.0;
+  if (x != d)
+    __builtin_abort ();
+  d /= 1000.0;
+}
+
+__attribute__ ((target ("arch=x86-64"))) int
+foo ()
+{
+  bar ((double) freq);
+  bar (1e-3 * freq);
+  bar (1e-6 * freq);
+  bar (1e-9 * freq);
+  return 0;
+}
+
+int
+main ()
+{
+  return foo ();
+}