[RFC PATCH 0/2] RISC-V: __builtin_riscv_pause for all environment

[RFC PATCH 0/2] RISC-V: __builtin_riscv_pause for all environment

[Tsukasa OI]

Hello,

I found that a built-in function "__builtin_riscv_pause" is not usable
unless we enable the 'Zihintpause' extension explicitly (still, this
built-in exists EVEN IF the 'Zihintpause' extension is disabled).

Contents of a.c:

> void sample(void)
> {
>     __builtin_riscv_pause();
> }


Compiling with the 'Zihintpause' extension is fine.

> $ riscv64-unknown-elf-gcc -O2 -march=rv64i_zihintpause -mabi=lp64 -c a.c


However, compiling without the 'Zihintpause' causes an assembler error
(tested with GNU Binutils 2.41):

> $ riscv64-unknown-elf-gcc -O2 -march=rv64i -mabi=lp64 -c a.c
> /tmp/ccFjacAz.s: Assembler messages:
> /tmp/ccFjacAz.s:11: Error: unrecognized opcode `pause', extension `zihintpause' required


This is because:

1.  GCC does not handle the 'Zihintpause' extension and
2.  "riscv_pause" (insn) unconditionally emits "pause" even if the
    assembler does not accept it (when the extension is disabled).


This patch set (PATCH 1/2) resolves this issue by:

1.  Handling the 'Zihintpause' extension and
2.  Splitting the "__builtin_riscv_pause" implementation
    depending on the existence of the 'Zihintpause' extension.

Because a released version of GCC defines "__builtin_riscv_pause"
unconditionally, I chose to define no-'Zihintpause' version.

There is another option to unconditionally emit ".insn 0x0100000f"
(the machine code of "pause") but I didn't because I wanted to improve the
diagnostics (e.g. *.s output).

I also fixed the description of this built-in function (in PATCH 2/2).


I'm not sure whether this is a good method to split the implementation
depending on the 'Zihintpause' extension.  Other than that, I believe that
this is okay and approval is appreciated.

Note that because I assigned copyright of GCC contribution to FSF, I didn't
attach "Signed-off-by" tag.  Tell me if I need it.

Thanks,
Tsukasa




Tsukasa OI (2):
  RISC-V: __builtin_riscv_pause for all environment
  RISC-V: Fix documentation of __builtin_riscv_pause

 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/doc/extend.texi                                 |  6 +++---
 gcc/testsuite/gcc.target/riscv/builtin_pause.c      | 10 ----------
 gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c | 11 +++++++++++
 gcc/testsuite/gcc.target/riscv/zihintpause.c        | 11 +++++++++++
 8 files changed, 39 insertions(+), 16 deletions(-)
 delete mode 100644 gcc/testsuite/gcc.target/riscv/builtin_pause.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/zihintpause.c


base-commit: c8b396243ec5bfa9b541555131df597ebc84b9d0
-- 
2.41.0

[RFC PATCH 1/2] RISC-V: __builtin_riscv_pause for all environment

[Tsukasa OI]

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.c:
	New test when the 'Zihintpause' extension is enabled.
	* 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-noarch.c | 11 +++++++++++
 gcc/testsuite/gcc.target/riscv/zihintpause.c        | 11 +++++++++++
 7 files changed, 36 insertions(+), 13 deletions(-)
 delete mode 100644 gcc/testsuite/gcc.target/riscv/builtin_pause.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/zihintpause.c

diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
index 2eb8c7cadff0..02502ba07e82 100644
--- a/gcc/common/config/riscv/riscv-common.cc
+++ b/gcc/common/config/riscv/riscv-common.cc
@@ -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},
@@ -1344,6 +1345,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},
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..f2fcbfa6163b 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 0x0100000f")
+
 ;;
 ;;  ....................
 ;;
diff --git a/gcc/testsuite/gcc.target/riscv/builtin_pause.c b/gcc/testsuite/gcc.target/riscv/builtin_pause.c
deleted file mode 100644
index 9250937cabb9..000000000000
--- a/gcc/testsuite/gcc.target/riscv/builtin_pause.c
+++ /dev/null
@@ -1,10 +0,0 @@
-/* { dg-do compile } */
-/* { dg-options "-O2" }  */
-
-void test_pause()
-{
-  __builtin_riscv_pause ();
-}
-
-/* { dg-final { scan-assembler "pause" } } */
-
diff --git a/gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c b/gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c
new file mode 100644
index 000000000000..99ab953bd1db
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c
@@ -0,0 +1,11 @@
+/* { 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_pause()
+{
+  __builtin_riscv_pause ();
+}
+
+/* { dg-final { scan-assembler-times "0x0100000f" 1 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zihintpause.c b/gcc/testsuite/gcc.target/riscv/zihintpause.c
new file mode 100644
index 000000000000..575b42277705
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zihintpause.c
@@ -0,0 +1,11 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64i_zihintpause -mabi=lp64"  { target { rv64 } } } */
+/* { dg-options "-march=rv32i_zihintpause -mabi=ilp32" { target { rv32 } } } */
+/* { dg-skip-if "" { *-*-* } { "-g" "-flto"} } */
+
+void test_pause()
+{
+  __builtin_riscv_pause ();
+}
+
+/* { dg-final { scan-assembler-times "\tpause" 1 } } */
-- 
2.41.0

[RFC PATCH 2/2] RISC-V: Fix documentation of __builtin_riscv_pause

[Tsukasa OI]

