[PATCH] RISC-V: Add the Zihpm and Zicntr extensions

[PATCH] RISC-V: Add the Zihpm and Zicntr extensions

[Palmer Dabbelt]

These extensions were recently frozen [1].  As per Andrew's post [2]
we're meant to ignore these in software, this just adds them to the list
of allowed extensions and otherwise ignores them.  I added these under
SPEC_CLASS_NONE even though the PDF lists them as 20190614 because it
seems pointless to add another spec class just to accept two extensions
we then ignore.

1: https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/HZGoqP1eyps/m/GTNKRLJoAQAJ
2: https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/QKjQhChrq9Q/m/7gqdkctgAgAJ

gcc/ChangeLog

	* common/config/riscv/riscv-common.cc: Add Zihpm and Zicnttr
	extensions.

---

These deserves documentation, a test case, and a NEWS entry.  I didn't
write those yet because it's not super clear this is the way we wanted
to go, though: just flat out ignoring the ISA feels like the wrong thing
to do, but the guidance here is pretty clear.  Still feels odd, though.

We've also still got an open discussion on how we want to handle -march
going forwards that's pretty relevant here, so I figured it'd be best to
send this out sooner rather than later as it's sort of related.
---
 gcc/common/config/riscv/riscv-common.cc | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gcc/common/config/riscv/riscv-common.cc b/gcc/common/config/riscv/riscv-common.cc
index 4b7f777c103..72981f05ac7 100644
--- a/gcc/common/config/riscv/riscv-common.cc
+++ b/gcc/common/config/riscv/riscv-common.cc
@@ -190,6 +190,9 @@ static const struct riscv_ext_version riscv_ext_version_table[] =
   {"zicbom",ISA_SPEC_CLASS_NONE, 1, 0},
   {"zicbop",ISA_SPEC_CLASS_NONE, 1, 0},
 
+  {"zicntr", ISA_SPEC_CLASS_NONE, 2, 0},
+  {"zihpm",  ISA_SPEC_CLASS_NONE, 2, 0},
+
   {"zk",    ISA_SPEC_CLASS_NONE, 1, 0},
   {"zkn",   ISA_SPEC_CLASS_NONE, 1, 0},
   {"zks",   ISA_SPEC_CLASS_NONE, 1, 0},
-- 
2.38.1

Re: [PATCH] RISC-V: Add the Zihpm and Zicntr extensions

[Christoph Müllner]

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

On Wed, Nov 9, 2022 at 4:01 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:

> These extensions were recently frozen [1].  As per Andrew's post [2]
> we're meant to ignore these in software, this just adds them to the list
> of allowed extensions and otherwise ignores them.  I added these under
> SPEC_CLASS_NONE even though the PDF lists them as 20190614 because it
> seems pointless to add another spec class just to accept two extensions
> we then ignore.
>
> 1:
> https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/HZGoqP1eyps/m/GTNKRLJoAQAJ
> 2:
> https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/QKjQhChrq9Q/m/7gqdkctgAgAJ
>
> gcc/ChangeLog
>
>         * common/config/riscv/riscv-common.cc: Add Zihpm and Zicnttr
>         extensions.
>
> ---
>
> These deserves documentation, a test case, and a NEWS entry.  I didn't
> write those yet because it's not super clear this is the way we wanted
> to go, though: just flat out ignoring the ISA feels like the wrong thing
> to do, but the guidance here is pretty clear.  Still feels odd, though.
>


We already have the infrastructure in GAS to check the CSR numbers.
It is an optional feature, but it is here and working.
We follow the guidance in the default configuration (CSR checking needs to
be turned on).
As long as we want to keep this infrastructure, there is no question if we
should continue
to support new extensions as required by this feature:
We have to because everything else will lead to a broken feature.

The question if CSR checking in GAS should be removed or not does not have
to be
answered right now if there is doubt about making the wrong decision.

Additionally, I fully agree that we can not ignore unknown extensions.
We must report an unknown extension in the march string to the user.
And even without CSR checking, GCC needs to be aware of all extensions
(e.g. for possible future support of -march=native).

