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

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

On Thu, Apr 22, 2021 at 3:01 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Wed, Apr 21, 2021 at 4:24 PM Martin Sebor <msebor@gmail.com> wrote:
> >
> > On 4/21/21 2:58 PM, H.J. Lu wrote:
> > > On Wed, Apr 21, 2021 at 10:09 AM Martin Sebor <msebor@gmail.com> wrote:
> > >>
> > >> On 4/14/21 4:39 PM, H.J. Lu wrote:
> > >>> commit 87c753ac241f25d222d46ba1ac66ceba89d6a200
> > >>> Author: H.J. Lu <hjl.tools@gmail.com>
> > >>> Date:   Fri Aug 21 09:42:49 2020 -0700
> > >>>
> > >>>       x86: Add target("general-regs-only") function attribute
> > >>>
> > >>> is incomplete since it is impossible to call integer intrinsics from
> > >>> a function with general-regs-only target attribute.
> > >>>
> > >>> 1. Add general_regs_only function attribute to inform the compiler that
> > >>> functions use only general purpose registers.  When making inlining
> > >>> decisions on such functions, non-GPR compiler options are excluded.
> > >>> 2. Add general_regs_only attribute to x86 intrinsics which use only
> > >>> general purpose registers.
> > >>>
> > >> ...
> > >>> --- a/gcc/doc/extend.texi
> > >>> +++ b/gcc/doc/extend.texi
> > >>> @@ -7066,6 +7066,11 @@ On x86 targets, the @code{fentry_section} attribute sets the name
> > >>>    of the section to record function entry instrumentation calls in when
> > >>>    enabled with @option{-pg -mrecord-mcount}
> > >>>
> > >>> +@item general_regs_only
> > >>> +@cindex @code{general_regs_only} function attribute, x86
> > >>> +The @code{general_regs_only} attribute on functions is used to
> > >>> +inform the compiler that functions use only general purpose registers.
> > >>
> > >> I'll just reiterate basically the same comment as before: it's not
> > >> clear from the very brief description above what the requirements
> > >> are for using the attribute.  I'm guessing it can be applied to
> > >> any function (inline or otherwise) but only has any effect when
> > >> the function is actually inlined and otherwise doesn't constrain
> > >> what the function can do.  (Whatever the constraints are, I think
> > >> the manual should spell them out, and likewise for its effects.)
> > >
> > > That is correct.
> > >
> > >> Similarly it's not clear what should be expected when the function
> > >> does use some other register.  Ideally, I think GCC would check and
> > >> issue a nice error message whether or not the function is inlined
> > >> or called.  I suspect that might only be possible for inline
> > >> functions that are actually called and for which the back end must
> > >> emit code.
> > >
> > > This is what GCC does today:
> > >
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=99744
> >
> > Yes, that's the rather obscure error I think I commented on before
> > and suggested should be improved.  Based on r99744-3.c I don't think
> > this has changed in the improved patch.
>
> My goal is to fix the inline failures, not to improve the compiler error
> message.
>
> > >
> > >> Other than that, I'd suggest to improve the phrasing a bit:
> > >>
> > >>     The @code{general_regs_only} function attribute indicates that
> > >>     the function uses only general purpose registers... [text
> > >>     explaining constraints and errors follows].
> > >>
> > >> Martin
> > >
> > > How about this
> > >
> > > @item general_regs_only
> > > @cindex @code{general_regs_only} function attribute, x86
> > > The @code{general_regs_only} attribute on functions is used to inform
> > > the compiler that functions use only general purpose registers.  It
> > > can be used together with the @code{always_inline} attribute to avoid
> > > inlining failure when there is a mismatch in compiler vector options.
> >
> > Without an article the part "that functions use only general purpose
> > registers" is unclear and/or grammatically incorrect.  What functions?
> > If the function the attribute is applied to, it needs an article, e.g.,
> > "the function" or "a function", and singular.  (Otherwise it could be
> > read as talking about the functions called from the one with
> > the attribute, or some other functions altogether).
> >
> > I tried to correct that above but, if you prefer, the following would
> > be closer to your phrasing but more correct/accurate:
> >
> >    The @code{general_regs_only} function attribute informs
> >    the compiler that the function uses only general purpose
> >    registers.
> >
> > I don't understand what the second sentence is trying to say, and
> > without a better error message for the problem in r99744, I suspect
> > few users will either.  I am suggesting to explain in the text you
> > are adding, under what conditions inlining might fail without
> > the attribute, and what effect the attribute has on the function
> > that prevents the inlining failure.
>
> 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.
>
> > (If we can't explain what the effect is then I wonder why
> > the attribute is being added at all instead of teaching GCC to
> > always behave as if the attribute were there when its absence
> > would otherwise lead to an error.)
> >
>
> Inlining an always_inline function into a function, which doesn't
> support the ISA needed by the always_inline function,  should fail.
> But when inlining the always_inline function, the compiler doesn't
> know if GPR instructions are sufficient for the always_inline function.
> The general_regs_only informs the compiler that the function uses
> only general purpose registers.   There are no other ways for the
> compiler to deduce such info at this stage.

Is placing this attribute on a function definition which ends up
using non-GPR regs invoking undefined behavior?  That is,
can IRA/LRA assume xmm regs need no saving around calls
to such functions?  Do we need to diagnose non-GPR uses
(late, of course)?

I'm not sure we're going to solve the always-inline issues with
this new attribute - for example BMI intrinsics won't use
non-GPR regs but then a function using those and marked
as general-regs-only should still not be inlined into a
function doing the BMI cpuid check.

You document the attribute only affects always_inline
decisions, this makes it even more special.

I still think we should do the same for target attributes
as we do for optimize attributes - always_inline should
ignore any option differences and do what it is documented
(inline):

"@item always_inline
@cindex @code{always_inline} function attribute
Generally, functions are not inlined unless optimization is specified.
For functions declared inline, this attribute inlines the function
independent of any restrictions that otherwise apply to inlining.
Failure to inline such a function is diagnosed as an error.
Note that if such a function is called indirectly the compiler may
or may not inline it depending on optimization level and a failure
to inline an indirect call may or may not be diagnosed."

nowhere does this suggest that inlining is not done if
the caller is AVX2 but the callee is not.

Richard.

> --
> H.J.