diff mbox series

RISC-V: Zihintpause: add __builtin_riscv_pause

Message ID 20210106173303.27988-1-philipp.tomsich@vrull.eu
State New
Headers show
Series RISC-V: Zihintpause: add __builtin_riscv_pause | expand

Commit Message

Philipp Tomsich Jan. 6, 2021, 5:33 p.m. UTC 
The Zihintpause extension uses an opcode from the 'fence' opcode range
to add a true hint instruction (i.e. if it is not supported on any
given platform, the 'fence' that is encoded will not enforce any
specific ordering on memory accesses) for entering a low-power state
(e.g. in an idle thread).  We expose this new instruction through a
machine-dependent builtin to allow generating it without a requirement
for any inline assembly.

Given that the encoding of 'pause' is valid (as a 'fence' encoding)
even for processors that do not (yet) support Zihintpause, we make
this builtin available without any further TARGET_* constraints.

The new builtin takes no arguments and has no return (void -> void),
which requires a change to maybe_gen_insn; similar builtins w/o
arguments and results in other architectures (e.g. rx_brk) bypass
maybe_gen_insn... making this the first time that nops == 0 is seen
here.

gcc/ChangeLog:

	* config/riscv/riscv-builtins.c: add the pause machine-dependent builtin
	with no result and no arguments; mark it as always present (pause is a
	true hint that encodes into a fence-insn, if not supported with the new
	pause semantics).
	* config/riscv/riscv-ftypes.def: Add type for void -> void.
	* config/riscv/riscv.md: Add risc_pause and UNSPECV_PAUSE
	* doc/extend.texi: Document.
	* optabs.c (maybe_gen_insn): Allow nops == 0 (void -> void).

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/builtin_pause.c: New test.

---

 gcc/config/riscv/riscv-builtins.c              |  4 +++-
 gcc/config/riscv/riscv-ftypes.def              |  1 +
 gcc/config/riscv/riscv.md                      |  8 ++++++++
 gcc/doc/extend.texi                            |  4 ++++
 gcc/optabs.c                                   |  2 ++
 gcc/testsuite/gcc.target/riscv/builtin_pause.c | 10 ++++++++++
 6 files changed, 28 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/builtin_pause.c

Comments

Kito Cheng Jan. 7, 2021, 3:35 a.m. UTC | #1 
Hi Philipp:

Could you add zihintpause to -march parser and guard that on the
pattern and builtin like zifencei[1-2]?

And could you sent a PR to
https://github.com/riscv/riscv-c-api-doc/blob/master/riscv-c-api.md to
mention __builtin_riscv_pause?

Thanks!

[1] march parser change:
https://github.com/gcc-mirror/gcc/commit/b03be74bad08c382da47e048007a78fa3fb4ef49
[2] Default version for ext.:
https://github.com/gcc-mirror/gcc/commit/4b81528241ca682025d92558ff6aeec91dafdca8


> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/builtin_pause.c
> @@ -0,0 +1,10 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" }  */
> +
> +void test_pause()

I would suggest you change the function name in the testcase,
otherwise the scan-assembler test will always pass even if you didn't
generate "pause" instruction.


> +{
> +  __builtin_riscv_pause ();
> +}
> +
> +/* { dg-final { scan-assembler "pause" } } */
> +
> --
> 2.18.4
>
Andrew Waterman Jan. 7, 2021, 3:50 a.m. UTC | #2 
I've got a contrary opinion:

Since HINTs are guaranteed to execute as no-ops--e.g., this one is
just a FENCE instruction, which is already a mandatory part of the
base ISA--they don't _need_ to be called out as separate extensions in
the toolchain.

Although there's nothing fundamentally wrong with Kito's suggestion,
it seems like an extra hoop to jump through without commensurate
benefit.  I see no reason to restrict the use of __builtin_pause,
since all RISC-V implementations, including old ones, are required to
support it.  And, of course, that's the reason we encoded it this way
:)