So I think this patch should go in (together with a test).

That's why I also sent something similar for Smaia and Ssaia:
https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606640.html

BR
Christoph





> We've also still got an open discussion on how we want to handle -march
> going forwards that's pretty relevant here, so I figured it'd be best to
> send this out sooner rather than later as it's sort of related.
> ---
>  gcc/common/config/riscv/riscv-common.cc | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/gcc/common/config/riscv/riscv-common.cc
> b/gcc/common/config/riscv/riscv-common.cc
> index 4b7f777c103..72981f05ac7 100644
> --- a/gcc/common/config/riscv/riscv-common.cc
> +++ b/gcc/common/config/riscv/riscv-common.cc
> @@ -190,6 +190,9 @@ static const struct riscv_ext_version
> riscv_ext_version_table[] =
>    {"zicbom",ISA_SPEC_CLASS_NONE, 1, 0},
>    {"zicbop",ISA_SPEC_CLASS_NONE, 1, 0},
>
> +  {"zicntr", ISA_SPEC_CLASS_NONE, 2, 0},
> +  {"zihpm",  ISA_SPEC_CLASS_NONE, 2, 0},
> +
>    {"zk",    ISA_SPEC_CLASS_NONE, 1, 0},
>    {"zkn",   ISA_SPEC_CLASS_NONE, 1, 0},
>    {"zks",   ISA_SPEC_CLASS_NONE, 1, 0},
> --
> 2.38.1
>
>

Re: [PATCH] RISC-V: Add the Zihpm and Zicntr extensions

[Palmer Dabbelt]

On Thu, 17 Nov 2022 18:14:18 PST (-0800), christoph.muellner@vrull.eu wrote:
> On Wed, Nov 9, 2022 at 4:01 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
>> These extensions were recently frozen [1].  As per Andrew's post [2]
>> we're meant to ignore these in software, this just adds them to the list
>> of allowed extensions and otherwise ignores them.  I added these under
>> SPEC_CLASS_NONE even though the PDF lists them as 20190614 because it
>> seems pointless to add another spec class just to accept two extensions
>> we then ignore.
>>
>> 1:
>> https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/HZGoqP1eyps/m/GTNKRLJoAQAJ
>> 2:
>> https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/QKjQhChrq9Q/m/7gqdkctgAgAJ
>>
>> gcc/ChangeLog
>>
>>         * common/config/riscv/riscv-common.cc: Add Zihpm and Zicnttr
>>         extensions.
>>
>> ---
>>
>> These deserves documentation, a test case, and a NEWS entry.  I didn't
>> write those yet because it's not super clear this is the way we wanted
>> to go, though: just flat out ignoring the ISA feels like the wrong thing
>> to do, but the guidance here is pretty clear.  Still feels odd, though.
>>
>
>
> We already have the infrastructure in GAS to check the CSR numbers.
> It is an optional feature, but it is here and working.
> We follow the guidance in the default configuration (CSR checking needs to
> be turned on).
> As long as we want to keep this infrastructure, there is no question if we
> should continue
> to support new extensions as required by this feature:
> We have to because everything else will lead to a broken feature.
>
> The question if CSR checking in GAS should be removed or not does not have
> to be
> answered right now if there is doubt about making the wrong decision.
>
> Additionally, I fully agree that we can not ignore unknown extensions.
> We must report an unknown extension in the march string to the user.
> And even without CSR checking, GCC needs to be aware of all extensions
> (e.g. for possible future support of -march=native).
>
> So I think this patch should go in (together with a test).
>
> That's why I also sent something similar for Smaia and Ssaia:
> https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606640.html

That's a different problem: with Zihpm and Zicntr we're ignoring known 
extensions, so we can pretend the ISA didn't make a backwards 
incompatible change.  That requires explicitly ignoring words in the ISA 
manual, which is something we've tried very hard to do in the past -- 
maybe less so these days, but IMO it's still worth calling out (see the 
__builtin_riscv_pause() doc patch, for example).

