On Thu, Mar 25, 2021 at 2:25 PM H.J. Lu via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Thu, Mar 25, 2021 at 6:13 AM Jakub Jelinek <ja...@redhat.com> wrote:
> >
> > On Thu, Mar 25, 2021 at 02:02:16PM +0100, Uros Bizjak wrote:
> > > > Aren't *intrin.h system headers too?
> > >
> > > I was under impression that they are not, since they live outside of
> > > /usr/include.
> >
> > Yes, they aren't in /usr/include, but they are still system headers.
> > If I preprocess something that #include <x86intrin.h> with my system
> > compiler, I get:
> > # 1 "/usr/lib/gcc/x86_64-redhat-linux/10/include/x86intrin.h" 1 3 4
> > where that 3 stands for system header.
> >
> > My preference would be a new attribute that for always_inline says it is ok
> > to inline even when there are target or optimization mismatches (and
> > effectively get the target/optimization options from the caller for the
> > body) and start using that new attribute in glibc headers (for
> > -D_FORTIFY_SOURCE wrappers at least, those really don't have any target
> > dependencies nor anything floating point that might e.g. depend on
> > -ffast-math etc.) and perhaps the __rdtsc and similar intrinsics in
> > *intrin.h.
> > Even that can be a can of worms, because some target or optimization options
> > are used already in the FE processing or during the GIMPLE passes before
> > inlining, and while it might work somehow if e.g. during those passes we
> > treat it like -ffast-math and after inlining not like that or vice versa,
> > there is a risk that we e.g. fold/lower something with some assumptions and
> > later assume that (with different options) such constructs can't appear in
> > the IL.
> >
> > > If the patch does not differentiate between system and user headers,
> > > then please revert it.
> >
> > It does but intrinsic headers are system headers.
> >
> > Jakub
> >
>
> Before my patch:
>
> [hjl@gnu-cfl-2 gcc]$ cat y.c
> #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
> [hjl@gnu-cfl-2 gcc]$ gcc -O2 -S y.c
> y.c: In function ‘bar’:
> y.c:13:6: warning: AVX512F vector return without AVX512F enabled
> changes the ABI [-Wpsabi]
> 13 | *p = _mm512_setzero_ps ();
> | ~~~^~~~~~~~~~~~~~~~~~~~~~
> In file included from
> /usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h:55,
> from
> /usr/lib/gcc/x86_64-redhat-linux/10/include/x86intrin.h:32,
> from y.c:1:
> /usr/lib/gcc/x86_64-redhat-linux/10/include/avx512fintrin.h:310:1:
> error: inlining failed in call to ‘always_inline’ ‘_mm512_setzero_ps’:
> target specific option mismatch
> 310 | _mm512_setzero_ps (void)
> | ^~~~~~~~~~~~~~~~~
> y.c:13:8: note: called from here
> 13 | *p = _mm512_setzero_ps ();
> | ^~~~~~~~~~~~~~~~~~~~
> [hjl@gnu-cfl-2 gcc]$ gcc -O2 -S y.c -DFOO
> y.c: In function ‘foo’:
> y.c:7:6: warning: AVX512F vector return without AVX512F enabled
> changes the ABI [-Wpsabi]
> 7 | *p = _mm512_setzero_ps ();
> | ~~~^~~~~~~~~~~~~~~~~~~~~~
> In file included from
> /usr/lib/gcc/x86_64-redhat-linux/10/include/immintrin.h:55,
> from
> /usr/lib/gcc/x86_64-redhat-linux/10/include/x86intrin.h:32,
> from y.c:1:
> /usr/lib/gcc/x86_64-redhat-linux/10/include/avx512fintrin.h:310:1:
> error: inlining failed in call to ‘always_inline’ ‘_mm512_setzero_ps’:
> target specific option mismatch
> 310 | _mm512_setzero_ps (void)
> | ^~~~~~~~~~~~~~~~~
> y.c:7:8: note: called from here
> 7 | *p = _mm512_setzero_ps ();
> | ^~~~~~~~~~~~~~~~~~~~
> [hjl@gnu-cfl-2 gcc]$
>
> After my patch,
>
> [hjl@gnu-cfl-2 gcc]$ ./xgcc -B./ -O2 -S y.c -DFOO
> y.c: In function ‘foo’:
> y.c:7:6: warning: AVX512F vector return without AVX512F enabled
> changes the ABI [-Wpsabi]
> 7 | *p = _mm512_setzero_ps ();
> | ~~~^~~~~~~~~~~~~~~~~~~~~~
> In file included from ./include/immintrin.h:49,
> from ./include/x86intrin.h:32,
> from y.c:1:
> ./include/avx512fintrin.h:305:1: error: inlining failed in call to
> ‘always_inline’ ‘_mm512_setzero_ps’: target specific option mismatch
> 305 | _mm512_setzero_ps (void)
> | ^~~~~~~~~~~~~~~~~
> y.c:7:8: note: called from here
> 7 | *p = _mm512_setzero_ps ();
> | ^~~~~~~~~~~~~~~~~~~~
> [hjl@gnu-cfl-2 gcc]$ ./xgcc -B./ -O2 -S y.c
> y.c: In function ‘bar’:
> y.c:13:6: warning: AVX512F vector return without AVX512F enabled
> changes the ABI [-Wpsabi]
> 13 | *p = _mm512_setzero_ps ();
> | ~~~^~~~~~~~~~~~~~~~~~~~~~
> [hjl@gnu-cfl-2 gcc]$
>
> If you look at the generated code:
>
> vpxor %xmm0, %xmm0, %xmm0
> vmovdqa %xmm0, (%rdi)
> vmovdqa %xmm0, 16(%rdi)
> vmovdqa %xmm0, 32(%rdi)
> vmovdqa %xmm0, 48(%rdi)
> ret
>
> The ABI change warning is on _mm512_setzero_ps. Since it is inlined,
> there is no wrong code here. I don't believe my patch will cause the wrong
> code nor ICE.
The ICE chance is that we fail to expand some __builtin_ia32_* or that
we expand it but will not recognize the used insn because it is gated on
a not enabled architecture feature. The fix would of course be to
fail expansion with a proper diagnostic here, but not sure if we reliably
do this.
That said, I agree with the direction of the patch but I'd have removed
the system header check entirely - I'd even have done this change in
the middle-end and avoid having the target particicpate in inlining decisions
of always-inline functions.
Richard.
>
> --
> H.J.
