Re: [PATCH v4 2/2] x86: Add general_regs_only function attribute

[Richard Biener <richard.guenther@gmail.com>]

On Thu, Apr 22, 2021 at 12:30 PM Jakub Jelinek via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Wed, Apr 21, 2021 at 06:01:07PM -0700, H.J. Lu via Gcc-patches wrote:
> > How about this?
> >
> > @item general_regs_only
> > @cindex @code{general_regs_only} function attribute, x86
> > The @code{general_regs_only} function attribute informs the compiler
> > that the function uses only general purpose registers.  When the
> > compiler inlines a function with the @code{always_inline} attribute,
> > target-specific compilation options may lead to inline failures.
> > The @code{general_regs_only} attribute, if applicable, can be used
> > together with the @code{always_inline} attribute to reduce inlining
> > failure.
>
> I don't really like this attribute.
> It is very specific to what you want to solve and doesn't address the
> general problem, that always_inline means different things in different
> code, and that is a problem for many targets, not just one.
>
> As has been written, in some cases it means inline always, error
> whenever it is called indirectly which can't be optimized into a direct
> call that can be inlined and error whenever the inlining fails for other
> reasons.
>
> Another case, e.g. in the glibc fortify wrappers, is inline always
> when the call is direct or an indirect call can be optimized into a direct
> call and error when the inlining fails, but support indirect calls without
> errors.
>
> Another case, which is most of the x86/aarch64/arm etc. intrinsics, is
> inline always unless there is a target mismatch (roughly what is
> actually implemented).
>
> Because from the always_inline attribute it is impossible to determine which
> one of those it is (for the indirect calls the rule could be
> gnu_inline extern inline means indirect calls are ok, anything else means
> indirect calls are bad), we need new syntax to distinguish those cases.
>
> general_regs_only attribute doesn't seem to be it, e.g. for the glibc
> fortify wrappers cases I don't see why we should forbid using floating point
> in such inlines.
>
> So IMHO we need some new attribute for one of those, or optional parameter
> to always_inline.
>
> For the intrinsic case, ideal would be if we could record which ISA flags
> (or more generally which options) are required and which are not.  Either
> have some syntax where those would be explicitly specified in attribute (but
> frankly that would be a maintainance nightmare), or derive those from
> surrounding pragmas.  Right now we have those wrapped in
> #ifndef __AVX2__
> #pragma GCC push_options
> #pragma GCC target("avx2")
> #define __DISABLE_AVX2__
> #endif /* __AVX2__ */
>
> ...
>
> #ifdef __DISABLE_AVX2__
> #undef __DISABLE_AVX2__
> #pragma GCC pop_options
> #endif /* __DISABLE_AVX2__ */
>
> The question is if the pragma GCC target right now behaves incrementally
> or not, whether
> #pragma GCC target("avx2")
> adds -mavx2 to options if it was missing before and nothing otherwise, or if
> it switches other options off.  If it is incremental, we could e.g. try to
> use the second least significant bit of global_options_set.x_* to mean
> this option has been set explicitly by some surrounding #pragma GCC target.
> The normal tests - global_options_set.x_flag_whatever could still work
> fine because they wouldn't care if the option was explicit from anywhere
> (command line or GCC target or target attribute) and just & 2 would mean
> it was explicit from pragma GCC target; though there is the case of
> bitfields... And then the inlining decision could check the & 2 flags to
> see what is required and what is just from command line.
> Or we can have some other pragma GCC that would be like target but would
> have flags that are explicit (and could e.g. be more restricted, to ISA
> options only, and let those use in addition to #pragma GCC target.

I'm still curious as to what you think will break if always-inline does what
it is documented to do.

Richard.

>         Jakub
>