[RISC-V][MC] Accept an absolute variable value as a CSR number #67377
Conversation
This is used by the riscv-arch-tests repository and is accepted by GCC so it seems like we should be doing the same here.
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-mc ChangesThis is used by https://github.com/riscv-non-isa/riscv-arch-test and is accepted by GCC so it seems like we should be doing the same here. Full diff: https://github.com/llvm/llvm-project/pull/67377.diff 2 Files Affected:
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 7d8d82e381313bf..4b1a5745308dcc2 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -1762,6 +1762,19 @@ ParseStatus RISCVAsmParser::parseCSRSystemRegister(OperandVector &Operands) {
SMLoc S = getLoc();
const MCExpr *Res;
+ auto SysRegFromConstantInt = [](const MCExpr *E, SMLoc S) {
+ if (auto *CE = dyn_cast<MCConstantExpr>(E)) {
+ int64_t Imm = CE->getValue();
+ if (isUInt<12>(Imm)) {
+ auto SysReg = RISCVSysReg::lookupSysRegByEncoding(Imm);
+ // Accept an immediate representing a named or un-named Sys Reg
+ // if the range is valid, regardless of the required features.
+ return RISCVOperand::createSysReg(SysReg ? SysReg->Name : "", S, Imm);
+ }
+ }
+ return std::unique_ptr<RISCVOperand>();
+ };
+
switch (getLexer().getKind()) {
default:
return ParseStatus::NoMatch;
@@ -1775,17 +1788,9 @@ ParseStatus RISCVAsmParser::parseCSRSystemRegister(OperandVector &Operands) {
if (getParser().parseExpression(Res))
return ParseStatus::Failure;
- auto *CE = dyn_cast<MCConstantExpr>(Res);
- if (CE) {
- int64_t Imm = CE->getValue();
- if (isUInt<12>(Imm)) {
- auto SysReg = RISCVSysReg::lookupSysRegByEncoding(Imm);
- // Accept an immediate representing a named or un-named Sys Reg
- // if the range is valid, regardless of the required features.
- Operands.push_back(
- RISCVOperand::createSysReg(SysReg ? SysReg->Name : "", S, Imm));
- return ParseStatus::Success;
- }
+ if (auto SysOpnd = SysRegFromConstantInt(Res, S)) {
+ Operands.push_back(std::move(SysOpnd));
+ return ParseStatus::Success;
}
return generateImmOutOfRangeError(S, 0, (1 << 12) - 1);
@@ -1851,6 +1856,18 @@ ParseStatus RISCVAsmParser::parseCSRSystemRegister(OperandVector &Operands) {
return ParseStatus::Success;
}
+ // Accept a symbol name that evaluates to an absolute value.
+ MCSymbol *Sym = getContext().lookupSymbol(Identifier);
+ if (Sym && Sym->isVariable()) {
+ // Pass false for SetUsed, since redefining the value later does not
+ // affect this instruction.
+ if (auto SysOpnd = SysRegFromConstantInt(
+ Sym->getVariableValue(/*SetUsed=*/false), S)) {
+ Operands.push_back(std::move(SysOpnd));
+ return ParseStatus::Success;
+ }
+ }
+
return generateImmOutOfRangeError(S, 0, (1 << 12) - 1,
"operand must be a valid system register "
"name or an integer in the range");
diff --git a/llvm/test/MC/RISCV/csr-abs-sym.s b/llvm/test/MC/RISCV/csr-abs-sym.s
new file mode 100644
index 000000000000000..93ecab0db106ba9
--- /dev/null
+++ b/llvm/test/MC/RISCV/csr-abs-sym.s
@@ -0,0 +1,48 @@
+## Check that we can use an absolute symbol value as a CSR number
+# RUN: llvm-mc -triple riscv32 %s | FileCheck %s
+# RUN: not llvm-mc -triple riscv32 %s --defsym=CHECK_BAD=1 2>&1 | FileCheck %s --check-prefix=ERROR
+
+.set fflags_abs_sym, 1
+csrr a0, fflags_abs_sym
+# CHECK: csrr a0, fflags
+
+csrr a0, (fflags_abs_sym+1)
+# CHECK: csrr a0, frm
+
+.equ fplus_one_abs_sym, fflags_abs_sym + 1
+csrr a0, fplus_one_abs_sym
+# CHECK: csrr a0, frm
+
+## Check that redefining the value is allowed
+## If we were to use Sym->getVariableValue(true) this code would assert with
+## Assertion `!IsUsed && "Cannot set a variable that has already been used."' failed.
+.set csr_index, 1
+# CHECK: csrr a0, fflags
+csrr a0, csr_index
+.set csr_index, 2
+# CHECK: csrr a0, frm
+csrr a0, csr_index
+
+.ifdef CHECK_BAD
+.set out_of_range, 4097
+csrr a0, out_of_range
+# ERROR: [[#@LINE-1]]:10: error: operand must be a valid system register name or an integer in the range [0, 4095]
+
+csrr a0, undef_symbol
+# ERROR: [[#@LINE-1]]:10: error: operand must be a valid system register name or an integer in the range [0, 4095]
+
+local_label:
+csrr a0, local_label
+# ERROR: [[#@LINE-1]]:10: error: operand must be a valid system register name or an integer in the range [0, 4095]
+
+.Lstart:
+.space 10
+.Lend:
+csrr a0, .Lstart-.Lend
+# ERROR: [[#@LINE-1]]:10: error: operand must be a valid system register name or an integer in the range [0, 4095]
+
+.set dot_set_sym_diff, .Lstart-.Lend
+csrr a0, dot_set_sym_diff
+# ERROR: [[#@LINE-1]]:10: error: operand must be a valid system register name or an integer in the range [0, 4095]
+
+.endif
|
The LLVM assembler does not accept string comparisons such as `\__MODE__\() == M` and instead requires the .ifc/.ifnc directive. This requires expanding some .elseif to nested else since GNU AS does not allow use of `.elseifc` (although in this case LLVM does). Another options would have been `.set M, 0x<constant>` to evaluate the expression as integers but I feel that using an explicit string comparison is cleaner. With LLVM patched to include llvm/llvm-project#67377 I was able to use riscof to compare RV64 sail assembled with LLVM vs. RV64 sail assembled with GCC.
The LLVM assembler does not accept string comparisons such as `\__MODE__\() == M` and instead requires the .ifc/.ifnc directive. This requires expanding some .elseif to nested else since GNU AS does not allow use of `.elseifc` (although in this case LLVM does). Another options would have been `.set M, 0x<constant>` to evaluate the expression as integers but I feel that using an explicit string comparison is cleaner. With LLVM patched to include llvm/llvm-project#67377 I was able to use riscof to compare RV64 sail assembled with LLVM vs. RV64 sail assembled with GCC.
|
ping? |
|
The RISC-V test suite changes were merged, so it would be good to also get the LLVM side side changes landed (and maybe even backported to 17?) This is a rather small change so hopefully that's ok. |
|
I mean, if GNU as accepts this then I guess so should we, but why can't we also make riscv-arch-test just use the preprocessor like a normal project? |
|
What I don't like about this syntax is it introduces parsing ambiguity if you do, say, |
I had a look into that and it seemed like I'd have to basically rewrite most of their macros to make that work since they use an assembler macro argument to remap the values dynamically. I agree that the ambiguity is not nice. Currently the string name takes precedence, but I notice that there are no tests for this. Will need to check what binutils does. CC: @allenjbaum |
|
I'm also not a huge fan of the introduced ambiguity, but I confirm that GNU Binutils have the same behavior as the one proposed in this patch by selecting the CSR first. So, I'd be up to add a test to exhibit that behavior and accept this change since being able to run the riscv-arch-test test-suite would be valuable to all LLVM based RISCV toolchains in my opinion. |
I agree, let's add a test case to document the behaviour but otherwise match binutils here. |
Sounds good, I'll try to work on adding tests later this week. |
|
Hi @arichardson, Just a reminder that this PR is still open ;-) |
This is used by https://github.com/riscv-non-isa/riscv-arch-test and is accepted by GCC so it seems like we should be doing the same here.