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

[Palmer Dabbelt <palmer@rivosinc.com>]

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.