> BR
> Christoph
>
>
>
>
>
>> We've also still got an open discussion on how we want to handle -march
>> going forwards that's pretty relevant here, so I figured it'd be best to
>> send this out sooner rather than later as it's sort of related.
>> ---
>>  gcc/common/config/riscv/riscv-common.cc | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/gcc/common/config/riscv/riscv-common.cc
>> b/gcc/common/config/riscv/riscv-common.cc
>> index 4b7f777c103..72981f05ac7 100644
>> --- a/gcc/common/config/riscv/riscv-common.cc
>> +++ b/gcc/common/config/riscv/riscv-common.cc
>> @@ -190,6 +190,9 @@ static const struct riscv_ext_version
>> riscv_ext_version_table[] =
>>    {"zicbom",ISA_SPEC_CLASS_NONE, 1, 0},
>>    {"zicbop",ISA_SPEC_CLASS_NONE, 1, 0},
>>
>> +  {"zicntr", ISA_SPEC_CLASS_NONE, 2, 0},
>> +  {"zihpm",  ISA_SPEC_CLASS_NONE, 2, 0},
>> +
>>    {"zk",    ISA_SPEC_CLASS_NONE, 1, 0},
>>    {"zkn",   ISA_SPEC_CLASS_NONE, 1, 0},
>>    {"zks",   ISA_SPEC_CLASS_NONE, 1, 0},
>> --
>> 2.38.1
>>
>>

Re: [PATCH] RISC-V: Add the Zihpm and Zicntr extensions

[Jeff Law]

On 11/8/22 20:00, Palmer Dabbelt wrote:
> These extensions were recently frozen [1].  As per Andrew's post [2]
> we're meant to ignore these in software, this just adds them to the list
> of allowed extensions and otherwise ignores them.  I added these under
> SPEC_CLASS_NONE even though the PDF lists them as 20190614 because it
> seems pointless to add another spec class just to accept two extensions
> we then ignore.
>
> 1: https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/HZGoqP1eyps/m/GTNKRLJoAQAJ
> 2: https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/QKjQhChrq9Q/m/7gqdkctgAgAJ
>
> gcc/ChangeLog
>
> 	* common/config/riscv/riscv-common.cc: Add Zihpm and Zicnttr
> 	extensions.

So the idea here is just to define the extension so that it gets defined 
in the ISA strings and passed through to the assembler, right?

Jeff

Re: [PATCH] RISC-V: Add the Zihpm and Zicntr extensions

[Kito Cheng]

> So the idea here is just to define the extension so that it gets defined
> in the ISA strings and passed through to the assembler, right?

That will also define arch test marco:

https://github.com/riscv-non-isa/riscv-c-api-doc/blob/master/riscv-c-api.md#architecture-extension-test-macro

On Mon, Nov 21, 2022 at 12:20 AM Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
> On 11/8/22 20:00, Palmer Dabbelt wrote:
> > These extensions were recently frozen [1].  As per Andrew's post [2]
> > we're meant to ignore these in software, this just adds them to the list
> > of allowed extensions and otherwise ignores them.  I added these under
> > SPEC_CLASS_NONE even though the PDF lists them as 20190614 because it
> > seems pointless to add another spec class just to accept two extensions
> > we then ignore.
> >
> > 1: https://groups.google.com/a/groups.riscv.org/g/isa-dev/c/HZGoqP1eyps/m/GTNKRLJoAQAJ
> > 2: https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/QKjQhChrq9Q/m/7gqdkctgAgAJ
> >
> > gcc/ChangeLog
> >
> >       * common/config/riscv/riscv-common.cc: Add Zihpm and Zicnttr
> >       extensions.
>
> So the idea here is just to define the extension so that it gets defined
> in the ISA strings and passed through to the assembler, right?
>
> Jeff

Re: [PATCH] RISC-V: Add the Zihpm and Zicntr extensions

[Jeff Law]

