[RFC,v2,1/2] RISC-V: __builtin_riscv_pause for all environment
Checks
Commit Message
From: Tsukasa OI <research_trasio@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.
---
gcc/common/config/riscv/riscv-common.cc | 2 ++
gcc/config/riscv/riscv-builtins.cc | 6 ++++--
gcc/config/riscv/riscv-opts.h | 2 ++
gcc/config/riscv/riscv.md | 7 ++++++-
gcc/testsuite/gcc.target/riscv/builtin_pause.c | 10 ----------
gcc/testsuite/gcc.target/riscv/zihintpause-1.c | 11 +++++++++++
gcc/testsuite/gcc.target/riscv/zihintpause-2.c | 11 +++++++++++
gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c | 12 ++++++++++++
8 files changed, 48 insertions(+), 13 deletions(-)
delete mode 100644 gcc/testsuite/gcc.target/riscv/builtin_pause.c
create mode 100644 gcc/testsuite/gcc.target/riscv/zihintpause-1.c
create mode 100644 gcc/testsuite/gcc.target/riscv/zihintpause-2.c
create mode 100644 gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c
Comments
On 8/9/23 20:25, Tsukasa OI wrote:
> From: Tsukasa OI <research_trasio@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
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
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_trasio@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
On 8/16/23 02:33, Philipp Tomsich wrote:
>
> 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)?
It seems reasonable from an encoding standpoint. But I haven't been
able to convince the assembler to encode a 0 into the pred/succ fields.
Jeff
@@ -209,6 +209,7 @@ static const struct riscv_ext_version riscv_ext_version_table[] =
{"zkt", ISA_SPEC_CLASS_NONE, 1, 0},
{"zihintntl", ISA_SPEC_CLASS_NONE, 1, 0},
+ {"zihintpause", ISA_SPEC_CLASS_NONE, 2, 0},
{"zicboz",ISA_SPEC_CLASS_NONE, 1, 0},
{"zicbom",ISA_SPEC_CLASS_NONE, 1, 0},
@@ -1343,6 +1344,7 @@ static const riscv_ext_flag_table_t riscv_ext_flag_table[] =
{"zkt", &gcc_options::x_riscv_zk_subext, MASK_ZKT},
{"zihintntl", &gcc_options::x_riscv_zi_subext, MASK_ZIHINTNTL},
+ {"zihintpause", &gcc_options::x_riscv_zi_subext, MASK_ZIHINTPAUSE},
{"zicboz", &gcc_options::x_riscv_zicmo_subext, MASK_ZICBOZ},
{"zicbom", &gcc_options::x_riscv_zicmo_subext, MASK_ZICBOM},
@@ -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
@@ -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)
@@ -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")
+
;;
;; ....................
;;
deleted file mode 100644
@@ -1,10 +0,0 @@
-/* { dg-do compile } */
-/* { dg-options "-O2" } */
-
-void test_pause()
-{
- __builtin_riscv_pause ();
-}
-
-/* { dg-final { scan-assembler "pause" } } */
-
new file mode 100644
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64i_zihintpause -mabi=lp64" } */
+/* { dg-skip-if "" { *-*-* } { "-g" "-flto"} } */
+
+void
+test ()
+{
+ __builtin_riscv_pause ();
+}
+
+/* { dg-final { scan-assembler-times "\tpause" 1 } } */
new file mode 100644
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv32i_zihintpause -mabi=ilp32" } */
+/* { dg-skip-if "" { *-*-* } { "-g" "-flto"} } */
+
+void
+test ()
+{
+ __builtin_riscv_pause ();
+}
+
+/* { dg-final { scan-assembler-times "\tpause" 1 } } */
new file mode 100644
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64i -mabi=lp64" { target { rv64 } } } */
+/* { dg-options "-march=rv32i -mabi=ilp32" { target { rv32 } } } */
+/* { dg-skip-if "" { *-*-* } { "-g" "-flto"} } */
+
+void
+test ()
+{
+ __builtin_riscv_pause ();
+}
+
+/* { dg-final { scan-assembler-times "0x0100000f" 1 } } */