From: Tsukasa OI <research_trasio@irq.a4lg.com>

This built-in does not imply the 'Xgnuzihintpausestate' extension.
It does not change architectural state (because all HINTs are prohibited
from doing that).

gcc/ChangeLog:

	* doc/extend.texi: Fix the description of __builtin_riscv_pause.
---
 gcc/doc/extend.texi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 89c5b4ea2b20..7ebbe70c78d6 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -21570,9 +21570,9 @@ Returns the value that is currently set in the @samp{tp} register.
 @enddefbuiltin
 
 @defbuiltin{void __builtin_riscv_pause (void)}
-Generates the @code{pause} (hint) machine instruction.  This implies the
-Xgnuzihintpausestate extension, which redefines the @code{pause} instruction to
-change architectural state.
+Generates the @code{pause} (hint) machine instruction.  If the target implements
+the Zihintpause extension, it indicates that the current hart should be
+temporarily paused or slowed down.
 @enddefbuiltin
 
 @node RISC-V Vector Intrinsics
-- 
2.41.0

Re: [RFC PATCH 0/2] RISC-V: __builtin_riscv_pause for all environment

[Jeff Law]

On 8/9/23 00:11, Tsukasa OI via Gcc-patches wrote:
> Hello,
> 
> I found that a built-in function "__builtin_riscv_pause" is not usable
> unless we enable the 'Zihintpause' extension explicitly (still, this
> built-in exists EVEN IF the 'Zihintpause' extension is disabled).
> 
> Contents of a.c:
> 
>> void sample(void)
>> {
>>      __builtin_riscv_pause();
>> }
> 
> 
> Compiling with the 'Zihintpause' extension is fine.
> 
>> $ riscv64-unknown-elf-gcc -O2 -march=rv64i_zihintpause -mabi=lp64 -c a.c
> 
> 
> However, compiling without the 'Zihintpause' causes an assembler error
> (tested with GNU Binutils 2.41):
> 
>> $ riscv64-unknown-elf-gcc -O2 -march=rv64i -mabi=lp64 -c a.c
>> /tmp/ccFjacAz.s: Assembler messages:
>> /tmp/ccFjacAz.s:11: Error: unrecognized opcode `pause', extension `zihintpause' required
> 
> 
> This is because:
> 
> 1.  GCC does not handle the 'Zihintpause' extension and
> 2.  "riscv_pause" (insn) unconditionally emits "pause" even if the
>      assembler does not accept it (when the extension is disabled).
> 
> 
> This patch set (PATCH 1/2) resolves this issue by:
> 
> 1.  Handling the 'Zihintpause' extension and
> 2.  Splitting the "__builtin_riscv_pause" implementation
>      depending on the existence of the 'Zihintpause' extension.
> 
> Because a released version of GCC defines "__builtin_riscv_pause"
> unconditionally, I chose to define no-'Zihintpause' version.
> 
> There is another option to unconditionally emit ".insn 0x0100000f"
> (the machine code of "pause") but I didn't because I wanted to improve the
> diagnostics (e.g. *.s output).
> 
> I also fixed the description of this built-in function (in PATCH 2/2).
> 
> 
> I'm not sure whether this is a good method to split the implementation
> depending on the 'Zihintpause' extension.  Other than that, I believe that
> this is okay and approval is appreciated.
> 
> Note that because I assigned copyright of GCC contribution to FSF, I didn't
> attach "Signed-off-by" tag.  Tell me if I need it.
I'd tend to think we do not want to expose the intrinsic unless the 
right extensions are enabled -- even though the encoding is a no-op and 
we could emit it as a .insn.

If others think otherwise, I'll go with the consensus opinion.  So let's 
hold off a bit until others have chimed in.

Thanks,
jeff

Re: [RFC PATCH 0/2] RISC-V: __builtin_riscv_pause for all environment

[Tsukasa OI]

On 2023/08/10 5:05, Jeff Law wrote:
> 
> 
> On 8/9/23 00:11, Tsukasa OI via Gcc-patches wrote:
>> Hello,
>>
>> I found that a built-in function "__builtin_riscv_pause" is not usable
>> unless we enable the 'Zihintpause' extension explicitly (still, this
>> built-in exists EVEN IF the 'Zihintpause' extension is disabled).
>>
>> Contents of a.c:
>>
>>> void sample(void)
>>> {
>>>      __builtin_riscv_pause();
>>> }
>>
>>
>> Compiling with the 'Zihintpause' extension is fine.
>>
>>> $ riscv64-unknown-elf-gcc -O2 -march=rv64i_zihintpause -mabi=lp64 -c a.c
>>
>>
>> However, compiling without the 'Zihintpause' causes an assembler error
>> (tested with GNU Binutils 2.41):
>>
>>> $ riscv64-unknown-elf-gcc -O2 -march=rv64i -mabi=lp64 -c a.c
>>> /tmp/ccFjacAz.s: Assembler messages:
>>> /tmp/ccFjacAz.s:11: Error: unrecognized opcode `pause', extension
>>> `zihintpause' required
>>
>>
>> This is because:
>>
>> 1.  GCC does not handle the 'Zihintpause' extension and
>> 2.  "riscv_pause" (insn) unconditionally emits "pause" even if the
>>      assembler does not accept it (when the extension is disabled).
>>
>>
>> This patch set (PATCH 1/2) resolves this issue by:
>>
>> 1.  Handling the 'Zihintpause' extension and
>> 2.  Splitting the "__builtin_riscv_pause" implementation
>>      depending on the existence of the 'Zihintpause' extension.
>>
>> Because a released version of GCC defines "__builtin_riscv_pause"
>> unconditionally, I chose to define no-'Zihintpause' version.
>>
>> There is another option to unconditionally emit ".insn 0x0100000f"
>> (the machine code of "pause") but I didn't because I wanted to improve
>> the
>> diagnostics (e.g. *.s output).
>>
>> I also fixed the description of this built-in function (in PATCH 2/2).
>>
>>
>> I'm not sure whether this is a good method to split the implementation
>> depending on the 'Zihintpause' extension.  Other than that, I believe
>> that
>> this is okay and approval is appreciated.
>>
>> Note that because I assigned copyright of GCC contribution to FSF, I
>> didn't
>> attach "Signed-off-by" tag.  Tell me if I need it.
> I'd tend to think we do not want to expose the intrinsic unless the
> right extensions are enabled -- even though the encoding is a no-op and
> we could emit it as a .insn.

