Re: [PATCH] RISC-V: Zihintpause: add __builtin_riscv_pause

[Philipp Tomsich <philipp.tomsich@vrull.eu>]

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
>> > >
>>
>