Re: [PATCH] RISC-V: Note that __builtin_riscv_pause() implies Xgnuzihintpausestate

[Andrew Waterman <andrew@sifive.com>]

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

It took me a few minutes to understand the purpose of this chicanery, but
there's indeed a contradiction in the ISA spec.  HINT instructions _do_
affect architectural state in a limited fashion--namely, updating the PC.
So, it's incorrect to say that PAUSE changes no architectural state.
Because these statements are contradictory, a common-sense reading should
parse this as "PAUSE changes no architectural state in the same informal
sense as other HINTs".  Otherwise, PAUSE wouldn't actually be a HINT.

I'm just going to delete the erroneous text.  This eliminates the
contradiction and makes the spec consistent with both the de facto and de
jure golden models, which behave in the common-sense manner Palmer's
Xgnuzihintpausestate extension would suggest.

To avoid confusion, I strongly suggest deleting all references
to Xgnuzihintpausestate, since its existence invites a question that no
longer needs to be answered.

cc'ing Greg since AFAICS he merged in the erroneous text.

On Fri, Dec 16, 2022 at 8:48 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:

> On Mon, 28 Nov 2022 10:45:51 PST (-0800), Palmer Dabbelt wrote:
> > On Fri, 18 Nov 2022 09:01:08 PST (-0800), Palmer Dabbelt wrote:
> >> On Thu, 17 Nov 2022 22:59:08 PST (-0800), Kito Cheng wrote:
> >>> Wait, what's Xgnuzihintpausestate???
> >>
> >> I just made it up, it's defined right next to the name like those
> >> profile extensions are.  I figured that's the most RISC-V way to define
> >> something like this, but we could just drop it and run with the
> >> definition -- IIRC we just stuck a comment in for Linux and QEMU, I
> >> doubt anyone is actually going to implement the "doesn't touch PC"
> >> version of pause.
> >
> > Just checking up on this one.  I don't care a ton about the name, just
> > that we document where we're intentionally violating the specs.
>
> I'm just committing this one, no big deal if you want to change the
> wording.  I just want it out of my queue.
>
> >
> >>
> >>> On Fri, Nov 18, 2022 at 12:30 PM Palmer Dabbelt <palmer@rivosinc.com>
> wrote:
> >>>>
> >>>> gcc/ChangeLog:
> >>>>
> >>>>         * doc/extend.texi (__builtin_riscv_pause): Imply
> >>>>         Xgnuzihintpausestate.
> >>>> ---
> >>>>  gcc/doc/extend.texi | 4 +++-
> >>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/gcc/doc/extend.texi b/gcc/doc/extend.texi
> >>>> index b1dd39e64b8..26f14e61bc8 100644
> >>>> --- a/gcc/doc/extend.texi
> >>>> +++ b/gcc/doc/extend.texi
> >>>> @@ -21103,7 +21103,9 @@ 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.
> >>>> +Generates the @code{pause} (hint) machine instruction.  This implies
> the
> >>>> +Xgnuzihintpausestate extension, which redefines the @code{pause}
> instruction to
> >>>> +change architectural state.
> >>>>  @end deftypefn
> >>>>
> >>>>  @node RX Built-in Functions
> >>>> --
> >>>> 2.38.1
> >>>>
>