On 11/20/22 18:36, Kito Cheng wrote:
>> So the idea here is just to define the extension so that it gets defined
>> in the ISA strings and passed through to the assembler, right?
> That will also define arch test marco:
>
> https://github.com/riscv-non-isa/riscv-c-api-doc/blob/master/riscv-c-api.md#architecture-extension-test-macro

Sorry I should have been clearer and included the test macro(s) as well.

So a better summary would be that while it doesn't change the codegen 
behavior in the compiler, it does provide the mechanisms to pass along 
isa strings to other tools such as the assembler and signal via the test 
macros that this extension is available.


If so I think that it meets Andrew's requirements and at least some of 
those issues raised by Jim.   But I'm not sure it can address your 
concern WRT consistency.  In fact, I don't really see a way to address 
that concern with option #2 which Andrew seems to think is the only 
reasonable path forward from an RVI standpoint.


I'm at a loss for next steps, particularly as the newbie in this world.


jeff

Re: [PATCH] RISC-V: Add the Zihpm and Zicntr extensions

[Palmer Dabbelt]

On Tue, 22 Nov 2022 07:20:15 PST (-0800), jeffreyalaw@gmail.com wrote:
>
> On 11/20/22 18:36, Kito Cheng wrote:
>>> So the idea here is just to define the extension so that it gets defined
>>> in the ISA strings and passed through to the assembler, right?
>> That will also define arch test marco:
>>
>> https://github.com/riscv-non-isa/riscv-c-api-doc/blob/master/riscv-c-api.md#architecture-extension-test-macro
>
> Sorry I should have been clearer and included the test macro(s) as well.
>
> So a better summary would be that while it doesn't change the codegen
> behavior in the compiler, it does provide the mechanisms to pass along
> isa strings to other tools such as the assembler and signal via the test
> macros that this extension is available.

IMO the important bit here is that we're not adding any compatibility 
flags, like we did when fence.i was removed from the ISA.  That's fine 
as long as we never remove these instructions from the base ISA in the 
software, but that's what's suggested by Andrew in the post.

> If so I think that it meets Andrew's requirements and at least some of
> those issues raised by Jim.   But I'm not sure it can address your
> concern WRT consistency.  In fact, I don't really see a way to address
> that concern with option #2 which Andrew seems to think is the only
> reasonable path forward from an RVI standpoint.
>
>
> I'm at a loss for next steps, particularly as the newbie in this world.

It's a super weird one, but there's a bunch of cases in RISC-V where 
we're told to just ignore words in the ISA manual.  Definitely a trap 
for users (and we already had some Linux folks get bit by the counter 
changes here), but that's just how RISC-V works.

Re: [PATCH] RISC-V: Add the Zihpm and Zicntr extensions

[Jeff Law]

On 11/22/22 08:29, Palmer Dabbelt wrote:
> On Tue, 22 Nov 2022 07:20:15 PST (-0800), jeffreyalaw@gmail.com wrote:
>>
>> On 11/20/22 18:36, Kito Cheng wrote:
>>>> So the idea here is just to define the extension so that it gets 
>>>> defined
>>>> in the ISA strings and passed through to the assembler, right?
>>> That will also define arch test marco:
>>>
>>> https://github.com/riscv-non-isa/riscv-c-api-doc/blob/master/riscv-c-api.md#architecture-extension-test-macro 
>>>
>>
>> Sorry I should have been clearer and included the test macro(s) as well.
>>
>> So a better summary would be that while it doesn't change the codegen
>> behavior in the compiler, it does provide the mechanisms to pass along
>> isa strings to other tools such as the assembler and signal via the test
>> macros that this extension is available.
>
> IMO the important bit here is that we're not adding any compatibility 
> flags, like we did when fence.i was removed from the ISA.  That's fine 
> as long as we never remove these instructions from the base ISA in the 
> software, but that's what's suggested by Andrew in the post.

