x86: Use target("baseline-isas-only") in <cpuid.h>

Message ID CAMe9rOo62gOw+_QP1xiK+stubF8qrkD9VOxy1Bms9nGr30wK5w@mail.gmail.com
State New
Headers show
Series
  • x86: Use target("baseline-isas-only") in <cpuid.h>
Related show

Commit Message

H.J. Lu via Gcc-patches Aug. 24, 2020, 4:16 p.m.
On Mon, Aug 24, 2020 at 7:55 AM Uros Bizjak <ubizjak@gmail.com> wrote:
>

> On Mon, Aug 24, 2020 at 3:23 PM H.J. Lu <hjl.tools@gmail.com> wrote:

>

> > > Speaking of pragmas, these should be added outside cpuid.h, like:

> > >

> > > #pragma GCC push_options

> > > #pragma GCC target("general-regs-only")

> > >

> > > #include <cpuid.h>

> > >

> > > void cpuid_check ()

> > > ...

> > >

> > > #pragma GCC pop_options

> > >

> > > >footnote

> > >

> > > Nowadays, -march=native is mostly used outside generic target

> > > compilations, so for relevant avx512 targets, we still generate spills

> > > to mask regs. In future, we can review the setting of the tuning flag

> > > for a generic target in the same way as with SSE2 inter-reg moves.

> > >

> >

> > Florian raised an issue that we need to limit <cpuid.h> to the basic ISAs.

> > <cpuid.h> should be handled similarly to other intrinsic header files.

> > That is <cpuid.h> should use

> >

> > #pragma GCC push_options

> > #ifdef __x86_64__

> > #pragma GCC target("arch=x86-64")

> > #else

> > #pragma GCC target("arch=i386")

> > ...

> > #pragma GCC pop_options

> >

> > Here is a patch.  OK for master?

>

> -ENOPATCH

>

> However, how will this affect inlining? Every single function in

> cpuid.h is defined as static __inline, and due to target flags

> mismatch, it won't be inlined anymore. These inline functions are used

> in some bit testing functions, and to keep them inlined, these should

> also use the same options to avoid non-basic ISAs. This is the reason

> cpuid.h should be #included after pragma, together with bit testing

> functions, as shown above.

>


How about target("baseline-isas-only")? All CPUID functions are
inlined.


-- 
H.J.

Comments

H.J. Lu via Gcc-patches Aug. 24, 2020, 7:25 p.m. | #1
On Mon, Aug 24, 2020 at 6:17 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>

> On Mon, Aug 24, 2020 at 7:55 AM Uros Bizjak <ubizjak@gmail.com> wrote:

> >

> > On Mon, Aug 24, 2020 at 3:23 PM H.J. Lu <hjl.tools@gmail.com> wrote:

> >

> > > > Speaking of pragmas, these should be added outside cpuid.h, like:

> > > >

> > > > #pragma GCC push_options

> > > > #pragma GCC target("general-regs-only")

> > > >

> > > > #include <cpuid.h>

> > > >

> > > > void cpuid_check ()

> > > > ...

> > > >

> > > > #pragma GCC pop_options

> > > >

> > > > >footnote

> > > >

> > > > Nowadays, -march=native is mostly used outside generic target

> > > > compilations, so for relevant avx512 targets, we still generate spills

> > > > to mask regs. In future, we can review the setting of the tuning flag

> > > > for a generic target in the same way as with SSE2 inter-reg moves.

> > > >

> > >

> > > Florian raised an issue that we need to limit <cpuid.h> to the basic ISAs.

> > > <cpuid.h> should be handled similarly to other intrinsic header files.

> > > That is <cpuid.h> should use

> > >

> > > #pragma GCC push_options

> > > #ifdef __x86_64__

> > > #pragma GCC target("arch=x86-64")

> > > #else

> > > #pragma GCC target("arch=i386")

> > > ...

> > > #pragma GCC pop_options

> > >

> > > Here is a patch.  OK for master?

> >

> > -ENOPATCH

> >

> > However, how will this affect inlining? Every single function in

> > cpuid.h is defined as static __inline, and due to target flags

> > mismatch, it won't be inlined anymore. These inline functions are used

> > in some bit testing functions, and to keep them inlined, these should

> > also use the same options to avoid non-basic ISAs. This is the reason

> > cpuid.h should be #included after pragma, together with bit testing

> > functions, as shown above.

> >

>

> How about target("baseline-isas-only")? All CPUID functions are

