[PATCH] D78764: [RISCV] Update debug scratch register names

Thu Apr 30 01:12:48 PDT 2020

asb added a comment.

Sorry to ask you to move things back, but I think the debug CSR tests would make most sense in machine-csr-names.s on the basis that the privileged spec does list those CSRs in the "machine-level CSR names" table.

================
Comment at: llvm/lib/Target/RISCV/RISCVSystemOperands.td:23
   bits<12> Encoding = op;
+  // FIXME: support multiple aliases when needed
+  string AltName = name;
----------------
apazos wrote:
> I think this limitation is fine.
> I have not seen a case with CSRs where we need a list of strings to support multiple aliases.
> Let's see what Alex or others say
I agree the limitation isn't a problem for the RISC-V backend right now. It's conceivable it could become one. My only concern with leaving it as a FIXME is it might prompt someone to invest effort in this when there's not much reason to do so. Maybe better as `// Note: changes to SearchableTableEmitter may be needed if multiple aliases are needed in the future`. But I'm not sure what is most consistent with the use of FIXME elsewhere in the codebase. Plus a note like that risks becoming stale. With that in mind, I'd err towards a factual `// A maximum of one alias is supported right now.`

But ultimately, I don't have a particularly strong view about how to best comment this.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78764/new/

https://reviews.llvm.org/D78764