Right.  IIUC these instructions were never supposed to be in the base 
ISA, but, in effect, snuck through.  We're retro-actively adding them as 
an extension, at least in terms of ISA strings & test macros.  We're 
currently (forever?) going to allow them in the assembler without 
strictly requiring the extension be on.


> It's a super weird one, but there's a bunch of cases in RISC-V where 
> we're told to just ignore words in the ISA manual.  Definitely a trap 
> for users (and we already had some Linux folks get bit by the counter 
> changes here), but that's just how RISC-V works.

Mistakes happen.  The key is to adjust for them as best as we can.    
I'd lean towards a stricter enforcement, bringing these 
instructions/extension in line with how we handle the others. It'd 
potentially mean source incompatibilities that would need to be fixed, 
but they shouldn't be difficult and we're still early enough in the game 
that we *could* take that route.  Andrew's position is more 
accommodating of existing code and while I may not 100% agree with his 
position, I understand it.


So while I'd lean towards a stricter checking, I can live with this 
approach.  I wouldn't mind hearing from Kito, Philipp and others though.


Jeff

Re: [PATCH] RISC-V: Add the Zihpm and Zicntr extensions

[Palmer Dabbelt]

On Tue, 22 Nov 2022 13:50:28 PST (-0800), jeffreyalaw@gmail.com wrote:
>
> On 11/22/22 08:29, Palmer Dabbelt wrote:
>> On Tue, 22 Nov 2022 07:20:15 PST (-0800), jeffreyalaw@gmail.com wrote:
>>>
>>> On 11/20/22 18:36, Kito Cheng wrote:
>>>>> So the idea here is just to define the extension so that it gets
>>>>> defined
>>>>> in the ISA strings and passed through to the assembler, right?
>>>> That will also define arch test marco:
>>>>
>>>> https://github.com/riscv-non-isa/riscv-c-api-doc/blob/master/riscv-c-api.md#architecture-extension-test-macro
>>>>
>>>
>>> Sorry I should have been clearer and included the test macro(s) as well.
>>>
>>> So a better summary would be that while it doesn't change the codegen
>>> behavior in the compiler, it does provide the mechanisms to pass along
>>> isa strings to other tools such as the assembler and signal via the test
>>> macros that this extension is available.
>>
>> IMO the important bit here is that we're not adding any compatibility
>> flags, like we did when fence.i was removed from the ISA.  That's fine
>> as long as we never remove these instructions from the base ISA in the
>> software, but that's what's suggested by Andrew in the post.
>
> Right.  IIUC these instructions were never supposed to be in the base
> ISA, but, in effect, snuck through.  We're retro-actively adding them as
> an extension, at least in terms of ISA strings & test macros.  We're
> currently (forever?) going to allow them in the assembler without
> strictly requiring the extension be on.

That'd the the idea.

>> It's a super weird one, but there's a bunch of cases in RISC-V where
>> we're told to just ignore words in the ISA manual.  Definitely a trap
>> for users (and we already had some Linux folks get bit by the counter
>> changes here), but that's just how RISC-V works.
>
> Mistakes happen.  The key is to adjust for them as best as we can.   
> I'd lean towards a stricter enforcement, bringing these
> instructions/extension in line with how we handle the others. It'd
> potentially mean source incompatibilities that would need to be fixed,
> but they shouldn't be difficult and we're still early enough in the game
> that we *could* take that route.  Andrew's position is more
> accommodating of existing code and while I may not 100% agree with his
> position, I understand it.
>
>
> So while I'd lean towards a stricter checking, I can live with this
> approach.  I wouldn't mind hearing from Kito, Philipp and others though.

That's the sort of thing we've traditionally done: essentially just read 
the actual words in the PDF and produce implementations that match 
those, tagging versions when things change (the fence.i stuff is a good 
example).  After some amount of time we can then move the default spec 
version over to the new one.  That's a little bit of churn for users, 
but it shouldn't be all that bad.

IMO that's the sane way to go, I'd certainly expect to be able to read 
the words in the PDFs and go implement things according to them.  It's 
pretty clearly not what the ISA folks want, though.

