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

[Kito Cheng <kito.cheng@gmail.com>]

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