I think that makes sense.  The only reason I implemented the
no-'Zihintpause' version is because GCC 13 implemented the built-in
unconditionally.  If the compatibility breakage is considered minimum (I
don't know, though), I'm ready to submit 'Zihintpause'-only version of
this patch set.

Thanks,
Tsukasa

> 
> If others think otherwise, I'll go with the consensus opinion.  So let's
> hold off a bit until others have chimed in.
> 
> Thanks,
> jeff
> 

[RFC PATCH v2 0/2] RISC-V: __builtin_riscv_pause for all environment

[Tsukasa OI]

**WARNING**

Following patch sets are exclusive:

1.  [RFC PATCH v2] RISC-V: __builtin_riscv_pause for all environment (this)
2.  [RFC PATCH] RISC-V: Make __builtin_riscv_pause 'Zihintpause' only

See <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626748.html>
for the background of this patch set.


Changes: v1 -> v2

*   Improve test case to test both RV32 and RV64 (+'Zihintpause').


Comparison: Patch sets [1] (this) and [2]

*   [1] completely preserves the compatibility with GCC 13
    ([2] removes __builtin_riscv_pause if the 'Zihintpause' extension is
     absent, making a minor compatibility issue)
*   Because of the nature of the instrinsic, [2] is more natural
    ("pause" is defined in the 'Zihintpause' extension).


Please consider those patch sets and decide which to apply.

Sincerely,
Tsukasa




Tsukasa OI (2):
  RISC-V: __builtin_riscv_pause for all environment
  RISC-V: Fix documentation of __builtin_riscv_pause

 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/doc/extend.texi                                 |  6 +++---
 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 ++++++++++++
 9 files changed, 51 insertions(+), 16 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


base-commit: 9b099a83b45b8fcdfc07d518e05d36ea741b2227
-- 
2.41.0

[RFC PATCH v2 1/2] RISC-V: __builtin_riscv_pause for all environment

[Tsukasa OI]

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

diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
index f25131eab28b..b77fdb909567 100644
--- a/gcc/common/config/riscv/riscv-common.cc
+++ b/gcc/common/config/riscv/riscv-common.cc
@@ -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},
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")
+
 ;;
 ;;  ....................
 ;;
diff --git a/gcc/testsuite/gcc.target/riscv/builtin_pause.c b/gcc/testsuite/gcc.target/riscv/builtin_pause.c
deleted file mode 100644
index 9250937cabb9..000000000000
--- a/gcc/testsuite/gcc.target/riscv/builtin_pause.c
+++ /dev/null
@@ -1,10 +0,0 @@
-/* { dg-do compile } */
-/* { dg-options "-O2" }  */
-
-void test_pause()
-{
-  __builtin_riscv_pause ();
-}
-
-/* { dg-final { scan-assembler "pause" } } */
-
diff --git a/gcc/testsuite/gcc.target/riscv/zihintpause-1.c b/gcc/testsuite/gcc.target/riscv/zihintpause-1.c
new file mode 100644
index 000000000000..fc86efe55902
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zihintpause-1.c
@@ -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 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zihintpause-2.c b/gcc/testsuite/gcc.target/riscv/zihintpause-2.c
new file mode 100644
index 000000000000..4eaca95e9f02
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zihintpause-2.c
@@ -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 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c b/gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c
new file mode 100644
index 000000000000..7ce5cba90d51
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c
@@ -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 } } */
-- 
2.41.0

[RFC PATCH v2 2/2] RISC-V: Fix documentation of __builtin_riscv_pause

[Tsukasa OI]

From: Tsukasa OI <research_trasio@irq.a4lg.com>

This built-in does not imply the 'Xgnuzihintpausestate' extension.
It does not change architectural state (because all HINTs are prohibited
from doing that).

gcc/ChangeLog:

	* doc/extend.texi: Fix the description of __builtin_riscv_pause.
---
 gcc/doc/extend.texi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 89c5b4ea2b20..7ebbe70c78d6 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -21570,9 +21570,9 @@ Returns the value that is currently set in the @samp{tp} register.
 @enddefbuiltin
 
 @defbuiltin{void __builtin_riscv_pause (void)}
-Generates the @code{pause} (hint) machine instruction.  This implies the
-Xgnuzihintpausestate extension, which redefines the @code{pause} instruction to
-change architectural state.
+Generates the @code{pause} (hint) machine instruction.  If the target implements
+the Zihintpause extension, it indicates that the current hart should be
+temporarily paused or slowed down.
 @enddefbuiltin
 
 @node RISC-V Vector Intrinsics
