On Wed, 16 Aug 2023 at 03:27, Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 8/9/23 20:25, Tsukasa OI wrote:
> > From: Tsukasa OI <research_tra...@irq.a4lg.com>
> >
> > The "pause" RISC-V hint instruction requires the 'Zihintpause' extension
> > (in the assembler). However, GCC emits "pause" unconditionally, making
> > an assembler error while compiling code with __builtin_riscv_pause while
> > the 'Zihintpause' extension disabled.
> >
> > However, the "pause" instruction code (0x0100000f) is a HINT and emitting
> > its instruction code is safe in any environment.
> >
> > This commit implements handling for the 'Zihintpause' extension and emits
> > ".insn 0x0100000f" instead of "pause" only if the extension is disabled
> > (making the diagnostics better).
> >
> > gcc/ChangeLog:
> >
> > * common/config/riscv/riscv-common.cc
> > (riscv_ext_version_table): Implement the 'Zihintpause' extension,
> > version 2.0. (riscv_ext_flag_table) Add 'Zihintpause' handling.
> > * config/riscv/riscv-builtins.cc: Remove availability predicate
> > "always" and add "hint_pause" and "hint_pause_pseudo", corresponding
> > the existence of the 'Zihintpause' extension.
> > (riscv_builtins) Split builtin implementation depending on the
> > existence of the 'Zihintpause' extension.
> > * config/riscv/riscv-opts.h
> > (MASK_ZIHINTPAUSE, TARGET_ZIHINTPAUSE): New.
> > * config/riscv/riscv.md (riscv_pause): Make it only available when
> > the 'Zihintpause' extension is enabled. (riscv_pause_insn) New
> > "pause" implementation when the 'Zihintpause' extension is disabled.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * gcc.target/riscv/builtin_pause.c: Removed.
> > * gcc.target/riscv/zihintpause-1.c:
> > New test when the 'Zihintpause' extension is enabled.
> > * gcc.target/riscv/zihintpause-2.c: Likewise.
> > * gcc.target/riscv/zihintpause-noarch.c:
> > New test when the 'Zihintpause' extension is disabled.
> So the conclusion from today's meeting was to make this available
> irrespective of the extension set. So I've dropped the alternate patch
> from patchwork.
>
>
> > diff --git a/gcc/config/riscv/riscv-builtins.cc
> > b/gcc/config/riscv/riscv-builtins.cc
> > index 79681d759628..554fb7f69bb0 100644
> > --- a/gcc/config/riscv/riscv-builtins.cc
> > +++ b/gcc/config/riscv/riscv-builtins.cc
> > @@ -122,7 +122,8 @@ AVAIL (clmul_zbkc32_or_zbc32, (TARGET_ZBKC ||
> > TARGET_ZBC) && !TARGET_64BIT)
> > AVAIL (clmul_zbkc64_or_zbc64, (TARGET_ZBKC || TARGET_ZBC) && TARGET_64BIT)
> > AVAIL (clmulr_zbc32, TARGET_ZBC && !TARGET_64BIT)
> > AVAIL (clmulr_zbc64, TARGET_ZBC && TARGET_64BIT)
> > -AVAIL (always, (!0))
> > +AVAIL (hint_pause, TARGET_ZIHINTPAUSE)
> > +AVAIL (hint_pause_pseudo, !TARGET_ZIHINTPAUSE)
> >
> > /* Construct a riscv_builtin_description from the given arguments.
> >
> > @@ -179,7 +180,8 @@ static const struct riscv_builtin_description
> > riscv_builtins[] = {
> >
> > DIRECT_BUILTIN (frflags, RISCV_USI_FTYPE, hard_float),
> > DIRECT_NO_TARGET_BUILTIN (fsflags, RISCV_VOID_FTYPE_USI, hard_float),
> > - DIRECT_NO_TARGET_BUILTIN (pause, RISCV_VOID_FTYPE, always),
> > + RISCV_BUILTIN (pause, "pause", RISCV_BUILTIN_DIRECT_NO_TARGET,
> > RISCV_VOID_FTYPE, hint_pause),
> > + RISCV_BUILTIN (pause_insn, "pause", RISCV_BUILTIN_DIRECT_NO_TARGET,
> > RISCV_VOID_FTYPE, hint_pause_pseudo),
> > };
> >
> > /* Index I is the function declaration for riscv_builtins[I], or null if
> > the
> > diff --git a/gcc/config/riscv/riscv-opts.h b/gcc/config/riscv/riscv-opts.h
> > index 28d9b81bd800..a6c3e0c9098f 100644
> > --- a/gcc/config/riscv/riscv-opts.h
> > +++ b/gcc/config/riscv/riscv-opts.h
> > @@ -102,10 +102,12 @@ enum riscv_entity
> > #define MASK_ZICSR (1 << 0)
> > #define MASK_ZIFENCEI (1 << 1)
> > #define MASK_ZIHINTNTL (1 << 2)
> > +#define MASK_ZIHINTPAUSE (1 << 3)
> >
> > #define TARGET_ZICSR ((riscv_zi_subext & MASK_ZICSR) != 0)
> > #define TARGET_ZIFENCEI ((riscv_zi_subext & MASK_ZIFENCEI) != 0)
> > #define TARGET_ZIHINTNTL ((riscv_zi_subext & MASK_ZIHINTNTL) != 0)
> > +#define TARGET_ZIHINTPAUSE ((riscv_zi_subext & MASK_ZIHINTPAUSE) != 0)
> >
> > #define MASK_ZAWRS (1 << 0)
> > #define TARGET_ZAWRS ((riscv_za_subext & MASK_ZAWRS) != 0)
> > diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> > index 688fd697255b..a6cdb32e9408 100644
> > --- a/gcc/config/riscv/riscv.md
> > +++ b/gcc/config/riscv/riscv.md
> > @@ -2192,9 +2192,14 @@
> >
> > (define_insn "riscv_pause"
> > [(unspec_volatile [(const_int 0)] UNSPECV_PAUSE)]
> > - ""
> > + "TARGET_ZIHINTPAUSE"
> > "pause")
> >
> > +(define_insn "riscv_pause_insn"
> > + [(unspec_volatile [(const_int 0)] UNSPECV_PAUSE)]
> > + ""
> > + ".insn\t0x0100000f")
> > +
> So I was wondering if we'd be better off always emitting the .insn form
> with a comment on the line indicating it's a pause. ie something like
>
> .insn\t0x0100000f ;; pause
>
> That would allow the implementation to simplify down to a single
> unconditional pattern as well as simplifications elsewhere.
>
> Alternately we could do:
>
> TARGET_ZIHINTPAUSE ? pause : .insn\t0x0100000f
Could we use the underlying 'fence' instruction (unless the assembler
rejects the specific form that is needed) instead of the hex-insn?
Should this also check HAVE_AS_MARCH_ZIHINTPAUSE (which must then also
be added to configure.ac)?
Philipp.
> in a single pattern that is always available if you feel strongly that
> we should emit different code based on TARGET_ZIHINTPAUSE.
>
> I think both simplify the riscv-builtins.cc change a bit.
>
> Thoughts?
>
> jeff