> inlined.


No, I don't think this is a good idea. Now consider the situation that
caller functions are compiled with e.g. -mgeneral-regs-only. Due to
#pragmas, CPUID functions are compiled with a superset ISAs, so they
again won't be inlined. ISAs of caller functions and CPUID should
match, the best way is to include <cpuid.h> after the #pragma. And
IMO, general-regs-only target #pragma is an excellent setting for
both: cpuid.h and caller bit testing functions.

So, if we care about inlining, decorating cpuid.h with target pragmas
is a bad idea.

Uros.
H.J. Lu via Gcc-patches Aug. 24, 2020, 7:40 p.m. | #2
On Mon, Aug 24, 2020 at 12:25 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>

> On Mon, Aug 24, 2020 at 6:17 PM H.J. Lu <hjl.tools@gmail.com> wrote:

> >

> > On Mon, Aug 24, 2020 at 7:55 AM Uros Bizjak <ubizjak@gmail.com> wrote:

> > >

> > > On Mon, Aug 24, 2020 at 3:23 PM H.J. Lu <hjl.tools@gmail.com> wrote:

> > >

> > > > > Speaking of pragmas, these should be added outside cpuid.h, like:

> > > > >

> > > > > #pragma GCC push_options

> > > > > #pragma GCC target("general-regs-only")

> > > > >

> > > > > #include <cpuid.h>

> > > > >

> > > > > void cpuid_check ()

> > > > > ...

> > > > >

> > > > > #pragma GCC pop_options

> > > > >

> > > > > >footnote

> > > > >

> > > > > Nowadays, -march=native is mostly used outside generic target

> > > > > compilations, so for relevant avx512 targets, we still generate spills

> > > > > to mask regs. In future, we can review the setting of the tuning flag

> > > > > for a generic target in the same way as with SSE2 inter-reg moves.

> > > > >

> > > >

> > > > Florian raised an issue that we need to limit <cpuid.h> to the basic ISAs.

> > > > <cpuid.h> should be handled similarly to other intrinsic header files.

> > > > That is <cpuid.h> should use

> > > >

> > > > #pragma GCC push_options

> > > > #ifdef __x86_64__

> > > > #pragma GCC target("arch=x86-64")

> > > > #else

> > > > #pragma GCC target("arch=i386")

> > > > ...

> > > > #pragma GCC pop_options

> > > >

> > > > Here is a patch.  OK for master?

> > >

> > > -ENOPATCH

> > >

> > > However, how will this affect inlining? Every single function in

> > > cpuid.h is defined as static __inline, and due to target flags

> > > mismatch, it won't be inlined anymore. These inline functions are used

> > > in some bit testing functions, and to keep them inlined, these should

> > > also use the same options to avoid non-basic ISAs. This is the reason

> > > cpuid.h should be #included after pragma, together with bit testing

> > > functions, as shown above.

> > >

> >

> > How about target("baseline-isas-only")? All CPUID functions are

> > inlined.

>

> No, I don't think this is a good idea. Now consider the situation that

> caller functions are compiled with e.g. -mgeneral-regs-only. Due to

> #pragmas, CPUID functions are compiled with a superset ISAs, so they

> again won't be inlined. ISAs of caller functions and CPUID should

> match, the best way is to include <cpuid.h> after the #pragma. And

> IMO, general-regs-only target #pragma is an excellent setting for

> both: cpuid.h and caller bit testing functions.

>

> So, if we care about inlining, decorating cpuid.h with target pragmas

> is a bad idea.


This can be done with #pragma in <cpuid.h>.

-- 
H.J.

Patch

From 82efbfdc58e6bdcf11a9e09018db6bbc690f77b1 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Fri, 21 Aug 2020 09:42:49 -0700
Subject: [PATCH] x86: Use target("baseline-isas-only") in <cpuid.h>

CPUID check should be done only with baseline ISAs, which include FXSR,
MMX, SSE and SSE2 in 64-bit mode.

gcc/

	PR target/96744
	* common/config/i386/i386-common.c (ix86_handle_option): Support
	-mbaseline-isas-only.
	* config/i386/cpuid.h: Add #pragma GCC target("baseline-isas-only").
	* config/i386/i386-options.c (ix86_valid_target_attribute_inner_p):
	Handle baseline-isas-only.
	* config/i386/i386.opt: Add -mbaseline-isas-only.
	* doc/extend.texi: Document target("baseline-isas-only") function
	attribute.
	* doc/invoke.texi: Document -mbaseline-isas-only.

