RISCV-V Privileged Specification 1.10 Preface defines satp, which has the same numeric CSR value as sptbr from 1.09.1.
This patch enables the use of the old sptbr name.
Details
Details
- Reviewers
asb pzheng luismarques lenary mmxsrup
Diff Detail
Diff Detail
Event Timeline
Comment Actions
It LGTM, but it's better to wait a couple of days before committing in order to give others time to comment.
Comment Actions
Why in 2020 are we *adding* new backwards compatibility aliases? RISC-V is young enough that we don't need to do this, just fix the code. QEMU has even dropped support for 1.9 already.
Comment Actions
To elaborate:
Any serious RISC-V codebase using S-mode needs to be updated to the more recent privilege spec versions if it wants to have any hope of not being lost to the ages as the ecosystem is still moving quite quickly; 2 years ago this maybe would have made some sense (though I would have still been against it), but any actively-maintained RISC-V OS should have long since fixed itself by now. 1.9 is very outdated now, with both 1.10 and 1.11 having been published since (and 1.11 ratified) and further changes defined in the draft 1.12. It is a waste of time to support CSR names that were outdated even 2 years ago, and merely encourages sloppy coding practice where the old names continue to be used. Also unlike GNU binutils we are also currently unable to distinguish between ISA versions so we can't warn like it will (if you enable the feature) if you use an old CSR name. Finally, if you really want to support version 1.9, be consistent and do the whole thing or nothing at all, but don't just pick and choose specific aspects of it.
Comment Actions
I agree with @jrtc27. What's the motivation to support a register name obsoleted by a ratified spec?
Comment Actions
I think there's a little more to supporting older names than just the date of the spec. I agree that we don't expect anyone doing serious RISC-V work to be working to the 1.9 spec (unless there are cores taped out with that spec we don't know about), but in cases where there have been CSR renames it's often the case that people migrate to newer specification versions but don't always change the naming. There's an argument for the compiler complaining at them in this case, but there's also the user experience issue. It looks like the RISC-V compliance suite for instance still uses the sptbr name https://github.com/riscv/riscv-compliance/issues/107
Adding older aliases if they're found to be an issue in the wild is I think a policy that is worth considering alongside the alternate policies of adding all such aliases, or none of them (an encouraging people to update their code).
Comment Actions
This is a wider discussion, but I would favor following the ratified specs, with exceptions on specific cases. I would also favor that the toolchain would enforce compliance on its own, even if more currently than an official, yet non compliant, compliance test suite.
Comment Actions
I agree to just fix the code.
The motivation was that I saw there were two v1.9 aliases added two months ago [1,2],
and I thought it may be acceptable to add the third one. (sptbr is currently used by pk[3])
If we are not going to merge this, should the existing aliases be removed as well to make the policy clear?
[1] https://reviews.llvm.org/D78764
[2] https://reviews.llvm.org/D85067
[3] https://github.com/riscv/riscv-pk/blob/master/machine/minit.c
Comment Actions
I think supporting the old CSR names is a small, simple thing that we can do which removes a barrier to people moving to using LLVM rather than GCC. Better this than death by a thousand cuts when people try to move their software over. Yes, people *should* just update their software (and dependencies). The question is will they?
Yes, I do think at some point we need a more principled approach to CSR naming support, but in the short term, doing so in an ad-hoc manner is fine. I think some kind of warning is a good idea, but LLVM's architecture may not be well set up for that, in all honesty.
Comment Actions
Not if they don't know they have old names in use.
Yes, I do think at some point we need a more principled approach to CSR naming support, but in the short term, doing so in an ad-hoc manner is fine. I think some kind of warning is a good idea, but LLVM's architecture may not be well set up for that, in all honesty.
We could always have an OldName like we have AltName and emit a warning (maybe optionally an error( in parseCSRSystemRegister if we ever use that to get the CSR. Although currently SysReg's AltName is used exclusively for deprecated names so we could just repurpose that and defer adding OldName until we need both.
This one is for the debug spec which as far as I know has never been ratified, so there's a bit of a difference there.
Yeah maybe that one shouldn't have happened, I guess I was in a different mood at the time. But also it's a more niche CSR that's unlikely to matter much either way, whereas people really need to move on from sptbr.
[3] https://github.com/riscv/riscv-pk/blob/master/machine/minit.c
I mean, all of pk is a pile of legacy stuff people shouldn't be using these days (specifically the pk part of it; bbl is also legacy but less bad). Maybe we're doing our users a service by not letting it build. I know we had to make some build system changes in our fork (that actually break GCC and I really do not care to fix as we don't use GCC) in order to get it to build with a full LLVM-based toolchain, and that was just for bbl (one day we'll move to OpenSBI...).
Comment Actions
Supporting old names without change of function is sensible, but, in this case, the bit fields in satp are different from stpbr. Then, the sensible result should be an error, because the code does need to be ported to the new version of the spec.
Comment Actions
I think this can explain the reason that we need to give users a warning or even an error when we support OldName.
Comment Actions
Thanks for the discussion.
The idea to separate AltName and OldName, and issue a warning are good,
and I think it is sptbr that is not convincing.