Page MenuHomePhabricator

RISCV: add a few deprecated aliases for CSRs
ClosedPublic

Authored by compnerd on May 5 2021, 9:19 AM.

Details

Summary

This adds the {s,u,m}badaddr CSR aliases as well as the sptbr alias.
These are for compatibility with binutils. Furthermore, these are used
by the RISC-V Proxy Kernel and are required to enable building the Proxy
Kernel with the LLVM IAS.

The aliases here are deprecated. These are being introduced in order to
provide a compatibility story for the existing GNU toolchain, which
still supports the deprecated spelling in the assembler. However, in
order to encourage the migration of existing coding, we provide warnings
indicating that the aliased CSRs are deprecated and should be replaced.

Diff Detail

Event Timeline

compnerd created this revision.May 5 2021, 9:19 AM
compnerd requested review of this revision.May 5 2021, 9:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 5 2021, 9:19 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
lenary resigned from this revision.May 5 2021, 9:21 AM
lenary added a reviewer: asb.
jrtc27 requested changes to this revision.May 5 2021, 9:22 AM

I've vetoed these kinds of changes in the past. People should fix their code; it's been many _years_ since those CSRs were called that, and sptbr actually meant something different in the past too. The old names were never ratified.

This revision now requires changes to proceed.May 5 2021, 9:22 AM

Hmm, perhaps we should be special casing these and adding diagnostics then. The thing is, that binutils still carries the aliases, and generally, LLVM has strived for compatibility with binutils so that it can remain a drop-in replacement.

jrtc27 added a comment.May 5 2021, 9:28 AM

Hmm, perhaps we should be special casing these and adding diagnostics then. The thing is, that binutils still carries the aliases, and generally, LLVM has strived for compatibility with binutils so that it can remain a drop-in replacement.

Well, binutils should properly deprecate the IMO.

Sure, but even if they were to deprecate them, that would be years before code would really be even nudged towards removal. In the mean time, this prevents the use of the IAS for building software, which is rather unfortunate. I think that supporting the aliases and adding a deprecation warning on use seems to be better. That way, we can actually both encourage people to update the code as well as remain compatible.

jrtc27 added a comment.May 5 2021, 9:41 AM

As I said in the other review, I'm fine with allowing them with a warning, just not silently accepting them and letting this cruft continue to live on. But I don't want to write that code, and believe it's likely much simpler to just fix the code in question; it'd take all of 5 minutes to fix riscv-pk to not be stuck in ~2017.

I don't disagree about the ease of updating that code (and, in fact, I've created PRs #241, #242, #243 on the project). However, the issue here is more than just the single project.

Maintaining compatibility with the tooling that LLVM is designed to be a drop in replacement for is important. This is a step towards that.

compnerd updated this revision to Diff 343103.May 5 2021, 10:31 AM
compnerd retitled this revision from RISCV: add a few more aliases for CSRs to RISCV: add a few deprecated aliases for CSRs.
compnerd edited the summary of this revision. (Show Details)

@jrtc27 - I'm open to improving the diagnostics, but this plumbs the functionality.

asb added a comment.May 13 2021, 1:12 AM

I'll put this patch on the agenda for the sync-up call today, but particularly now it emits a warning I'm inclined to think we should accept this in order to provide drop-in compatibility for GNU toolchains.

I'll put this patch on the agenda for the sync-up call today, but particularly now it emits a warning I'm inclined to think we should accept this in order to provide drop-in compatibility for GNU toolchains.

We discussed this as part of today's LLVM RISC-V community sync-up call. I don't think there objections to merging this support now that it also emits warnings (correct me if I'm misremembering). Personally, I think it's a borderline decision, but I can see the appeal of ensuring drop-in compatibility with the GNU toolchains. Let's give it a few more days to let others chime in, now that this issue has been brought up in the call.

I'll put this patch on the agenda for the sync-up call today, but particularly now it emits a warning I'm inclined to think we should accept this in order to provide drop-in compatibility for GNU toolchains.

We discussed this as part of today's LLVM RISC-V community sync-up call. I don't think there objections to merging this support now that it also emits warnings (correct me if I'm misremembering). Personally, I think it's a borderline decision, but I can see the appeal of ensuring drop-in compatibility with the GNU toolchains. Let's give it a few more days to let others chime in, now that this issue has been brought up in the call.

Yeah. At this point I'd personally prefer to keep it an error, backwards compatibility makes less and less sense as time goes on, but I'm not going to object now there are warnings.

Ping; it seems like there aren't any objections, can I get a final sign off on the change?

craig.topper added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1318

Shouldn't this be aligned with the "'" on the previous line or did clang-format get confused somehow?

llvm/lib/Target/RISCV/RISCVSystemOperands.td
25

Should we document that DeprecatedName emits a warning? AltName appears to also be used for older names.

craig.topper added inline comments.May 19 2021, 1:59 PM
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1318

Nevermind I guess, that's actually clang-format indenting it by 4 spaces?

compnerd marked 2 inline comments as done.May 19 2021, 2:13 PM
compnerd added inline comments.
llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
1318

Yeah, this is what clang-format wants.

llvm/lib/Target/RISCV/RISCVSystemOperands.td
25

Ah, good idea. I'll update the comment (can do that right before committing).

This revision was not accepted when it landed; it landed in state Needs Review.May 21 2021, 1:55 PM
This revision was automatically updated to reflect the committed changes.
compnerd marked an inline comment as done.