-- 
2.41.0

[RFC PATCH 0/2] RISC-V: Make __builtin_riscv_pause 'Zihintpause' only

[Tsukasa OI]

**WARNING**

Following patch sets are exclusive:

1.  [RFC PATCH v2] RISC-V: __builtin_riscv_pause for all environment
2.  [RFC PATCH] RISC-V: Make __builtin_riscv_pause 'Zihintpause' only (this)

See <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626748.html>
for the background of this patch set.


Changes: (all environment) v1 -> ('Zihintpause'-only) v1

*   Based on the feedback from Jeff Law, I made an alternative patch set
    which makes "__builtin_riscv_pause" 'Zihintpause'-only.


Comparison: Patch sets [1] and [2] (this)

*   [1] completely preserves the compatibility with GCC 13
    ([2] removes __builtin_riscv_pause if the 'Zihintpause' extension is
     absent, making a minor compatibility issue)
*   Because of the nature of the instrinsic, [2] is more natural
    ("pause" is defined in the 'Zihintpause' extension).


Please consider those patch sets and decide which to apply.

Sincerely,
Tsukasa




Tsukasa OI (2):
  RISC-V: Make __builtin_riscv_pause 'Zihintpause' only
  RISC-V: Fix documentation of __builtin_riscv_pause

 gcc/common/config/riscv/riscv-common.cc        |  2 ++
 gcc/config/riscv/riscv-builtins.cc             |  4 ++--
 gcc/config/riscv/riscv-opts.h                  |  2 ++
 gcc/config/riscv/riscv.md                      |  2 +-
 gcc/doc/extend.texi                            |  6 +++---
 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 +++++++++++
 8 files changed, 32 insertions(+), 16 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


base-commit: 9b099a83b45b8fcdfc07d518e05d36ea741b2227
-- 
2.41.0

[RFC PATCH 1/2] RISC-V: Make __builtin_riscv_pause 'Zihintpause' only

[Tsukasa OI]

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.

Despite that the "pause" instruction code (0x0100000f) is a HINT and
emitting its instruction code is safe in any environment, enabling the
built-in only for certain environment is not a good idea.

This commit implements handling for the 'Zihintpause' extension and makes
__built_riscv_pause built-in 'Zihintpause'-only.

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", corresponding the existence of the
	'Zihintpause' extension.
	(riscv_builtins) Change che "riscv_pause" requirements.
	* 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.

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/common/config/riscv/riscv-common.cc        |  2 ++
 gcc/config/riscv/riscv-builtins.cc             |  4 ++--
 gcc/config/riscv/riscv-opts.h                  |  2 ++
 gcc/config/riscv/riscv.md                      |  2 +-
 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 +++++++++++
 7 files changed, 29 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

diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
index f25131eab28b..b77fdb909567 100644
--- a/gcc/common/config/riscv/riscv-common.cc
+++ b/gcc/common/config/riscv/riscv-common.cc
@@ -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},
diff --git a/gcc/config/riscv/riscv-builtins.cc b/gcc/config/riscv/riscv-builtins.cc
index 79681d759628..73e1512a5992 100644
--- a/gcc/config/riscv/riscv-builtins.cc
+++ b/gcc/config/riscv/riscv-builtins.cc
@@ -122,7 +122,7 @@ 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)
 
 /* Construct a riscv_builtin_description from the given arguments.
 
@@ -179,7 +179,7 @@ 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),
+  DIRECT_NO_TARGET_BUILTIN (pause, RISCV_VOID_FTYPE, hint_pause),
 };
 
 /* 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..44ea1c54b96b 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -2192,7 +2192,7 @@
 
 (define_insn "riscv_pause"
   [(unspec_volatile [(const_int 0)] UNSPECV_PAUSE)]
-  ""
+  "TARGET_ZIHINTPAUSE"
   "pause")
 
 ;;
diff --git a/gcc/testsuite/gcc.target/riscv/builtin_pause.c b/gcc/testsuite/gcc.target/riscv/builtin_pause.c
deleted file mode 100644
index 9250937cabb9..000000000000
--- a/gcc/testsuite/gcc.target/riscv/builtin_pause.c
+++ /dev/null
@@ -1,10 +0,0 @@
-/* { dg-do compile } */
-/* { dg-options "-O2" }  */
-
-void test_pause()
-{
-  __builtin_riscv_pause ();
-}
-
-/* { dg-final { scan-assembler "pause" } } */
-
diff --git a/gcc/testsuite/gcc.target/riscv/zihintpause-1.c b/gcc/testsuite/gcc.target/riscv/zihintpause-1.c
new file mode 100644
index 000000000000..fc86efe55902
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zihintpause-1.c
@@ -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 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zihintpause-2.c b/gcc/testsuite/gcc.target/riscv/zihintpause-2.c
new file mode 100644
index 000000000000..4eaca95e9f02
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zihintpause-2.c
@@ -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 } } */
-- 
2.41.0

[RFC PATCH 2/2] RISC-V: Fix documentation of __builtin_riscv_pause

[Tsukasa OI]

From: Tsukasa OI <research_trasio@irq.a4lg.com>

This built-in does not imply the 'Xgnuzihintpausestate' extension.
It does not change architectural state (because all HINTs are prohibited
from doing that).

