-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[RISCV] Support [mh]edelegh CSRs #121634
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RISCV] Support [mh]edelegh CSRs #121634
Conversation
|
@llvm/pr-subscribers-mc @llvm/pr-subscribers-backend-risc-v Author: None (dong-miao) ChangesAccording to the newest RISC-V Privileged Spec v1.13, CSRs have been updated. Full diff: https://github.com/llvm/llvm-project/pull/121634.diff 3 Files Affected:
diff --git a/llvm/lib/Target/RISCV/RISCVSystemOperands.td b/llvm/lib/Target/RISCV/RISCVSystemOperands.td
index 39853cf13a920c..41b96e1497e706 100644
--- a/llvm/lib/Target/RISCV/RISCVSystemOperands.td
+++ b/llvm/lib/Target/RISCV/RISCVSystemOperands.td
@@ -158,6 +158,7 @@ def : SysReg<"hip", 0x644>;
def : SysReg<"hvip", 0x645>;
def : SysReg<"htinst", 0x64A>;
def : SysReg<"hgeip", 0xE12>;
+def : SysReg<"hedelegh", 0x612>;
//===----------------------------------------------------------------------===//
// Hypervisor Configuration
@@ -239,6 +240,7 @@ def : SysReg<"mbadaddr", 0x343>;
def : SysReg<"mip", 0x344>;
def : SysReg<"mtinst", 0x34A>;
def : SysReg<"mtval2", 0x34B>;
+def : SysReg<"medelegh", 0x312>;
//===----------------------------------------------------------------------===//
// Machine Configuration
diff --git a/llvm/test/MC/RISCV/rv32-hypervisor-csr-names.s b/llvm/test/MC/RISCV/rv32-hypervisor-csr-names.s
index aadee4fb4f3add..79d87b3f2471cd 100644
--- a/llvm/test/MC/RISCV/rv32-hypervisor-csr-names.s
+++ b/llvm/test/MC/RISCV/rv32-hypervisor-csr-names.s
@@ -219,3 +219,21 @@ csrrs t2, 0x214, zero
csrrs t1, vsiph, zero
# uimm12
csrrs t2, 0x254, zero
+
+##################################
+# Hypervisor Trap Setup
+##################################
+
+# hedelegh
+# name
+# CHECK-INST: csrrs t1, hedelegh, zero
+# CHECK-ENC: encoding: [0x73,0x23,0x20,0x61]
+# CHECK-INST-ALIAS: csrr t1, hedelegh
+# uimm12
+# CHECK-INST: csrrs t2, hedelegh, zero
+# CHECK-ENC: encoding: [0xf3,0x23,0x20,0x61]
+# CHECK-INST-ALIAS: csrr t2, hedelegh
+# name
+csrrs t1, hedelegh, zero
+# uimm12
+csrrs t2, 0x612, zero
diff --git a/llvm/test/MC/RISCV/rv32-machine-csr-names.s b/llvm/test/MC/RISCV/rv32-machine-csr-names.s
index 3d527e382376e7..9e929b7eddeeda 100644
--- a/llvm/test/MC/RISCV/rv32-machine-csr-names.s
+++ b/llvm/test/MC/RISCV/rv32-machine-csr-names.s
@@ -22,6 +22,20 @@ csrrs t1, mstatush, zero
# uimm12
csrrs t2, 0x310, zero
+# medelegh
+# name
+# CHECK-INST: csrrs t1, medelegh, zero
+# CHECK-ENC: encoding: [0x73,0x23,0x20,0x31]
+# CHECK-INST-ALIAS: csrr t1, medelegh
+# uimm12
+# CHECK-INST: csrrs t2, medelegh, zero
+# CHECK-ENC: encoding: [0xf3,0x23,0x20,0x31]
+# CHECK-INST-ALIAS: csrr t2, medelegh
+# name
+csrrs t1, medelegh, zero
+# uimm12
+csrrs t2, 0x312, zero
+
#########################
# Machine Configuration
#########################
|
| def : SysReg<"hvip", 0x645>; | ||
| def : SysReg<"htinst", 0x64A>; | ||
| def : SysReg<"hgeip", 0xE12>; | ||
| def : SysReg<"hedelegh", 0x612>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need let isRV32Only = 1 in
Should this be in the "Hypervisor Trap Setup" section with hedeleg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to move to the "Hypervisor Trap Setup" section instead of "Hypervisor Trap Handling". It belongs in the same group as "hedeleg"
|
Also need to test that this CSR errors on RV64 by adding to llvm/test/MC/RISCV/rv32-only-csr-names.s |
|
topperc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message is rather lacking specifics; especially "[RISCV]Update CSRs" is (a) badly formatted (b) too terse and vague. Since this is just for RV32 [mh]edelegh CSRs, it would be appropriate to mention that explicitly.
|
A better commit message would be something like:
or:
|
Thanks for your help,I will be more careful next time. |
|
topperc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@dong-miao why was this closed? Were the changes merged separately? |
I don't know why this pull request cannot be merged. It seems that another reviewer did not approve it, but he did not provide a corresponding reason or solution.This commit has been put on hold here without any new progress. |
Myself and other reviewers look at a lot of patches. Often we forget to come back to them on our own. The best thing to do is ping the review after a week so it comes back to the top of our email and/or open review list. |
|
@jrtc27 does this look ok to you now? |
Okay. I'll pay attention to this next time. |
Is it feasible to close this PR and bring it up again? |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/13124 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/195/builds/4970 Here is the relevant piece of the build log for the reference |
These RV32-only CSRs are defined in privileged spec v1.13.
These RV32-only CSRs are defined in privileged spec v1.13.