This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Update debug scratch register names
ClosedPublic

Authored by pzheng on Apr 23 2020, 3:08 PM.

Details

Summary

The RISC-V debug register was named dscratch in a previous draft of the RISC-V
debug mode spec. The number of registers has been increased to 2 in the latest
ratified version of the debug mode spec and the registers were named dscratch0
and dscratch1. We still support using the old register name "dscratch", but it
would be disassembled as "dscratch0" with this change.

Diff Detail

Event Timeline

pzheng created this revision.Apr 23 2020, 3:08 PM
jrtc27 added inline comments.Apr 23 2020, 5:04 PM
llvm/lib/Target/RISCV/RISCVSystemOperands.td
358

I think

def : SysReg<"dscratch0", 0x7B2> {
  let AltName = "dscratch";
}

is nicer, but otherwise please drop the braces, ie:

let AltName = "dscratch" in
def : SysReg<"dscratch0", 0x7B2>
pzheng updated this revision to Diff 259790.Apr 23 2020, 7:56 PM

Addressing comments

pzheng marked an inline comment as done.Apr 23 2020, 7:56 PM
apazos added inline comments.Apr 24 2020, 11:58 AM
llvm/test/MC/RISCV/machine-csr-names.s
369

Maybe we should move these tests to debug-csr-names.s, similar to what you did in D78583.
I created this file in the past for CSRs vailid in machine mode.

pzheng updated this revision to Diff 259956.Apr 24 2020, 12:18 PM

Moving debug mode registers to a new test file

pzheng marked an inline comment as done.Apr 24 2020, 12:19 PM
apazos added inline comments.Apr 24 2020, 12:41 PM
llvm/lib/Target/RISCV/RISCVSystemOperands.td
23

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

asb added a comment.Apr 30 2020, 1:11 AM

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.

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

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.

pzheng updated this revision to Diff 261839.May 4 2020, 9:01 AM

Address @asb's comments

pzheng marked an inline comment as done.May 4 2020, 9:02 AM
asb accepted this revision.May 5 2020, 7:47 AM

LGTM, thanks. Left one tiny nit inline.

llvm/lib/Target/RISCV/Utils/RISCVBaseInfo.h
160

I'd drop this FIXME - it's not an immediate problem and the limitation is documented in RISCVSystemOperands.td

This revision is now accepted and ready to land.May 5 2020, 7:47 AM
pzheng updated this revision to Diff 262127.May 5 2020, 8:43 AM

Good catch. Thanks for reviewing, @asb.

pzheng marked an inline comment as done.May 5 2020, 8:44 AM
This revision was automatically updated to reflect the committed changes.

Note: this patch maintains backwards compatibility with a preliminary spec.