gcc/testsuite/

	PR target/96744
	* gcc.target/i386/avx512-check.h: Add #pragma GCC
	target("baseline-isas-only") for CPUID check.
	* gcc.target/i386/pr96744-10.c: New test.
---
 gcc/common/config/i386/i386-common.c         | 20 +++++++++++++++
 gcc/config/i386/cpuid.h                      | 13 ++++++++++
 gcc/config/i386/i386-options.c               |  7 ++++-
 gcc/config/i386/i386.opt                     |  6 ++++-
 gcc/doc/extend.texi                          |  4 +++
 gcc/doc/invoke.texi                          |  5 ++++
 gcc/testsuite/gcc.target/i386/avx512-check.h |  5 ++++
 gcc/testsuite/gcc.target/i386/pr96744-10.c   | 27 ++++++++++++++++++++
 8 files changed, 85 insertions(+), 2 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/pr96744-10.c

diff --git a/gcc/common/config/i386/i386-common.c b/gcc/common/config/i386/i386-common.c
index bb14305ad7b..2c0b7f3fe0f 100644
--- a/gcc/common/config/i386/i386-common.c
+++ b/gcc/common/config/i386/i386-common.c
@@ -338,6 +338,26 @@  ix86_handle_option (struct gcc_options *opts,
 	gcc_unreachable ();
       return true;
 
+    case OPT_mbaseline_isas_only:
+      if (value)
+	{
+	  /* Only enable baseline ISAs.  */
+	  if ((opts->x_ix86_isa_flags & OPTION_MASK_ISA_64BIT))
+	    opts->x_ix86_isa_flags = (OPTION_MASK_ISA_64BIT
+				      | OPTION_MASK_ISA_FXSR
+				      | OPTION_MASK_ISA_MMX
+				      | OPTION_MASK_ISA_SSE
+				      | OPTION_MASK_ISA_SSE2);
+	  else
+	    opts->x_ix86_isa_flags = 0;
+	  opts->x_ix86_isa_flags2 = 0;
+	  opts->x_ix86_isa_flags_explicit = -1;
+	  opts->x_ix86_isa_flags2_explicit = -1;
+	}
+      else
+	gcc_unreachable ();
+      return true;
+
     case OPT_mmmx:
       if (value)
 	{
diff --git a/gcc/config/i386/cpuid.h b/gcc/config/i386/cpuid.h
index bca61d620db..dd2ef8f9b30 100644
--- a/gcc/config/i386/cpuid.h
+++ b/gcc/config/i386/cpuid.h
@@ -24,6 +24,17 @@ 
 #ifndef _CPUID_H_INCLUDED
 #define _CPUID_H_INCLUDED
 
+#pragma GCC push_options
+#if __GNUC__ >= 11
+#pragma GCC target("baseline-isas-only")
+#else
+#ifdef __x86_64__
+#pragma GCC target("arch=x86-64")
+#else
+#pragma GCC target("arch=i386")
+#endif
+#endif
+
 /* %eax */
 #define bit_AVX512BF16	(1 << 5)
 
@@ -324,4 +335,6 @@  __cpuidex (int __cpuid_info[4], int __leaf, int __subleaf)
 		 __cpuid_info[2], __cpuid_info[3]);
 }
 
+#pragma GCC pop_options
+
 #endif /* _CPUID_H_INCLUDED */
diff --git a/gcc/config/i386/i386-options.c b/gcc/config/i386/i386-options.c
index e0fc68c27bf..4a09c1c93ee 100644
--- a/gcc/config/i386/i386-options.c
+++ b/gcc/config/i386/i386-options.c
@@ -1072,6 +1072,10 @@  ix86_valid_target_attribute_inner_p (tree fndecl, tree args, char *p_strings[],
     IX86_ATTR_IX86_YES ("general-regs-only",
 			OPT_mgeneral_regs_only,
 			OPTION_MASK_GENERAL_REGS_ONLY),
+
+    IX86_ATTR_IX86_YES ("baseline-isas-only",
+			OPT_mbaseline_isas_only,
+			OPTION_MASK_BASELINE_ISAS_ONLY),
   };
 
   location_t loc
