RISCV: add a few deprecated aliases for CSRs

[compnerd]

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.

[jrtc27]

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.

[jrtc27]

See https://reviews.llvm.org/D89288

[compnerd]

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]

In D101919#2739285, @compnerd wrote:

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.

[compnerd]

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]

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.

[compnerd]

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]

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

[compnerd]

@jrtc27 ping

[asb]

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.

[luismarques]

In D101919#2756343, @asb wrote:

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.

[jrtc27]

In D101919#2757958, @luismarques wrote:

In D101919#2756343, @asb wrote:

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.

[compnerd]

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

[craig.topper]

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]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

1318

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

[compnerd]

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

[craig.topper]

LGTM

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

RISCVAsmParser::parseCSRSystemRegister(OperandVector &Operands) {
   case AsmToken::Identifier: {
     StringRef Identifier;
     if (getParser().parseIdentifier(Identifier))
       return MatchOperand_ParseFail;
 
     auto SysReg = RISCVSysReg::lookupSysRegByName(Identifier);
     if (!SysReg)
       SysReg = RISCVSysReg::lookupSysRegByAltName(Identifier);
+    if (!SysReg)
+      if ((SysReg = RISCVSysReg::lookupSysRegByDeprecatedName(Identifier)))
+        Warning(S, "'" + Identifier + "' is a deprecated alias for '" +
+                       SysReg->Name + "'");
+
     // Accept a named Sys Reg if the required features are present.
     if (SysReg) {
       if (!SysReg->haveRequiredFeatures(getSTI().getFeatureBits())) {
         Error(S, "system register use requires an option to be enabled");
         return MatchOperand_ParseFail;
       }
       Operands.push_back(RISCVOperand::createSysReg(
           Identifier, S, SysReg->Encoding, isRV64()));

llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h

case RISCVFPRndMode::DYN:
     return true;
   }
 }
 } // namespace RISCVFPRndMode
 
 namespace RISCVSysReg {
 struct SysReg {
   const char *Name;
-  unsigned Encoding;
   const char *AltName;
+  const char *DeprecatedName;
+  unsigned Encoding;
   // FIXME: add these additional fields when needed.
   // Privilege Access: Read, Write, Read-Only.
   // unsigned ReadWrite;
   // Privilege Mode: User, System or Machine.
   // unsigned Mode;
   // Check field name.
   // unsigned Extra;
   // Register number without the privilege bits.

llvm/lib/Target/RISCV/RISCVSystemOperands.td

 include "llvm/TableGen/SearchableTable.td"
 
 //===----------------------------------------------------------------------===//
 // CSR (control and status register read/write) instruction options.
 //===----------------------------------------------------------------------===//
 
 class SysReg<string name, bits<12> op> {
   string Name = name;
-  bits<12> Encoding = op;
   // A maximum of one alias is supported right now.
   string AltName = name;
+  // A maximum of one deprecated name is supported right now.  Unlike the
+  // `AltName` alias, a `DeprecatedName` generates a diagnostic when the name is
+  // used to encourage software to migrate away from the name.
+  string DeprecatedName = "";
+  bits<12> Encoding = op;
   // FIXME: add these additional fields when needed.
   // Privilege Access: Read and Write = 0, 1, 2; Read-Only = 3.
   // Privilege Mode: User = 0, System = 1 or Machine = 3.
   // bits<2> ReadWrite = op{11 - 10};
   // bits<2> XMode = op{9 - 8};
   // Check Extra field name and what bits 7-6 correspond to.
   // bits<2> Extra = op{7 - 6};
   // Register number without the privilege bits.
   // bits<6> Number = op{5 - 0};
   code FeaturesRequired = [{ {} }];
   bit isRV32Only = 0;
 }
 
 def SysRegsList : GenericTable {
   let FilterClass = "SysReg";
   // FIXME: add "ReadWrite", "Mode", "Extra", "Number" fields when needed.
-  let Fields = [ "Name", "Encoding", "AltName", "FeaturesRequired", "isRV32Only" ];
+  let Fields = [
+    "Name", "AltName", "DeprecatedName", "Encoding", "FeaturesRequired",
+    "isRV32Only",
+  ];
 
   let PrimaryKey = [ "Encoding" ];
   let PrimaryKeyName = "lookupSysRegByEncoding";
 }
 
 def lookupSysRegByName : SearchIndex {
   let Table = SysRegsList;
   let Key = [ "Name" ];
 }
 
 def lookupSysRegByAltName : SearchIndex {
   let Table = SysRegsList;
   let Key = [ "AltName" ];
 }
 
+def lookupSysRegByDeprecatedName : SearchIndex {
+  let Table = SysRegsList;
+  let Key = [ "DeprecatedName" ];
+}
+
 // The following CSR encodings match those given in Tables 2.2,
 // 2.3, 2.4 and  2.5 in the RISC-V Instruction Set Manual
 // Volume II: Privileged Architecture.
 
 //===--------------------------
 // User Trap Setup
 //===--------------------------
 def : SysReg<"ustatus", 0x000>;
 def : SysReg<"uie", 0x004>;
 def : SysReg<"utvec", 0x005>;
 
 //===--------------------------
 // User Trap Handling
 //===--------------------------
 def : SysReg<"uscratch", 0x040>;
 def : SysReg<"uepc", 0x041>;
 def : SysReg<"ucause", 0x042>;
+let DeprecatedName = "ubadaddr" in
 def : SysReg<"utval", 0x043>;
 def : SysReg<"uip", 0x044>;
 
 //===--------------------------
 // User Floating-Point CSRs
 //===--------------------------
 
 def SysRegFFLAGS : SysReg<"fflags", 0x001>;

 def : SysReg<"scounteren", 0x106>;
 
 //===--------------------------
 // Supervisor Trap Handling
 //===--------------------------
 def : SysReg<"sscratch", 0x140>;
 def : SysReg<"sepc", 0x141>;
 def : SysReg<"scause", 0x142>;
+let DeprecatedName = "sbadaddr" in
 def : SysReg<"stval", 0x143>;
 def : SysReg<"sip", 0x144>;
 
 //===-------------------------------------
 // Supervisor Protection and Translation
 //===-------------------------------------
+let DeprecatedName = "sptbr" in
 def : SysReg<"satp", 0x180>;
 
 //===-----------------------------
 // Machine Information Registers
 //===-----------------------------
 
 def : SysReg<"mvendorid", 0xF11>;
 def : SysReg<"marchid", 0xF12>;

 def : SysReg<"mcounteren", 0x306>;
 
 //===-----------------------------
 // Machine Trap Handling
 //===-----------------------------
 def : SysReg<"mscratch", 0x340>;
 def : SysReg<"mepc", 0x341>;
 def : SysReg<"mcause", 0x342>;
+let DeprecatedName = "mbadaddr" in
 def : SysReg<"mtval", 0x343>;
 def : SysReg<"mip", 0x344>;
 
 //===----------------------------------
 // Machine Protection and Translation
 //===----------------------------------
 def : SysReg<"pmpcfg0", 0x3A0>;
 def : SysReg<"pmpcfg2", 0x3A2>;

llvm/test/MC/RISCV/deprecated-csr-names.s

+# RUN: llvm-mc -triple riscv32 -riscv-no-aliases -show-encoding %s \
+# RUN:     | FileCheck -check-prefixes CHECK-INST,CHECK-ENC %s
+# RUN: llvm-mc -filetype obj -triple riscv32 %s \
+# RUN:     | llvm-objdump -d - \
+# RUN:     | FileCheck -check-prefix=CHECK-INST-ALIAS %s
+
+# RUN: llvm-mc -triple riscv64 -riscv-no-aliases -show-encoding %s \
+# RUN:     | FileCheck -check-prefixes CHECK-INST,CHECK-ENC %s
+# RUN: llvm-mc -filetype obj -triple riscv64 %s \
+# RUN:     | llvm-objdump -d - \
+# RUN:     | FileCheck -check-prefix=CHECK-INST-ALIAS %s
+
+# RUN: llvm-mc -triple riscv32 %s 2>&1 | FileCheck -check-prefix CHECK-WARN %s
+
+# sbadaddr
+# name
+# CHECK-INST: csrrw zero, stval, zero
+# CHECK-ENC: encoding: [0x73,0x10,0x30,0x14]
+# CHECK-INST-ALIAS: csrw stval, zero
+# uimm12
+# CHECK-INST: csrrw zero, stval, zero
+# CHECK-ENC: encoding: [0x73,0x10,0x30,0x14]
+# CHECK-INST-ALIAS: csrw stval, zero
+# name
+csrw sbadaddr, zero
+# uimm12
+csrrw zero, 0x143, zero
+
+# CHECK-WARN: warning: 'sbadaddr' is a deprecated alias for 'stval'
+
+# mbadaddr
+# name
+# CHECK-INST: csrrw zero, mtval, zero
+# CHECK-ENC: encoding: [0x73,0x10,0x30,0x34]
+# CHECK-INST-ALIAS: csrw mtval, zero
+# uimm12
+# CHECK-INST: csrrw zero, mtval, zero
+# CHECK-ENC: encoding: [0x73,0x10,0x30,0x34]
+# CHECK-INST-ALIAS: csrw mtval, zero
+# name
+csrw mbadaddr, zero
+# uimm12
+csrrw zero, 0x343, zero
+
+# CHECK-WARN: warning: 'mbadaddr' is a deprecated alias for 'mtval'
+
+# ubadaddr
+# name
+# CHECK-INST: csrrw zero, utval, zero
+# CHECK-ENC: encoding: [0x73,0x10,0x30,0x04]
+# CHECK-INST-ALIAS: csrw utval, zero
+# uimm12
+# CHECK-INST: csrrw zero, utval, zero
+# CHECK-ENC: encoding: [0x73,0x10,0x30,0x04]
+# CHECK-INST-ALIAS: csrw utval, zero
+# name
+csrw ubadaddr, zero
+# uimm12
+csrrw zero, 0x043, zero
+
+# CHECK-WARN: warning: 'ubadaddr' is a deprecated alias for 'utval'
+
+# sptbr
+# name
+# CHECK-INST: csrrw zero, satp, zero
+# CHECK-ENC: encoding: [0x73,0x10,0x00,0x18]
+# CHECK-INST-ALIAS: csrw satp, zero
+# uimm12
+# CHECK-INST: csrrw zero, satp, zero
+# CHECK-ENC: encoding: [0x73,0x10,0x00,0x18]
+# CHECK-INST-ALIAS: csrw satp, zero
+# name
+csrw sptbr, zero
+# uimm12
+csrrw zero, 0x180, zero
+
+# CHECK-WARN: warning: 'sptbr' is a deprecated alias for 'satp'