On Thu, Mar 25, 2021 at 2:03 PM Uros Bizjak via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Thu, Mar 25, 2021 at 1:54 PM Jakub Jelinek <ja...@redhat.com> wrote:
> >
> > On Wed, Mar 24, 2021 at 10:23:44AM -0700, H.J. Lu via Gcc-patches wrote:
> > > For always_inline in system headers, we don't know if caller's ISAs are
> > > compatible with callee's ISAs until much later. Skip ISA check for
> > > always_inline in system headers if caller has target attribute.
> > >
> > > gcc/
> > >
> > > PR target/98209
> > > PR target/99744
> > > * config/i386/i386.c (ix86_can_inline_p): Don't check ISA for
> > > always_inline in system headers.
> >
> > Aren't *intrin.h system headers too?
>
> I was under impression that they are not, since they live outside of
> /usr/include.
>
> > Doesn't this mean we can now inline all the intrinsics if the caller doesn't
> > have the default target options and doesn't have the needed ISA?
>
> No, we should not.
>
>
> > Consider e.g.
> > #include <x86intrin.h>
> >
> > #ifdef FOO
> > void
> > foo (__m512 *p)
> > {
> > *p = _mm512_setzero_ps ();
> > }
> > #else
> > __attribute__((target ("avx"))) void
> > bar (__m512 *p)
> > {
> > *p = _mm512_setzero_ps ();
> > }
> > #endif
> >
> > #ifdef FOO
> > void
> > baz (__m512d *p, __m512d *q, int mask)
> > {
> > *p = _mm512_mask_mov_pd (*p, mask, *q);
> > }
> > #else
> > __attribute__((target ("avx"))) void
> > qux (__m512d *p, __m512d *q, int mask)
> > {
> > *p = _mm512_mask_mov_pd (*p, mask, *q);
> > }
> > #endif
> >
> > If you compile this without your patch, you'll get
> > inlining failed in call to ‘always_inline’ ‘_mm512_setzero_ps’: target
> > specific option mismatch
> > errors in all cases (always the first one), but with your patch
> > the _mm512_setzero_ps (); gets through completely and on the mask move
> > one gets instead
> > ‘__builtin_ia32_movapd512_mask’ needs isa option -mavx512f
> > error and
> > the ABI for passing parameters with 64-byte alignment has changed in GCC 4.6
> > note. IMNSHO this change needs to be reverted and we need to come up with a
> > way (some attribute) to say explicitly whether we can or can't inline that
> > always_inline function despite target specific option mismatches.
>
> If the patch does not differentiate between system and user headers,
> then please revert it.
Note that my suggestion to give leeway to always_inline annotated functions
wasn't restricted to system headers but would apply generally with the logic
that if people use always_inline then they should better make sure that
such inlining is valid - after all always_inline is an attribute that should be
used if inlining is required for functional correctness, it is _not_
an optimization
hint.
Now IIRC there were some cases where we end up with obscure ICEs when
using the "wrong" intrinsics in a function context that has certain ISA features
disabled. But then I might misremember.
Richard.
> Uros.