On Wed, Jan 6, 2021 at 7:35 PM Kito Cheng <kito.cheng@gmail.com> wrote:
>
> Hi Philipp:
>
> Could you add zihintpause to -march parser and guard that on the
> pattern and builtin like zifencei[1-2]?
>
> And could you sent a PR to
> https://github.com/riscv/riscv-c-api-doc/blob/master/riscv-c-api.md to
> mention __builtin_riscv_pause?
>
> Thanks!
>
> [1] march parser change:
> https://github.com/gcc-mirror/gcc/commit/b03be74bad08c382da47e048007a78fa3fb4ef49
> [2] Default version for ext.:
> https://github.com/gcc-mirror/gcc/commit/4b81528241ca682025d92558ff6aeec91dafdca8
>
>
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/riscv/builtin_pause.c
> > @@ -0,0 +1,10 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2" }  */
> > +
> > +void test_pause()
>
> I would suggest you change the function name in the testcase,
> otherwise the scan-assembler test will always pass even if you didn't
> generate "pause" instruction.
>
>
> > +{
> > +  __builtin_riscv_pause ();
> > +}
> > +
> > +/* { dg-final { scan-assembler "pause" } } */
> > +
> > --
> > 2.18.4
> >
Kito Cheng Jan. 7, 2021, 5:41 a.m. UTC | #3 
Hi Andrew:

It's safe to execute on old machine, but it is still a new extension not
included on baseline ISA, so I still prefer having -march to guard that,
and then we can track that in the ELF attribute to see what extensions and
which version are used in the executable / object files.


On Thu, Jan 7, 2021 at 11:51 AM Andrew Waterman <aswaterman@gmail.com>
wrote:

> I've got a contrary opinion:
>
> Since HINTs are guaranteed to execute as no-ops--e.g., this one is
> just a FENCE instruction, which is already a mandatory part of the
> base ISA--they don't _need_ to be called out as separate extensions in
> the toolchain.
>
> Although there's nothing fundamentally wrong with Kito's suggestion,
> it seems like an extra hoop to jump through without commensurate
> benefit.  I see no reason to restrict the use of __builtin_pause,
> since all RISC-V implementations, including old ones, are required to
> support it.  And, of course, that's the reason we encoded it this way
> :)
>
>
> On Wed, Jan 6, 2021 at 7:35 PM Kito Cheng <kito.cheng@gmail.com> wrote:
> >
> > Hi Philipp:
> >
> > Could you add zihintpause to -march parser and guard that on the
> > pattern and builtin like zifencei[1-2]?
> >
> > And could you sent a PR to
> > https://github.com/riscv/riscv-c-api-doc/blob/master/riscv-c-api.md to
> > mention __builtin_riscv_pause?
> >
> > Thanks!
> >
> > [1] march parser change:
> >
> https://github.com/gcc-mirror/gcc/commit/b03be74bad08c382da47e048007a78fa3fb4ef49
> > [2] Default version for ext.:
> >
> https://github.com/gcc-mirror/gcc/commit/4b81528241ca682025d92558ff6aeec91dafdca8
> >
> >
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/riscv/builtin_pause.c
> > > @@ -0,0 +1,10 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-O2" }  */
> > > +
> > > +void test_pause()
> >
> > I would suggest you change the function name in the testcase,
> > otherwise the scan-assembler test will always pass even if you didn't
> > generate "pause" instruction.
> >
> >
> > > +{
> > > +  __builtin_riscv_pause ();
> > > +}
> > > +
> > > +/* { dg-final { scan-assembler "pause" } } */
> > > +
> > > --
> > > 2.18.4
> > >
>
Philipp Tomsich Jan. 7, 2021, 6:53 a.m. UTC | #4 
Kito:

We had originally considered to guard this with a -march, but decided
against it
eventually: this instruction will be (among other cases) used in the
cpu_relax() of
the Linux kernel.  For cases like that, we should consider this the
baseline (i.e.
either there's no pause—in which case, the encoded fence will not hurt—or
the
Zihintpause extension)... but it all maps back to a single builtin-call.