gcc/ChangeLog:

	* doc/extend.texi: Fix the description of __builtin_riscv_pause.
---
 gcc/doc/extend.texi | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index 89c5b4ea2b20..7be27430666a 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -21570,9 +21570,9 @@ Returns the value that is currently set in the @samp{tp} register.
 @enddefbuiltin
 
 @defbuiltin{void __builtin_riscv_pause (void)}
-Generates the @code{pause} (hint) machine instruction.  This implies the
-Xgnuzihintpausestate extension, which redefines the @code{pause} instruction to
-change architectural state.
+Generates the @code{pause} (hint) machine instruction, indicating that the
+current hart should be temporarily paused or slowed down.
+Available if the Zihintpause extension is enabled.
 @enddefbuiltin
 
 @node RISC-V Vector Intrinsics
-- 
2.41.0

Re: [RFC PATCH 0/2] RISC-V: __builtin_riscv_pause for all environment

[Jeff Law]

On 8/9/23 16:39, Tsukasa OI wrote:
> On 2023/08/10 5:05, Jeff Law wrote:

>> I'd tend to think we do not want to expose the intrinsic unless the
>> right extensions are enabled -- even though the encoding is a no-op and
>> we could emit it as a .insn.
> 
> I think that makes sense.  The only reason I implemented the
> no-'Zihintpause' version is because GCC 13 implemented the built-in
> unconditionally.  If the compatibility breakage is considered minimum (I
> don't know, though), I'm ready to submit 'Zihintpause'-only version of
> this patch set.
While it's a compatibility break I don't think we have a need to 
preserve this kind of compatibility.  I suspect anyone using 
__builtin_riscv_pause was probably already turning on Zihintpause and if 
they weren't they should have been :-0


I'm sure we'll kick this around in the Tuesday meeting and hopefully 
make a decision about the desired direction.  You're obviously welcome 
to join if you're inclined.  Let me know if you need an invite.

jeff

Re: [RFC PATCH 0/2] RISC-V: __builtin_riscv_pause for all environment

[Palmer Dabbelt]

On Fri, 11 Aug 2023 16:30:29 PDT (-0700), gcc-patches@gcc.gnu.org wrote:
>
>
> On 8/9/23 16:39, Tsukasa OI wrote:
>> On 2023/08/10 5:05, Jeff Law wrote:
>
>>> I'd tend to think we do not want to expose the intrinsic unless the
>>> right extensions are enabled -- even though the encoding is a no-op and
>>> we could emit it as a .insn.
>>
>> I think that makes sense.  The only reason I implemented the
>> no-'Zihintpause' version is because GCC 13 implemented the built-in
>> unconditionally.  If the compatibility breakage is considered minimum (I
>> don't know, though), I'm ready to submit 'Zihintpause'-only version of
>> this patch set.
> While it's a compatibility break I don't think we have a need to
> preserve this kind of compatibility.  I suspect anyone using
> __builtin_riscv_pause was probably already turning on Zihintpause and if
> they weren't they should have been :-0

I agree it's fine to just call this a bug: the builtin wasn't doing 
anything on non-Zihintpause systems anyway, so it's not like it could 
have been all that useful.

> I'm sure we'll kick this around in the Tuesday meeting and hopefully
> make a decision about the desired direction.  You're obviously welcome
> to join if you're inclined.  Let me know if you need an invite.
>
> jeff

Re: [RFC PATCH 0/2] RISC-V: __builtin_riscv_pause for all environment

[Tsukasa OI]

On 2023/08/12 8:30, Jeff Law wrote:
> 
> 
> On 8/9/23 16:39, Tsukasa OI wrote:
>> On 2023/08/10 5:05, Jeff Law wrote:
> 
>>> I'd tend to think we do not want to expose the intrinsic unless the
>>> right extensions are enabled -- even though the encoding is a no-op and
>>> we could emit it as a .insn.
>>
>> I think that makes sense.  The only reason I implemented the
>> no-'Zihintpause' version is because GCC 13 implemented the built-in
>> unconditionally.  If the compatibility breakage is considered minimum (I
>> don't know, though), I'm ready to submit 'Zihintpause'-only version of
>> this patch set.
> While it's a compatibility break I don't think we have a need to
> preserve this kind of compatibility.  I suspect anyone using
> __builtin_riscv_pause was probably already turning on Zihintpause and if
> they weren't they should have been :-0
> 
> 
> I'm sure we'll kick this around in the Tuesday meeting and hopefully
> make a decision about the desired direction.  You're obviously welcome
> to join if you're inclined.  Let me know if you need an invite.
> 
> jeff
> 

I'll not be able to attend that meeting due to Japanese religious events
around Aug 13-16 (it may not be impossible but at least difficult) but
look forward seeing that some conclusion is made.

I leave two patch sets corresponding two options so in either case, we
can apply a fix after the conclusion is made.

(1) __builtin_riscv_pause for 'Zihintpause'-only
<https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626912.html>
(2) __builtin_riscv_pause for all
<https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626909.html>

Thanks,
Tsukasa

Re: [RFC PATCH 0/2] RISC-V: __builtin_riscv_pause for all environment

[Philipp Tomsich]

On Sat, 12 Aug 2023 at 01:31, Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 8/9/23 16:39, Tsukasa OI wrote:
> > On 2023/08/10 5:05, Jeff Law wrote:
>
> >> I'd tend to think we do not want to expose the intrinsic unless the
> >> right extensions are enabled -- even though the encoding is a no-op and
> >> we could emit it as a .insn.
> >
> > I think that makes sense.  The only reason I implemented the
> > no-'Zihintpause' version is because GCC 13 implemented the built-in
> > unconditionally.  If the compatibility breakage is considered minimum (I
> > don't know, though), I'm ready to submit 'Zihintpause'-only version of
> > this patch set.
> While it's a compatibility break I don't think we have a need to
> preserve this kind of compatibility.  I suspect anyone using
> __builtin_riscv_pause was probably already turning on Zihintpause and if
> they weren't they should have been :-0
>
>
> I'm sure we'll kick this around in the Tuesday meeting and hopefully
> make a decision about the desired direction.  You're obviously welcome
> to join if you're inclined.  Let me know if you need an invite.

The original discussion (and I believe that Andrew was the decisive
voice in the end) came to the conclusion that—given that pause is a
true hint—it could always be enabled.
We had originally expected to enable it only if Zihintpause was part
of the target architecture, but viewing it as "just a name for an
already existing pure hint" also made sense.
Note that on systems that don't implement Zihintpause, the hint is
guarantueed to not have an architectural effect.

That said, I don't really have a strong leaning one way or another.
Philipp.

Re: [RFC PATCH 0/2] RISC-V: __builtin_riscv_pause for all environment

[Andrew Waterman]

On Sun, Aug 13, 2023 at 12:53 PM Philipp Tomsich
<philipp.tomsich@vrull.eu> wrote:
>
> On Sat, 12 Aug 2023 at 01:31, Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> >
> >
> > On 8/9/23 16:39, Tsukasa OI wrote:
> > > On 2023/08/10 5:05, Jeff Law wrote:
> >
> > >> I'd tend to think we do not want to expose the intrinsic unless the
> > >> right extensions are enabled -- even though the encoding is a no-op and
> > >> we could emit it as a .insn.
> > >
> > > I think that makes sense.  The only reason I implemented the
> > > no-'Zihintpause' version is because GCC 13 implemented the built-in
> > > unconditionally.  If the compatibility breakage is considered minimum (I
> > > don't know, though), I'm ready to submit 'Zihintpause'-only version of
> > > this patch set.
> > While it's a compatibility break I don't think we have a need to
> > preserve this kind of compatibility.  I suspect anyone using
> > __builtin_riscv_pause was probably already turning on Zihintpause and if
> > they weren't they should have been :-0
> >
> >
> > I'm sure we'll kick this around in the Tuesday meeting and hopefully
> > make a decision about the desired direction.  You're obviously welcome
> > to join if you're inclined.  Let me know if you need an invite.
>
> The original discussion (and I believe that Andrew was the decisive
> voice in the end) came to the conclusion that—given that pause is a
> true hint—it could always be enabled.

I continue to think that, since it's semantically valid to execute a
HINT on any implementation, there's little utility in ever rejecting
the HINT builtins, or in rejecting explicit HINTs in asm, irrespective
of -march.  But ultimately it isn't a big deal either way.

> We had originally expected to enable it only if Zihintpause was part
> of the target architecture, but viewing it as "just a name for an
> already existing pure hint" also made sense.
> Note that on systems that don't implement Zihintpause, the hint is
> guarantueed to not have an architectural effect.
>
> That said, I don't really have a strong leaning one way or another.
> Philipp.

Re: [RFC PATCH 0/2] RISC-V: __builtin_riscv_pause for all environment

[Jeff Law]

On 8/11/23 18:20, Tsukasa OI wrote:
>>
> 
> I'll not be able to attend that meeting due to Japanese religious events
> around Aug 13-16 (it may not be impossible but at least difficult) but
> look forward seeing that some conclusion is made.
No problem.  We hold that meeting weekly to work through any outstanding 
patches related to RISC-V.  You're always welcome to attend if you want.

> 
> I leave two patch sets corresponding two options so in either case, we
> can apply a fix after the conclusion is made.
> 
> (1) __builtin_riscv_pause for 'Zihintpause'-only
> <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626912.html>
> (2) __builtin_riscv_pause for all
> <https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626909.html>
Thanks.   It's not clear what direction we'll take on this, but having a 
patchkit for both potential outcomes is definitely helpful.

jeff

Re: [RFC PATCH 0/2] RISC-V: __builtin_riscv_pause for all environment

[Jeff Law]

On 8/13/23 13:52, Philipp Tomsich wrote:
> On Sat, 12 Aug 2023 at 01:31, Jeff Law via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>>
>> On 8/9/23 16:39, Tsukasa OI wrote:
>>> On 2023/08/10 5:05, Jeff Law wrote:
>>
>>>> I'd tend to think we do not want to expose the intrinsic unless the
>>>> right extensions are enabled -- even though the encoding is a no-op and
>>>> we could emit it as a .insn.
>>>
>>> I think that makes sense.  The only reason I implemented the
>>> no-'Zihintpause' version is because GCC 13 implemented the built-in
>>> unconditionally.  If the compatibility breakage is considered minimum (I
>>> don't know, though), I'm ready to submit 'Zihintpause'-only version of
>>> this patch set.
>> While it's a compatibility break I don't think we have a need to
>> preserve this kind of compatibility.  I suspect anyone using
>> __builtin_riscv_pause was probably already turning on Zihintpause and if
>> they weren't they should have been :-0
>>
>>
>> I'm sure we'll kick this around in the Tuesday meeting and hopefully
>> make a decision about the desired direction.  You're obviously welcome
>> to join if you're inclined.  Let me know if you need an invite.
> 
> The original discussion (and I believe that Andrew was the decisive
> voice in the end) came to the conclusion that—given that pause is a
> true hint—it could always be enabled.
> We had originally expected to enable it only if Zihintpause was part
> of the target architecture, but viewing it as "just a name for an
> already existing pure hint" also made sense.
> Note that on systems that don't implement Zihintpause, the hint is
> guarantueed to not have an architectural effect.
> 
> That said, I don't really have a strong leaning one way or another.
> Philipp.
I don't have a strong opinion either way and I certainly see both sides 
of the argument.

It sounds like the current situation is by design; knowing that now I 
would tend to lean towards keeping status quo, which would mean going 
with Tsukasa's first patch or something similar.

We'll certainly discuss on the call in a half-hour or so.

jeff

Re: [RFC PATCH v2 1/2] RISC-V: __builtin_riscv_pause for all environment

[Jeff Law]

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

Re: [RFC PATCH v2 1/2] RISC-V: __builtin_riscv_pause for all environment

[Philipp Tomsich]

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

Re: [RFC PATCH v2 1/2] RISC-V: __builtin_riscv_pause for all environment

[Jeff Law]

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

Re: [RFC PATCH 1/2] RISC-V: __builtin_riscv_pause for all environment

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]



On 8/9/23 00:11, Tsukasa OI via Gcc-patches 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.c:
> 	New test when the 'Zihintpause' extension is enabled.
> 	* gcc.target/riscv/zihintpause-noarch.c:
> 	New test when the 'Zihintpause' extension is disabled.
So I cleaned this up a bit per the list discussion and pushed the final 
result.  Hopefully I didn't goof anything too badly ;-)  The net is we 
emit "pause" or a suitable .insn based on TARGET_ZIHINTPAUSE.

Jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 6914 bytes --]

commit c2d04dd659c499d8df19f68d0602ad4c7d7065c2
Author: Tsukasa OI <research_trasio@irq.a4lg.com>
Date:   Mon Aug 28 15:04:13 2023 -0600

    RISC-V: __builtin_riscv_pause for all environment
    
    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".
            (riscv_builtins) : Add "pause" extension.
            * config/riscv/riscv-opts.h (MASK_ZIHINTPAUSE, TARGET_ZIHINTPAUSE): New.
            * config/riscv/riscv.md (riscv_pause): Adjust output based on
            TARGET_ZIHINTPAUSE.
    
    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.

diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
index 128a7020172..a5b62cda3a0 100644
--- a/gcc/common/config/riscv/riscv-common.cc
+++ b/gcc/common/config/riscv/riscv-common.cc
@@ -224,6 +224,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},
@@ -1381,6 +1382,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},
diff --git a/gcc/config/riscv/riscv-builtins.cc b/gcc/config/riscv/riscv-builtins.cc
index 79681d75962..8afe7b7e97d 100644
--- a/gcc/config/riscv/riscv-builtins.cc
+++ b/gcc/config/riscv/riscv-builtins.cc
@@ -122,7 +122,7 @@ 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, (!0))
 
 /* Construct a riscv_builtin_description from the given arguments.
 
@@ -179,7 +179,7 @@ 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),
 };
 
 /* 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 b1e05967c1f..5ed69abd214 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 47d14d99903..87f4a4a33f9 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -2275,7 +2275,7 @@ (define_insn "fence_i"
 (define_insn "riscv_pause"
   [(unspec_volatile [(const_int 0)] UNSPECV_PAUSE)]
   ""
-  "pause")
+  "* return TARGET_ZIHINTPAUSE ? \"pause\" : \".insn\t0x0100000f\";")
 
 ;;
 ;;  ....................
diff --git a/gcc/testsuite/gcc.target/riscv/builtin_pause.c b/gcc/testsuite/gcc.target/riscv/builtin_pause.c
deleted file mode 100644
index 9250937cabb..00000000000
--- a/gcc/testsuite/gcc.target/riscv/builtin_pause.c
+++ /dev/null
@@ -1,10 +0,0 @@
-/* { dg-do compile } */
-/* { dg-options "-O2" }  */
-
-void test_pause()
-{
-  __builtin_riscv_pause ();
-}
-
-/* { dg-final { scan-assembler "pause" } } */
-
diff --git a/gcc/testsuite/gcc.target/riscv/zihintpause-1.c b/gcc/testsuite/gcc.target/riscv/zihintpause-1.c
new file mode 100644
index 00000000000..fc86efe5590
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zihintpause-1.c
@@ -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 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zihintpause-2.c b/gcc/testsuite/gcc.target/riscv/zihintpause-2.c
new file mode 100644
index 00000000000..4eaca95e9f0
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zihintpause-2.c
@@ -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 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c b/gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c
new file mode 100644
index 00000000000..7ce5cba90d5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/zihintpause-noarch.c
@@ -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 } } */

