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

[Palmer Dabbelt <palmer@rivosinc.com>]

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