Note that the Zihintfence will be enabled for all (also older) targets, as
the insn
is supported there as well (as a fence that doesn't do anything)... so
guarding it
will not really change the behavior.

That said, I'll get going on an v2 that will include the -march guard (and
we can
still turn things back to how they are today).

Thanks,
Philipp.

On Thu, 7 Jan 2021 at 06:42, Kito Cheng <kito.cheng@sifive.com> wrote:

> Hi Andrew:
>
> It's safe to execute on old machine, but it is still a new extension not
> included on baseline ISA, so I still prefer having -march to guard that,
> and then we can track that in the ELF attribute to see what extensions and
> which version are used in the executable / object files.
>
>
> On Thu, Jan 7, 2021 at 11:51 AM Andrew Waterman <aswaterman@gmail.com>
> wrote:
>
>> I've got a contrary opinion:
>>
>> Since HINTs are guaranteed to execute as no-ops--e.g., this one is
>> just a FENCE instruction, which is already a mandatory part of the
>> base ISA--they don't _need_ to be called out as separate extensions in
>> the toolchain.
>>
>> Although there's nothing fundamentally wrong with Kito's suggestion,
>> it seems like an extra hoop to jump through without commensurate
>> benefit.  I see no reason to restrict the use of __builtin_pause,
>> since all RISC-V implementations, including old ones, are required to
>> support it.  And, of course, that's the reason we encoded it this way
>> :)
>>
>>
>> On Wed, Jan 6, 2021 at 7:35 PM Kito Cheng <kito.cheng@gmail.com> wrote:
>> >
>> > Hi Philipp:
>> >
>> > Could you add zihintpause to -march parser and guard that on the
>> > pattern and builtin like zifencei[1-2]?
>> >
>> > And could you sent a PR to
>> > https://github.com/riscv/riscv-c-api-doc/blob/master/riscv-c-api.md to
>> > mention __builtin_riscv_pause?
>> >
>> > Thanks!
>> >
>> > [1] march parser change:
>> >
>> https://github.com/gcc-mirror/gcc/commit/b03be74bad08c382da47e048007a78fa3fb4ef49
>> > [2] Default version for ext.:
>> >
>> https://github.com/gcc-mirror/gcc/commit/4b81528241ca682025d92558ff6aeec91dafdca8
>> >
>> >
>> > > --- /dev/null
>> > > +++ b/gcc/testsuite/gcc.target/riscv/builtin_pause.c
>> > > @@ -0,0 +1,10 @@
>> > > +/* { dg-do compile } */
>> > > +/* { dg-options "-O2" }  */
>> > > +
>> > > +void test_pause()
>> >
>> > I would suggest you change the function name in the testcase,
>> > otherwise the scan-assembler test will always pass even if you didn't
>> > generate "pause" instruction.
>> >
>> >
>> > > +{
>> > > +  __builtin_riscv_pause ();
>> > > +}
>> > > +
>> > > +/* { dg-final { scan-assembler "pause" } } */
>> > > +
>> > > --
>> > > 2.18.4
>> > >
>>
>
Kito Cheng Jan. 7, 2021, 8:49 a.m. UTC | #5 
My point is tracking info and consistent behavior/scheme with other
extensions, so personally I strongly prefer it should be guarded with
-march.

But maybe we could create an issue on riscv-c-api-doc[1] or
riscv-toolchain-conventions[2] to
get feedback from LLVM folks, since I think this behavior should align
between LLVM and GCC.

[1] https://github.com/riscv/riscv-c-api-doc
[2] https://github.com/riscv/riscv-toolchain-conventions

