Page MenuHomePhabricator

[RISCV] Enable the use of the old sptbr name
AbandonedPublic

Authored by arcbbb on Oct 12 2020, 8:15 PM.

Details

Summary

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.

Diff Detail

Event Timeline

arcbbb created this revision.Oct 12 2020, 8:15 PM
arcbbb requested review of this revision.Oct 12 2020, 8:15 PM
arcbbb updated this revision to Diff 297750.Oct 12 2020, 8:18 PM

It LGTM, but it's better to wait a couple of days before committing in order to give others time to comment.

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.

jrtc27 added a comment.EditedOct 13 2020, 5:08 PM

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.

I agree with @jrtc27. What's the motivation to support a register name obsoleted by a ratified spec?

asb added a comment.Oct 15 2020, 5:02 AM

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

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.

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

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.

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?

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.

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

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.

[2] https://reviews.llvm.org/D85067

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

evandro added a comment.EditedOct 19 2020, 4:58 PM

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.

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.

I think this can explain the reason that we need to give users a warning or even an error when we support OldName.

arcbbb abandoned this revision.Oct 20 2020, 2:07 AM

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.