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

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

[Palmer Dabbelt]

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

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

[Kito Cheng]

Wait, what's Xgnuzihintpausestate???


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
>

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

[Palmer Dabbelt]

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.

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

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

[Palmer Dabbelt]

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.

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

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

[Palmer Dabbelt]

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

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

[Andrew Waterman]

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

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

[Andreas Schwab]

On Dez 17 2022, Andrew Waterman wrote:

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

How can an insn _not_ affect the PC? (Other than the trivial infinite
loop.)

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

[Andrew Waterman]

On Sat, Dec 17, 2022 at 2:10 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Dez 17 2022, Andrew Waterman wrote:
>
> > 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.
>
> How can an insn _not_ affect the PC? (Other than the trivial infinite
> loop.)

Heh, yeah, that's roughly what I meant by "common-sense reading" (and
that's my rationale for simply clarifying the spec and nuking this
Xgnuzihintpausestate extension).

>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."

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

[Andreas Schwab]

On Dez 17 2022, Andrew Waterman wrote:

> On Sat, Dec 17, 2022 at 2:10 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
>>
>> On Dez 17 2022, Andrew Waterman wrote:
>>
>> > 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.
>>
>> How can an insn _not_ affect the PC? (Other than the trivial infinite
>> loop.)
>
> Heh, yeah, that's roughly what I meant by "common-sense reading" (and
> that's my rationale for simply clarifying the spec and nuking this
> Xgnuzihintpausestate extension).

My point is that the implicit update of the PC cannot be part of the
architectural state in the first place.  Even the trivial infinite loop
has this, before the actual state change (setting PC back) is performed.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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

[Andrew Waterman]

On Sat, Dec 17, 2022 at 2:21 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Dez 17 2022, Andrew Waterman wrote:
>
> > On Sat, Dec 17, 2022 at 2:10 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
> >>
> >> On Dez 17 2022, Andrew Waterman wrote:
> >>
> >> > 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.
> >>
> >> How can an insn _not_ affect the PC? (Other than the trivial infinite
> >> loop.)
> >
> > Heh, yeah, that's roughly what I meant by "common-sense reading" (and
> > that's my rationale for simply clarifying the spec and nuking this
> > Xgnuzihintpausestate extension).
>
> My point is that the implicit update of the PC cannot be part of the
> architectural state in the first place.  Even the trivial infinite loop
> has this, before the actual state change (setting PC back) is performed.

It's just a definitional issue.  By analogy, this is why we have the
concept of "explicit memory access" (the thing a load or store is
trying to do) and "implicit memory access" (all of the other memory
accesses, like the instruction fetch or page-table walk).  The PC
update is very much an architectural-state change, but it would be
fair to call it an "implicit architectural-state change" to contrast
with e.g. writing an x-register being an "explicit architectural state
change".

Anyway, I don't think we're disagreeing with each other (and I still
think there's no problem to be solved here).

>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."