On Thu, Jan 7, 2021 at 2:53 PM Philipp Tomsich <philipp.tomsich@vrull.eu> wrote:
>
> Kito:
>
> We had originally considered to guard this with a -march, but decided against it
> eventually: this instruction will be (among other cases) used in the cpu_relax() of
> the Linux kernel.  For cases like that, we should consider this the baseline (i.e.
> either there's no pause—in which case, the encoded fence will not hurt—or the
> Zihintpause extension)... but it all maps back to a single builtin-call.
>
> Note that the Zihintfence will be enabled for all (also older) targets, as the insn
> is supported there as well (as a fence that doesn't do anything)... so guarding it
> will not really change the behavior.
>
> That said, I'll get going on an v2 that will include the -march guard (and we can
> still turn things back to how they are today).
>
> Thanks,
> Philipp.
>
> On Thu, 7 Jan 2021 at 06:42, Kito Cheng <kito.cheng@sifive.com> wrote:
>>
>> Hi Andrew:
>>
>> It's safe to execute on old machine, but it is still a new extension not included on baseline ISA, so I still prefer having -march to guard that, and then we can track that in the ELF attribute to see what extensions and which version are used in the executable / object files.
>>
>>
>> On Thu, Jan 7, 2021 at 11:51 AM Andrew Waterman <aswaterman@gmail.com> wrote:
>>>
>>> I've got a contrary opinion:
>>>
>>> Since HINTs are guaranteed to execute as no-ops--e.g., this one is
>>> just a FENCE instruction, which is already a mandatory part of the
>>> base ISA--they don't _need_ to be called out as separate extensions in
>>> the toolchain.
>>>
>>> Although there's nothing fundamentally wrong with Kito's suggestion,
>>> it seems like an extra hoop to jump through without commensurate
>>> benefit.  I see no reason to restrict the use of __builtin_pause,
>>> since all RISC-V implementations, including old ones, are required to
>>> support it.  And, of course, that's the reason we encoded it this way
>>> :)
>>>
>>>
>>> On Wed, Jan 6, 2021 at 7:35 PM Kito Cheng <kito.cheng@gmail.com> wrote:
>>> >
>>> > Hi Philipp:
>>> >
>>> > Could you add zihintpause to -march parser and guard that on the
>>> > pattern and builtin like zifencei[1-2]?
>>> >
>>> > And could you sent a PR to
>>> > https://github.com/riscv/riscv-c-api-doc/blob/master/riscv-c-api.md to
>>> > mention __builtin_riscv_pause?
>>> >
>>> > Thanks!
>>> >
>>> > [1] march parser change:
>>> > https://github.com/gcc-mirror/gcc/commit/b03be74bad08c382da47e048007a78fa3fb4ef49
>>> > [2] Default version for ext.:
>>> > https://github.com/gcc-mirror/gcc/commit/4b81528241ca682025d92558ff6aeec91dafdca8
>>> >
>>> >
>>> > > --- /dev/null
>>> > > +++ b/gcc/testsuite/gcc.target/riscv/builtin_pause.c
>>> > > @@ -0,0 +1,10 @@
>>> > > +/* { dg-do compile } */
>>> > > +/* { dg-options "-O2" }  */
>>> > > +
>>> > > +void test_pause()
>>> >
>>> > I would suggest you change the function name in the testcase,
>>> > otherwise the scan-assembler test will always pass even if you didn't
>>> > generate "pause" instruction.
>>> >
>>> >
>>> > > +{
>>> > > +  __builtin_riscv_pause ();
>>> > > +}
>>> > > +
>>> > > +/* { dg-final { scan-assembler "pause" } } */
>>> > > +
>>> > > --
>>> > > 2.18.4
>>> > >
Jim Wilson Feb. 18, 2021, 8:21 p.m. UTC | #6 
On Thu, Jan 7, 2021 at 12:50 AM Kito Cheng <kito.cheng@gmail.com> wrote:

> My point is tracking info and consistent behavior/scheme with other
> extensions, so personally I strongly prefer it should be guarded with
> -march.
>

It is a hint.  We should allow it even if the architecture extension is not
enabled.

For comparison, I suggest you look at the aarch64 port.  They have 3 kinds
of hints: branch protection (bti), pointer authentication, and speculation
control.  They deliberately allow you to emit the instructions even if the
hardware doesn't support the feature because they are hints, and execute as
nops if the hardware support is missing.  The rationale is that the code
will work with or without the hardware support, but will work better with
the hardware support, so it is best to always allow it.  We should do the
same with RISC-V hints.