Re: [RFC PATCH 2/2] RISC-V: Fix documentation of __builtin_riscv_pause

[Jeff Law]

On 8/9/23 00:11, Tsukasa OI via Gcc-patches wrote:
> From: Tsukasa OI <research_trasio@irq.a4lg.com>
> 
> This built-in does not imply the 'Xgnuzihintpausestate' extension.
> It does not change architectural state (because all HINTs are prohibited
> from doing that).
> 
> gcc/ChangeLog:
> 
> 	* doc/extend.texi: Fix the description of __builtin_riscv_pause.
I've pushed this to the trunk.
jeff

Re: [RFC PATCH 2/2] RISC-V: Fix documentation of __builtin_riscv_pause

[Hans-Peter Nilsson]

On Mon, 28 Aug 2023, Jeff Law via Gcc-patches wrote:
> 
> 
> On 8/9/23 00:11, Tsukasa OI via Gcc-patches wrote:
> > From: Tsukasa OI <research_trasio@irq.a4lg.com>
> > 
> > This built-in does not imply the 'Xgnuzihintpausestate' extension.
> > It does not change architectural state (because all HINTs are prohibited
> > from doing that).
> > 
> > gcc/ChangeLog:
> > 
> > 	* doc/extend.texi: Fix the description of __builtin_riscv_pause.
> I've pushed this to the trunk.

