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

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

On Thu, Apr 22, 2021 at 2:52 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Thu, Apr 22, 2021 at 2:22 PM Jakub Jelinek <jakub@redhat.com> wrote:
> >
> > On Thu, Apr 22, 2021 at 01:23:20PM +0200, Richard Biener via Gcc-patches wrote:
> > > > 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.
> >
> > We will silently accept calling intrinsics that must be used only in certain
> > ISA contexts, which will lead to people writing non-portable code.
> >
> > So -O2 -mno-avx
> > #include <x86intrin.h>
> >
> > void
> > foo (__m256 *x)
> > {
> >   x[0] = _mm256_sub_ps (x[1], x[2]);
> > }
> > etc. will now be accepted when it shouldn't be.
> > clang rejects it like gcc with:
> > 1.c:6:10: error: always_inline function '_mm256_sub_ps' requires target feature 'avx', but would be inlined into function 'foo' that is compiled without support for 'avx'
> >   x[0] = _mm256_sub_ps (x[1], x[2]);
> >          ^
> >
> > Note, if I do:
> > #include <x86intrin.h>
> >
> > __attribute__((target ("no-sse3"))) void
> > foo (__m256 *x)
> > {
> >   x[0] = _mm256_sub_ps (x[1], x[2]);
> > }
> > and compile
> > clang -S -O2 -mavx2 1.c
> > 1.c:6:10: error: always_inline function '_mm256_sub_ps' requires target feature 'avx', but would be inlined into function 'foo' that is compiled without support for 'avx'
> >   x[0] = _mm256_sub_ps (x[1], x[2]);
> >          ^
> > then from the error message it seems that unlike GCC, clang remembers
> > the exact target features that are needed for the intrinsics and checks just
> > those.
> > Though, looking at the preprocessed source, seems it uses
> > static __inline __m256 __attribute__((__always_inline__, __nodebug__, __target__("avx"), __min_vector_width__(256)))
> > _mm256_sub_ps(__m256 __a, __m256 __b)
> > {
> >   return (__m256)((__v8sf)__a-(__v8sf)__b);
> > }
> > and not target pragmas.
> >
> > Anyway, if we tweak our intrinsic headers so that
> > -#ifndef __AVX__
> >  #pragma GCC push_options
> >  #pragma GCC target("avx")
> > -#define __DISABLE_AVX__
> > -#endif /* __AVX__ */
> >
> > ...
> > -#ifdef __DISABLE_AVX__
> > -#undef __DISABLE_AVX__
> >  #pragma GCC pop_options
> > -#endif /* __DISABLE_AVX__ */
> > and do the opts_set->x_* & 2 stuff on explicit options coming out of
> > target/optimize pragmas and attributes, perhaps we don't even need
> > to introduce a new attribute and can handle everything magically:

Oh, and any such changes will likely interact with Martins ideas to rework
how optimize and target attributes work (aka adding ontop of the
commandline options).  That is, attribute target will then not be enough
to remember the exact set of needed ISA features (as opposed to what
likely clang implements?)

> > 1) if it is gnu_inline extern inline, allow indirect calls, otherwise
> > disallow them for always_inline functions
>
> There are a lot of intrinsics using extern inline __gnu_inline though...
>
> > 2) for the isa flags and option mismatches, only disallow opts_set->x_* & 2
> > stuff
> > This will keep both intrinsics and glibc fortify macros working fine
> > in all the needed use cases.
>
> Yes, see my example in the other mail.
>
> I think before we add any new attributes we should sort out the
> current mess, eventually adding some testcases for desired
> diagnostic.
>
> Richard.
>
> >         Jakub
> >