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

[Kito Cheng <kito.cheng@gmail.com>]

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.