I randomly noticed a typo: "hart", perhaps for "part"?
Not sure though.

brgds, H-P

Re: [RFC PATCH 2/2] RISC-V: Fix documentation of __builtin_riscv_pause

[Jeff Law]

On 8/28/23 17:09, Hans-Peter Nilsson wrote:
> On Mon, 28 Aug 2023, Jeff Law via Gcc-patches wrote:
>>
>>
>> On 8/9/23 00:11, Tsukasa OI via Gcc-patches wrote:
>>> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>>>
>>> This built-in does not imply the 'Xgnuzihintpausestate' extension.
>>> It does not change architectural state (because all HINTs are prohibited
>>> from doing that).
>>>
>>> gcc/ChangeLog:
>>>
>>> 	* doc/extend.texi: Fix the description of __builtin_riscv_pause.
>> I've pushed this to the trunk.
> 
> I randomly noticed a typo: "hart", perhaps for "part"?
> Not sure though.
Not a typo.  "hart" has a well defined meaning in the risc-v world.

Thanks,
jeff

Re: [RFC PATCH 1/2] RISC-V: __builtin_riscv_pause for all environment

[Tsukasa OI]

On 2023/08/29 6:12, Jeff Law wrote:
> 
> 
> On 8/9/23 00:11, Tsukasa OI via Gcc-patches 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.c:
>>     New test when the 'Zihintpause' extension is enabled.
>>     * gcc.target/riscv/zihintpause-noarch.c:
>>     New test when the 'Zihintpause' extension is disabled.
> So I cleaned this up a bit per the list discussion and pushed the final
> result.  Hopefully I didn't goof anything too badly ;-)  The net is we
> emit "pause" or a suitable .insn based on TARGET_ZIHINTPAUSE.
> 
> Jeff