I agree that we need to include LLVM folks in the discussion to make sure
that GCC and LLVM handle this the same way.

Jim
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv-builtins.c b/gcc/config/riscv/riscv-builtins.c
index bc959389c76..18b9dc579a1 100644
--- a/gcc/config/riscv/riscv-builtins.c
+++ b/gcc/config/riscv/riscv-builtins.c
@@ -86,6 +86,7 @@  struct riscv_builtin_description {
 };
 
 AVAIL (hard_float, TARGET_HARD_FLOAT)
+AVAIL (always,     (!0))
 
 /* Construct a riscv_builtin_description from the given arguments.
 
@@ -129,7 +130,8 @@  AVAIL (hard_float, TARGET_HARD_FLOAT)
 
 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 (fsflags, RISCV_VOID_FTYPE_USI, hard_float),
+  DIRECT_NO_TARGET_BUILTIN (pause, RISCV_VOID_FTYPE, always),
 };
 
 /* Index I is the function declaration for riscv_builtins[I], or null if the
diff --git a/gcc/config/riscv/riscv-ftypes.def b/gcc/config/riscv/riscv-ftypes.def
index 1c6bc4e9dce..fcb042222db 100644
--- a/gcc/config/riscv/riscv-ftypes.def
+++ b/gcc/config/riscv/riscv-ftypes.def
@@ -27,4 +27,5 @@  along with GCC; see the file COPYING3.  If not see
         argument type.  */
 
 DEF_RISCV_FTYPE (0, (USI))
+DEF_RISCV_FTYPE (0, (VOID))
 DEF_RISCV_FTYPE (1, (VOID, USI))
diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index 254147c112a..b8fb2b8c279 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -69,6 +69,9 @@  (define_c_enum "unspecv" [
   ;; Stack Smash Protector
   UNSPEC_SSP_SET
   UNSPEC_SSP_TEST
+
+  ;; Zihintpause unspec
+  UNSPECV_PAUSE
 ])
 
 (define_constants
@@ -1559,6 +1562,11 @@  (define_insn "fence_i"
   "TARGET_ZIFENCEI"
   "fence.i")
 
+(define_insn "riscv_pause"
+  [(unspec_volatile [(const_int 0)] UNSPECV_PAUSE)]
+  ""
+  "pause")
+
 ;;
 ;;  ....................
 ;;
diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
index e73464a7f19..4cd19c2ebbb 100644
--- a/gcc/doc/extend.texi
+++ b/gcc/doc/extend.texi
@@ -22056,6 +22056,10 @@  processors.
 Returns the value that is currently set in the @samp{tp} register.
 @end deftypefn
 
+@deftypefn {Built-in Function}  void __builtin_riscv_pause (void)
+Generates the @code{pause} (hint) machine instruction.
+@end deftypefn
+
 @node RX Built-in Functions
 @subsection RX Built-in Functions
 GCC supports some of the RX instructions which cannot be expressed in
diff --git a/gcc/optabs.c b/gcc/optabs.c
index 0427063e277..f7a1bf5be1c 100644
--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -7777,6 +7777,8 @@  maybe_gen_insn (enum insn_code icode, unsigned int nops,
 
   switch (nops)
     {
+    case 0:
+      return GEN_FCN (icode) ();
     case 1:
       return GEN_FCN (icode) (ops[0].value);
     case 2:
diff --git a/gcc/testsuite/gcc.target/riscv/builtin_pause.c b/gcc/testsuite/gcc.target/riscv/builtin_pause.c
new file mode 100644
index 00000000000..9250937cabb
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/builtin_pause.c
@@ -0,0 +1,10 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2" }  */
+
+void test_pause()
+{
+  __builtin_riscv_pause ();
+}
+
+/* { dg-final { scan-assembler "pause" } } */
+