Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zicntr and zihpm #43

Closed
jim-wilson opened this issue Apr 15, 2022 · 6 comments
Closed

zicntr and zihpm #43

jim-wilson opened this issue Apr 15, 2022 · 6 comments

Comments

@jim-wilson
Copy link

The references to these extensions could be problematic, as no one ever told the software folk to add them. I didn't even know that they existed until I read this spec. Anyone trying to specify a full architecture spec string is going to find that software tools won't accept them, which is going to be confusing.

@aswaterman
Copy link
Member

Indeed, it was an oversight that these were never explicitly ratified, as @kasanovic mentioned in this commit: 77aff0b

The ratification process should alert more people to their existence.

Toolchains will need updating to support the profiles concept, anyway, and these changes can hopefully be folded into the same release.

@jim-wilson
Copy link
Author

This is also yet another backward compatibility break for the software. Old versions of binutils will accept cntr and hpm csrs with just the base I extension. If new versions of binutils require specifying zicntr and/or zihpm then some code is going to break. It isn't as big a problem as the zicsr break, as not as much code uses the cntr/hpm registers, but stuff will break, and people will be annoyed. We really need to stop making backward compatibility breaks at some point. Or more clearly distinguish new versions of the RISC-V ISA from old ones, like the ARMvX scheme that ARM uses.

@aswaterman
Copy link
Member

aswaterman commented Apr 15, 2022

My advice would be to accept and ignore these new ISA strings--i.e., continue making these CSRs unconditionally available. This is easy for the toolchain and will annoy the fewest number of users.

This obviously only works for extensions that the compiler won't automatically target, but these extensions would seem to fit the bill.

(I gave the same advice when Zicsr was introduced, but I can't remember why it was rejected.)

@jim-wilson
Copy link
Author

Software that lies about hardware support isn't a good long term strategy. You can't catch programmer errors if we aren't properly implementing the ISA spec in the software tools. So there will be trouble no matter how we handle this. We were hoping that zicsr was a one time break, but zicntr and zihpm are another break and too late for binutils-2.38 released in January so too late for gcc-12 to be released within a month or so. But you are right that if we can get people to move from ISA strings to profile strings that should eliminate some of the hassle.

@aswaterman
Copy link
Member

aswaterman commented Apr 18, 2022

I've gotten some flak through other channels about Zicntr/Ziphm being backwards-incompatible changes, so I wanted to set the record straight here. The definition of these extensions is compatible with the ratified RVI 2.1.

Two related mistakes happened since the ratification of the base ISAs. The first is that we weren't prompt in standardizing Zicntr/Zihpm; we should've done so years ago. Mea culpa.

The second is that the GNU toolchain assumed that these CSRs should be accessible if Zicsr is present. The ISA specs never justified this interpretation. But that assumption has been working out fine this whole time. With that in mind, I don't understand why the introduction of these extension names should change the toolchain's behavior... hence my earlier suggestion to simply ignore the new extension names.

(BTW, I understand your point, and agree with it in the common case. Nevertheless, I don't think it's important in practice for these particular extensions. And the fact that the toolchain has technically been too permissive for a few years would seem to bolster my take.)

@kasanovic
Copy link
Collaborator

Closing this. It has been debated ad nauseam in other venues and we have a resolution.

topperc added a commit to llvm/llvm-project that referenced this issue Apr 26, 2023
This change adds the definition of the two extensions, but does not either a) make any register definitions conditional on them or b) enabled the extensions by default.

This is somewhat analogous to https://reviews.llvm.org/D143953, but with some key differences.  The best discussion I can find on status is here: riscv/riscv-profiles#43.  These were removed between document version 2.1 and 2.2, but were not defined as new extensions in 2.2.  That addition came later - in March 2022.

According to https://drive.google.com/file/d/1qa57pePesOiDOrNzxuuGFhCL4Rbi9AYB/view these were ratified in March 2023.

Reviewed By: asb, reames

Differential Revision: https://reviews.llvm.org/D144215
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants