From: "H.J. Lu" <hjl.tools@gmail.com>
To: Richard Biener <richard.guenther@gmail.com>,
Uros Bizjak <ubizjak@gmail.com>,
Hongyu Wang <wwwhhhyyy333@gmail.com>,
Hongtao Liu <crazylht@gmail.com>
Cc: Jakub Jelinek <jakub@redhat.com>, GCC Patches <gcc-patches@gcc.gnu.org>
Subject: PING^1 [PATCH v5] <x86gprintrin.h>: Add pragma GCC target("general-regs-only")
Date: Sat, 31 Jul 2021 08:35:19 -0700 [thread overview]
Message-ID: <CAMe9rOon4hhBnTQd-DY9T5_wkOE7RwWiFJCqTWHTVMpQn3XTgA@mail.gmail.com> (raw)
In-Reply-To: <CAMe9rOpyx_wMGJaESrokKrM+wbPQYSYz9z1QVdYZUXDpmZpy8g@mail.gmail.com>
On Sat, Jul 17, 2021 at 6:45 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, Apr 22, 2021 at 7:30 AM Richard Biener via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > 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
>
> Here is the v5 patch:
>
> 1. Intrinsics in <x86gprintrin.h> only require GPR ISAs. Add
>
> #if defined __MMX__ || defined __SSE__
> #pragma GCC push_options
> #pragma GCC target("general-regs-only")
> #define __DISABLE_GENERAL_REGS_ONLY__
> #endif
>
> and
>
> #ifdef __DISABLE_GENERAL_REGS_ONLY__
> #undef __DISABLE_GENERAL_REGS_ONLY__
> #pragma GCC pop_options
> #endif /* __DISABLE_GENERAL_REGS_ONLY__ */
>
> to <x86gprintrin.h> to disable non-GPR ISAs so that they can be used in
> functions with __attribute__ ((target("general-regs-only"))).
> 2. When checking always_inline attribute, if callee only uses GPRs,
> ignore MASK_80387 since enable MASK_80387 in caller has no impact on
> callee inline.
>
> OK for master?
>
PING.
--
H.J.
next prev parent reply other threads:[~2021-07-31 15:35 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-14 22:39 [PATCH v4 0/2] x86: Add general_regs_only function attribute H.J. Lu
2021-04-14 22:39 ` [PATCH v4 1/2] x86: Move OPTION_MASK_* to i386-common.h H.J. Lu
2021-04-14 22:39 ` [PATCH v4 2/2] x86: Add general_regs_only function attribute H.J. Lu
2021-04-21 7:30 ` Uros Bizjak
2021-04-21 13:47 ` H.J. Lu
2021-04-21 16:54 ` Martin Sebor
2021-04-21 17:09 ` Martin Sebor
2021-04-21 20:58 ` H.J. Lu
2021-04-21 23:23 ` Martin Sebor
2021-04-22 1:01 ` H.J. Lu
2021-04-22 8:27 ` Richard Biener
2021-04-22 9:02 ` Jakub Jelinek
2021-04-22 11:23 ` Richard Biener
2021-04-22 11:57 ` H.J. Lu
2021-04-22 12:16 ` Richard Biener
2021-04-22 12:22 ` Jakub Jelinek
2021-04-22 12:52 ` Richard Biener
2021-04-22 12:55 ` Richard Biener
2021-07-18 1:45 ` [PATCH v5] <x86gprintrin.h>: Add pragma GCC target("general-regs-only") H.J. Lu
2021-07-31 15:35 ` H.J. Lu [this message]
2021-08-03 11:47 ` Richard Biener
2021-08-03 14:45 ` [PATCH v6] " H.J. Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAMe9rOon4hhBnTQd-DY9T5_wkOE7RwWiFJCqTWHTVMpQn3XTgA@mail.gmail.com \
--to=hjl.tools@gmail.com \
--cc=crazylht@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=richard.guenther@gmail.com \
--cc=ubizjak@gmail.com \
--cc=wwwhhhyyy333@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).