Thanks!  I had having a problem to type words through the keyboard for a
while and I appreciate doing that instead of me (your modifications were
mostly "I would do so too" ones).

Also, it seems that I will no longer need to ask you to remove leading
"[PATCH xxx]" (not just the commit title is not my intention, I worried
that you have been doing something inefficient [other than "git am"]).

Tsukasa

Re: [RFC PATCH 2/2] RISC-V: Fix documentation of __builtin_riscv_pause

[Tsukasa OI]

On 2023/08/29 8:09, Hans-Peter Nilsson wrote:
> On Mon, 28 Aug 2023, Jeff Law via Gcc-patches wrote:
>>
>>
>> On 8/9/23 00:11, Tsukasa OI via Gcc-patches wrote:
>>> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>>>
>>> This built-in does not imply the 'Xgnuzihintpausestate' extension.
>>> It does not change architectural state (because all HINTs are prohibited
>>> from doing that).
>>>
>>> gcc/ChangeLog:
>>>
>>> 	* doc/extend.texi: Fix the description of __builtin_riscv_pause.
>> I've pushed this to the trunk.
> 
> I randomly noticed a typo: "hart", perhaps for "part"?
> Not sure though.
> 
> brgds, H-P
> 

Hi H-P,

As Jeff mentioned you, the word "hart" in the RISC-V world means a
HARdware Thread and commonly used to represent a hardware-based unit of
execution.

Tsukasa

Re: [RFC PATCH 2/2] RISC-V: Fix documentation of __builtin_riscv_pause

[Hans-Peter Nilsson]

On Tue, 29 Aug 2023, Tsukasa OI wrote:

> On 2023/08/29 8:09, Hans-Peter Nilsson wrote:
> > On Mon, 28 Aug 2023, Jeff Law via Gcc-patches wrote:
> >>
> >>
> >> On 8/9/23 00:11, Tsukasa OI via Gcc-patches wrote:
> >>> From: Tsukasa OI <research_trasio@irq.a4lg.com>
> >>>
> >>> This built-in does not imply the 'Xgnuzihintpausestate' extension.
> >>> It does not change architectural state (because all HINTs are prohibited
> >>> from doing that).
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>> 	* doc/extend.texi: Fix the description of __builtin_riscv_pause.
> >> I've pushed this to the trunk.
> > 
> > I randomly noticed a typo: "hart", perhaps for "part"?
> > Not sure though.
> > 
> > brgds, H-P
> > 
> 
> Hi H-P,
> 
> As Jeff mentioned you, the word "hart" in the RISC-V world means a
> HARdware Thread and commonly used to represent a hardware-based unit of
> execution.

Thanks for the explanation.  Perhaps it's worth telling general 
readers of the document that it's a term, by simply decorating 
"hart" like @samp{hart} or adding the text "(hardware thread)"?  
It's the only grep-hit in the document at this time.

brgds, H-P