RISC-V: Add the Zihpm and Zicntr extensions
Checks
Commit Message
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(+)
Comments
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
>
>
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
>>
>>
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
> 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
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
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.
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
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.
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.
@@ -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},