Re: [PATCH] x86: Use target("general-regs-only,baseline-isas-only") in

2020-08-25 Thread Uros Bizjak via Gcc-patches
On Tue, Aug 25, 2020 at 2:13 PM H.J. Lu  wrote:
>
> On Mon, Aug 24, 2020 at 12:40 PM H.J. Lu  wrote:
> >
> > On Mon, Aug 24, 2020 at 12:25 PM Uros Bizjak  wrote:
> > >
> > > On Mon, Aug 24, 2020 at 6:17 PM H.J. Lu  wrote:
> > > >
> > > > On Mon, Aug 24, 2020 at 7:55 AM Uros Bizjak  wrote:
> > > > >
> > > > > On Mon, Aug 24, 2020 at 3:23 PM H.J. Lu  wrote:
> > > > >
> > > > > > > Speaking of pragmas, these should be added outside cpuid.h, like:
> > > > > > >
> > > > > > > #pragma GCC push_options
> > > > > > > #pragma GCC target("general-regs-only")
> > > > > > >
> > > > > > > #include 
> > > > > > >
> > > > > > > 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  to the 
> > > > > > basic ISAs.
> > > > > >  should be handled similarly to other intrinsic header 
> > > > > > files.
> > > > > > That is  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  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 .
> >
>
> We just need to update ix86_can_inline_p to allow inline functions
> with baseline-isas-only and general-regs-only attributes if caller
> supports the same set of ISAs.
>
> Here is the updated patch.

I'm not against it, but I don't plan to approve the attached patch.

Uros.


[PATCH] x86: Use target("general-regs-only, baseline-isas-only") in

2020-08-25 Thread H.J. Lu via Gcc-patches
On Mon, Aug 24, 2020 at 12:40 PM H.J. Lu  wrote:
>
> On Mon, Aug 24, 2020 at 12:25 PM Uros Bizjak  wrote:
> >
> > On Mon, Aug 24, 2020 at 6:17 PM H.J. Lu  wrote:
> > >
> > > On Mon, Aug 24, 2020 at 7:55 AM Uros Bizjak  wrote:
> > > >
> > > > On Mon, Aug 24, 2020 at 3:23 PM H.J. Lu  wrote:
> > > >
> > > > > > Speaking of pragmas, these should be added outside cpuid.h, like:
> > > > > >
> > > > > > #pragma GCC push_options
> > > > > > #pragma GCC target("general-regs-only")
> > > > > >
> > > > > > #include 
> > > > > >
> > > > > > 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  to the basic 
> > > > > ISAs.
> > > > >  should be handled similarly to other intrinsic header files.
> > > > > That is  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  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 .
>

We just need to update ix86_can_inline_p to allow inline functions
with baseline-isas-only and general-regs-only attributes if caller
supports the same set of ISAs.

Here is the updated patch.

-- 
H.J.
From 78eb1a4c4938494349032f0e10017ce553fb8fdd Mon Sep 17 00:00:00 2001
From: "H.J. Lu" 
Date: Fri, 21 Aug 2020 09:42:49 -0700
Subject: [PATCH] x86: Use target("general-regs-only,baseline-isas-only") in
 

Add -mbaseline-isas-only and target("baseline-isas-only") attribute to
support baseline ISAs, which include FXSR, MMX, SSE and SSE2 in 64-bit
mode.  Use only general registers and baseline ISAs to perform CPUID
check.  We can inline functions with general registers and baseline
ISAs attributes if caller supports the same set of ISAs.

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("general-regs-only,baseline-isas-only").
	* config/i386/i386-options.c (ix86_valid_target_attribute_inner_p):
	Handle baseline-isas-only.
	* config/i386/i386.c (ix86_can_inline_p): Allow inline functions
	with baseline-isas-only and general-regs-only attributes if caller
	supports the same set of ISAs.
	* config/i386/i386.h (TARGET_64BIT_BASELINE_ISAS): New.