@@ -1187,7 +1191,8 @@  ix86_valid_target_attribute_inner_p (tree fndecl, tree args, char *p_strings[],
 
       else if (type == ix86_opt_ix86_yes || type == ix86_opt_ix86_no)
 	{
-	  if (mask == OPTION_MASK_GENERAL_REGS_ONLY)
+	  if (mask == OPTION_MASK_GENERAL_REGS_ONLY
+	      || mask == OPTION_MASK_BASELINE_ISAS_ONLY)
 	    {
 	      if (type != ix86_opt_ix86_yes)
 		gcc_unreachable ();
diff --git a/gcc/config/i386/i386.opt b/gcc/config/i386/i386.opt
index c9f7195d423..f3a088aaa28 100644
--- a/gcc/config/i386/i386.opt
+++ b/gcc/config/i386/i386.opt
@@ -1015,6 +1015,10 @@  mgeneral-regs-only
 Target Report RejectNegative Mask(GENERAL_REGS_ONLY) Var(ix86_target_flags) Save
 Generate code which uses only the general registers.
 
+mbaseline-isas-only
+Target Report RejectNegative Mask(BASELINE_ISAS_ONLY) Var(ix86_target_flags) Save
+Generate code which uses only the baseline ISAs.
+
 mshstk
 Target Report Mask(ISA_SHSTK) Var(ix86_isa_flags) Save
 Enable shadow stack built-in functions from Control-flow Enforcement
@@ -1114,4 +1118,4 @@  Support SERIALIZE built-in functions and code generation.
 
 mtsxldtrk
 Target Report Mask(ISA2_TSXLDTRK) Var(ix86_isa_flags2) Save
-Support TSXLDTRK built-in functions and code generation.
\ No newline at end of file
+Support TSXLDTRK built-in functions and code generation.
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 2bb9b2f72f5..eadb8dd71a4 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -6660,6 +6660,10 @@  doing a floating-point division.
 @cindex @code{target("general-regs-only")} function attribute, x86
 Generate code which uses only the general registers.
 
+@item baseline-isas-only
+@cindex @code{target("baseline-isas-only")} function attribute, x86
+Generate code which uses only the baseline ISAs.
+
 @item arch=@var{ARCH}
 @cindex @code{target("arch=@var{ARCH}")} function attribute, x86
 Specify the architecture to generate code for in compiling the function.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 4cf6b204b56..5499bbe809e 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -30579,6 +30579,11 @@  Generate code that uses only the general-purpose registers.  This
 prevents the compiler from using floating-point, vector, mask and bound
 registers.
 
+@item -mbaseline-isas-only
+@opindex mbaseline-isas-only
+Generate code that uses only the baseline ISAs which include FXSR, MMX,
+SSE and SSE2 in 64-bit mode.
+
 @item -mindirect-branch=@var{choice}
 @opindex mindirect-branch
 Convert indirect call and jump with @var{choice}.  The default is
diff --git a/gcc/testsuite/gcc.target/i386/avx512-check.h b/gcc/testsuite/gcc.target/i386/avx512-check.h
index 0a377dba1d5..396a18d377b 100644
--- a/gcc/testsuite/gcc.target/i386/avx512-check.h
+++ b/gcc/testsuite/gcc.target/i386/avx512-check.h
@@ -25,6 +25,9 @@  do_test (void)
 }
 #endif
 
+#pragma GCC push_options
+#pragma GCC target("baseline-isas-only")
+
 static int
 check_osxsave (void)
 {
@@ -110,3 +113,5 @@  main ()
 #endif
   return 0;
 }
+
+#pragma GCC pop_options
diff --git a/gcc/testsuite/gcc.target/i386/pr96744-10.c b/gcc/testsuite/gcc.target/i386/pr96744-10.c
new file mode 100644
index 00000000000..f6f9badde1b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/pr96744-10.c
@@ -0,0 +1,27 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -march=skylake" } */
+
+#include <cpuid.h>
+
+int
+main ()
+{
+  unsigned int eax, ebx, ecx, edx;
+  int cpuid_info[4];
+
+  if (!__get_cpuid_count (7, 0, &eax, &ebx, &ecx, &edx))
+    return 0;
+
+  __cpuidex (cpuid_info, 7, 0);
+
+  if (cpuid_info[0] != eax
+      || cpuid_info[1] != ebx
+      || cpuid_info[2] != ecx
+      || cpuid_info[3] != edx)
+    __builtin_abort ();
+
+  return 0;
+}
+
+/* { dg-final { scan-assembler-not {call[ \t]+_?__get_cpuid_count} } } */
+/* { dg-final { scan-assembler-not {call[ \t]+_?__cpuidex} } } */
-- 
2.26.2