There's also the secondary issue of getting ISA strings to match between 
the various bits of the software stack that uses them.  We're trying to 
move away from ISA strings as a stable uABI in Linux for exactly this 
reason, but ISA strings have already ended up all over the place so 
there's only so much we can do.

Re: [PATCH] RISC-V: Add the Zihpm and Zicntr extensions

[Kito Cheng]

I realized we missed this on trunk, and I need this on adding -mcpu
for sfive cores, so I'm gonna push this to trunk.
Most concerns are around the assembler stuff, so I believe it's less
controversial on the toolchain driver side.

On Wed, Nov 23, 2022 at 6:01 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> On Tue, 22 Nov 2022 13:50:28 PST (-0800), jeffreyalaw@gmail.com wrote:
> >
> > On 11/22/22 08:29, Palmer Dabbelt wrote:
> >> On Tue, 22 Nov 2022 07:20:15 PST (-0800), jeffreyalaw@gmail.com wrote:
> >>>
> >>> On 11/20/22 18:36, Kito Cheng wrote:
> >>>>> So the idea here is just to define the extension so that it gets
> >>>>> defined
> >>>>> in the ISA strings and passed through to the assembler, right?
> >>>> That will also define arch test marco:
> >>>>
> >>>> https://github.com/riscv-non-isa/riscv-c-api-doc/blob/master/riscv-c-api.md#architecture-extension-test-macro
> >>>>
> >>>
> >>> Sorry I should have been clearer and included the test macro(s) as well.
> >>>
> >>> So a better summary would be that while it doesn't change the codegen
> >>> behavior in the compiler, it does provide the mechanisms to pass along
> >>> isa strings to other tools such as the assembler and signal via the test
> >>> macros that this extension is available.
> >>
> >> IMO the important bit here is that we're not adding any compatibility
> >> flags, like we did when fence.i was removed from the ISA.  That's fine
> >> as long as we never remove these instructions from the base ISA in the
> >> software, but that's what's suggested by Andrew in the post.
> >
> > Right.  IIUC these instructions were never supposed to be in the base
> > ISA, but, in effect, snuck through.  We're retro-actively adding them as
> > an extension, at least in terms of ISA strings & test macros.  We're
> > currently (forever?) going to allow them in the assembler without
> > strictly requiring the extension be on.
>
> That'd the the idea.
>
> >> It's a super weird one, but there's a bunch of cases in RISC-V where
> >> we're told to just ignore words in the ISA manual.  Definitely a trap
> >> for users (and we already had some Linux folks get bit by the counter
> >> changes here), but that's just how RISC-V works.
> >
> > Mistakes happen.  The key is to adjust for them as best as we can.
> > I'd lean towards a stricter enforcement, bringing these
> > instructions/extension in line with how we handle the others. It'd
> > potentially mean source incompatibilities that would need to be fixed,
> > but they shouldn't be difficult and we're still early enough in the game
> > that we *could* take that route.  Andrew's position is more
> > accommodating of existing code and while I may not 100% agree with his
> > position, I understand it.
> >
> >
> > So while I'd lean towards a stricter checking, I can live with this
> > approach.  I wouldn't mind hearing from Kito, Philipp and others though.
>
> That's the sort of thing we've traditionally done: essentially just read
> the actual words in the PDF and produce implementations that match
> those, tagging versions when things change (the fence.i stuff is a good
> example).  After some amount of time we can then move the default spec
> version over to the new one.  That's a little bit of churn for users,
> but it shouldn't be all that bad.
>
> IMO that's the sane way to go, I'd certainly expect to be able to read
> the words in the PDFs and go implement things according to them.  It's
> pretty clearly not what the ISA folks want, though.
>
> There's also the secondary issue of getting ISA strings to match between
> the various bits of the software stack that uses them.  We're trying to
> move away from ISA strings as a stable uABI in Linux for exactly this
> reason, but ISA strings have already ended up all over the place so
> there's only so much we can do.