[RISCV] Refactor register list parsing and improve error messages. #134938
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Structure the code into a loop that parses a series of ranges and gives an error when there are too many ranges. Give errors when ABI and non-ABI names are mixed. Properly diagnose 'x1-` starting a list. Use a default bool argument to merge parseRegListCommon and parseRegList.
Member
|
@llvm/pr-subscribers-backend-risc-v @llvm/pr-subscribers-mc Author: Craig Topper (topperc) ChangesStructure the code into a loop that parses a series of ranges and gives an error when there are too many ranges. Give errors when ABI and non-ABI names are mixed. Properly diagnose 'x1-` starting a list. Use a default bool argument to merge parseRegListCommon and parseRegList. Full diff: https://github.com/llvm/llvm-project/pull/134938.diff 5 Files Affected:
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index dba78fef0bad8..a1b3a3f299829 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -215,13 +215,10 @@ class RISCVAsmParser : public MCTargetAsmParser {
ParseStatus parseGPRPair(OperandVector &Operands, bool IsRV64Inst);
ParseStatus parseFRMArg(OperandVector &Operands);
ParseStatus parseFenceArg(OperandVector &Operands);
- ParseStatus parseRegList(OperandVector &Operands) {
- return parseRegListCommon(Operands, /*MustIncludeS0=*/false);
- }
+ ParseStatus parseRegList(OperandVector &Operands, bool MustIncludeS0 = false);
ParseStatus parseRegListS0(OperandVector &Operands) {
- return parseRegListCommon(Operands, /*MustIncludeS0=*/true);
+ return parseRegList(Operands, /*MustIncludeS0=*/true);
}
- ParseStatus parseRegListCommon(OperandVector &Operands, bool MustIncludeS0);
ParseStatus parseRegReg(OperandVector &Operands);
ParseStatus parseRetval(OperandVector &Operands);
@@ -2567,96 +2564,97 @@ ParseStatus RISCVAsmParser::parseRegReg(OperandVector &Operands) {
return ParseStatus::Success;
}
-ParseStatus RISCVAsmParser::parseRegListCommon(OperandVector &Operands,
- bool MustIncludeS0) {
- // RegList: {ra [, s0[-sN]]}
- // XRegList: {x1 [, x8[-x9][, x18[-xN]]]}
-
- // When MustIncludeS0 = true (not the default) (used for `qc.cm.pushfp`) which
- // must include `fp`/`s0` in the list:
- // RegList: {ra, s0[-sN]}
- // XRegList: {x1, x8[-x9][, x18[-xN]]}
+// RegList: {ra [, s0[-sN]]}
+// XRegList: {x1 [, x8[-x9][, x18[-xN]]]}
+// When MustIncludeS0 = true (not the default) (used for `qc.cm.pushfp`) which
+// must include `fp`/`s0` in the list:
+// RegList: {ra, s0[-sN]}
+// XRegList: {x1, x8[-x9][, x18[-xN]]}
+ParseStatus RISCVAsmParser::parseRegList(OperandVector &Operands,
+ bool MustIncludeS0) {
if (getTok().isNot(AsmToken::LCurly))
return ParseStatus::NoMatch;
SMLoc S = getLoc();
+
Lex();
- bool IsRVE = isRVE();
+ bool UsesXRegs;
+ MCRegister RegEnd;
+ do {
+ if (getTok().isNot(AsmToken::Identifier) ||
+ (RegEnd == RISCV::X18 && isRVE()))
+ return Error(getLoc(), "invalid register");
- if (getLexer().isNot(AsmToken::Identifier))
- return Error(getLoc(), "register list must start from 'ra' or 'x1'");
+ StringRef RegName = getTok().getIdentifier();
+ MCRegister Reg = matchRegisterNameHelper(RegName);
+ if (!Reg)
+ return Error(getLoc(), "invalid register");
- StringRef RegName = getLexer().getTok().getIdentifier();
- MCRegister RegEnd = matchRegisterNameHelper(RegName);
- if (RegEnd != RISCV::X1)
- return Error(getLoc(), "register list must start from 'ra' or 'x1'");
- getLexer().Lex();
+ if (!RegEnd) {
+ UsesXRegs = RegName[0] == 'x';
+ if (Reg != RISCV::X1)
+ return Error(getLoc(), "register list must start from 'ra' or 'x1'");
+ } else if (RegEnd == RISCV::X1) {
+ if (Reg != RISCV::X8 || (UsesXRegs != (RegName[0] == 'x')))
+ return Error(getLoc(), Twine("register must be '") +
+ (UsesXRegs ? "x8" : "s0") + "'");
+ } else if (RegEnd == RISCV::X9 && UsesXRegs) {
+ if (Reg != RISCV::X18 || (RegName[0] != 'x'))
+ return Error(getLoc(), "register must be 'x18'");
+ } else {
+ return Error(getLoc(), "too many register ranges");
+ }
- // parse case like ,s0 (knowing the comma must be there if required)
- if (parseOptionalToken(AsmToken::Comma)) {
- if (getLexer().isNot(AsmToken::Identifier))
- return Error(getLoc(), "invalid register");
- StringRef RegName = getLexer().getTok().getIdentifier();
- RegEnd = matchRegisterNameHelper(RegName);
- if (!RegEnd)
- return Error(getLoc(), "invalid register");
- if (RegEnd != RISCV::X8)
- return Error(getLoc(),
- "continuous register list must start from 's0' or 'x8'");
- getLexer().Lex(); // eat reg
+ RegEnd = Reg;
- // parse case like -s1
+ Lex();
+
+ SMLoc MinusLoc = getLoc();
if (parseOptionalToken(AsmToken::Minus)) {
- StringRef EndName = getLexer().getTok().getIdentifier();
- // FIXME: the register mapping and checks of RVE is wrong
- RegEnd = matchRegisterNameHelper(EndName);
- if (!(RegEnd == RISCV::X9 ||
- (RegEnd >= RISCV::X18 && RegEnd <= RISCV::X27)))
- return Error(getLoc(), "invalid register");
- getLexer().Lex();
- }
+ if (RegEnd == RISCV::X1)
+ return Error(MinusLoc, Twine("register '") + (UsesXRegs ? "x1" : "ra") +
+ "' cannot start a multiple register range");
- // parse extra part like ', x18[-x20]' for XRegList
- if (parseOptionalToken(AsmToken::Comma)) {
- if (RegEnd != RISCV::X9)
- return Error(
- getLoc(),
- "first contiguous registers pair of register list must be 'x8-x9'");
+ if (getTok().isNot(AsmToken::Identifier) ||
+ (RegEnd == RISCV::X18 && isRVE()))
+ return Error(getLoc(), "invalid register");
- // parse ', x18' for extra part
- if (getLexer().isNot(AsmToken::Identifier) || IsRVE)
+ StringRef RegName = getTok().getIdentifier();
+ MCRegister Reg = matchRegisterNameHelper(RegName);
+ if (!Reg)
return Error(getLoc(), "invalid register");
- StringRef EndName = getLexer().getTok().getIdentifier();
- RegEnd = MatchRegisterName(EndName);
- if (RegEnd != RISCV::X18)
- return Error(getLoc(),
- "second contiguous registers pair of register list "
- "must start from 'x18'");
- getLexer().Lex();
-
- // parse '-x20' for extra part
- if (parseOptionalToken(AsmToken::Minus)) {
- if (getLexer().isNot(AsmToken::Identifier) || IsRVE)
- return Error(getLoc(), "invalid register");
- EndName = getLexer().getTok().getIdentifier();
- RegEnd = MatchRegisterName(EndName);
- if (!(RegEnd >= RISCV::X19 && RegEnd <= RISCV::X27))
- return Error(getLoc(), "invalid register");
- getLexer().Lex();
- }
+
+ if (RegEnd == RISCV::X8) {
+ if ((Reg != RISCV::X9 &&
+ (UsesXRegs || Reg < RISCV::X18 || Reg > RISCV::X27)) ||
+ (UsesXRegs != (RegName[0] == 'x'))) {
+ if (UsesXRegs)
+ return Error(getLoc(), "register must be 'x9'");
+ return Error(getLoc(), "register must be in the range 's1' to 's11'");
+ }
+ } else if (RegEnd == RISCV::X18) {
+ if (Reg < RISCV::X19 || Reg > RISCV::X27 || (RegName[0] != 'x'))
+ return Error(getLoc(),
+ "register must be in the range 'x19' to 'x27'");
+ } else
+ llvm_unreachable("unexpected register");
+
+ RegEnd = Reg;
+
+ Lex();
}
- }
+ } while (parseOptionalToken(AsmToken::Comma));
- if (parseToken(AsmToken::RCurly, "register list must end with '}'"))
+ if (parseToken(AsmToken::RCurly, "expected ',' or '}'"))
return ParseStatus::Failure;
if (RegEnd == RISCV::X26)
- return Error(S, "invalid register list, {ra, s0-s10} or {x1, x8-x9, "
- "x18-x26} is not supported");
+ return Error(S, "invalid register list, '{ra, s0-s10}' or '{x1, x8-x9, "
+ "x18-x26}' is not supported");
- auto Encode = RISCVZC::encodeRegList(RegEnd, IsRVE);
+ auto Encode = RISCVZC::encodeRegList(RegEnd, isRVE());
assert(Encode != RISCVZC::INVALID_RLIST);
if (MustIncludeS0 && Encode == RISCVZC::RA)
diff --git a/llvm/test/MC/RISCV/rv32xqccmp-invalid.s b/llvm/test/MC/RISCV/rv32xqccmp-invalid.s
index ece3513120392..a955a33dc5a55 100644
--- a/llvm/test/MC/RISCV/rv32xqccmp-invalid.s
+++ b/llvm/test/MC/RISCV/rv32xqccmp-invalid.s
@@ -10,7 +10,7 @@ qc.cm.mvsa01 s0, s0
# CHECK-ERROR: :[[@LINE+1]]:14: error: invalid operand for instruction
qc.cm.mva01s a1, a2
-# CHECK-ERROR: :[[@LINE+1]]:15: error: invalid register list, {ra, s0-s10} or {x1, x8-x9, x18-x26} is not supported
+# CHECK-ERROR: :[[@LINE+1]]:15: error: invalid register list, '{ra, s0-s10}' or '{x1, x8-x9, x18-x26}' is not supported
qc.cm.popretz {ra, s0-s10}, 112
# CHECK-ERROR: :[[@LINE+1]]:28: error: stack adjustment for register list must be a multiple of 16 bytes in the range [16, 64]
diff --git a/llvm/test/MC/RISCV/rv32zcmp-invalid.s b/llvm/test/MC/RISCV/rv32zcmp-invalid.s
index b4261f865fae7..68aa7fc548f61 100644
--- a/llvm/test/MC/RISCV/rv32zcmp-invalid.s
+++ b/llvm/test/MC/RISCV/rv32zcmp-invalid.s
@@ -10,7 +10,7 @@ cm.mvsa01 s0, s0
# CHECK-ERROR: :[[@LINE+1]]:11: error: invalid operand for instruction
cm.mva01s a1, a2
-# CHECK-ERROR: :[[@LINE+1]]:12: error: invalid register list, {ra, s0-s10} or {x1, x8-x9, x18-x26} is not supported
+# CHECK-ERROR: :[[@LINE+1]]:12: error: invalid register list, '{ra, s0-s10}' or '{x1, x8-x9, x18-x26}' is not supported
cm.popretz {ra, s0-s10}, 112
# CHECK-ERROR: :[[@LINE+1]]:25: error: stack adjustment for register list must be a multiple of 16 bytes in the range [16, 64]
@@ -28,23 +28,26 @@ cm.push {ra}, -8
# CHECK-ERROR: :[[@LINE+1]]:9: error: register list must start from 'ra' or 'x1'
cm.pop {s0}, -40
-# CHECK-ERROR: :[[@LINE+1]]:13: error: continuous register list must start from 's0' or 'x8'
+# CHECK-ERROR: :[[@LINE+1]]:13: error: register must be 's0'
cm.pop {ra, t1}, -40
-# CHECK-ERROR: :[[@LINE+1]]:16: error: invalid register
+# CHECK-ERROR: :[[@LINE+1]]:16: error: register must be in the range 's1' to 's11'
cm.pop {ra, s0-t1}, -40
-# CHECK-ERROR: :[[@LINE+1]]:20: error: second contiguous registers pair of register list must start from 'x18'
-cm.pop {ra, x8-x9, x28}, -40
+# CHECK-ERROR: :[[@LINE+1]]:20: error: register must be 'x18'
+cm.pop {x1, x8-x9, x28}, -40
-# CHECK-ERROR: :[[@LINE+1]]:24: error: invalid register
-cm.pop {ra, x8-x9, x18-x28}, -40
+# CHECK-ERROR: :[[@LINE+1]]:24: error: register must be in the range 'x19' to 'x27'
+cm.pop {x1, x8-x9, x18-x28}, -40
-# CHECK-ERROR: :[[@LINE+1]]:24: error: invalid register
-cm.pop {ra, x8-x9, x18-x17}, -40
+# CHECK-ERROR: :[[@LINE+1]]:24: error: register must be in the range 'x19' to 'x27'
+cm.pop {x1, x8-x9, x18-x17}, -40
-# CHECK-ERROR: :[[@LINE+1]]:16: error: invalid register
-cm.pop {ra, x8-f8, x18-x17}, -40
+# CHECK-ERROR: :[[@LINE+1]]:16: error: register must be 'x9'
+cm.pop {x1, x8-f8, x18-x17}, -40
# CHECK-ERROR: :[[@LINE+1]]:9: error: operand must be {ra [, s0[-sN]]} or {x1 [, x8[-x9][, x18[-xN]]]}
cm.push x1, -16
+
+# CHECK-ERROR: :[[@LINE+1]]:16: error: register must be in the range 's1' to 's11'
+cm.pop {ra, s0-f8}, -40
diff --git a/llvm/test/MC/RISCV/rv64xqccmp-invalid.s b/llvm/test/MC/RISCV/rv64xqccmp-invalid.s
index 953887cc77e47..24ffc6b9fc02a 100644
--- a/llvm/test/MC/RISCV/rv64xqccmp-invalid.s
+++ b/llvm/test/MC/RISCV/rv64xqccmp-invalid.s
@@ -10,7 +10,7 @@ qc.cm.mvsa01 s0, s0
# CHECK-ERROR: :[[@LINE+1]]:14: error: invalid operand for instruction
qc.cm.mva01s a1, a2
-# CHECK-ERROR: :[[@LINE+1]]:15: error: invalid register list, {ra, s0-s10} or {x1, x8-x9, x18-x26} is not supported
+# CHECK-ERROR: :[[@LINE+1]]:15: error: invalid register list, '{ra, s0-s10}' or '{x1, x8-x9, x18-x26}' is not supported
qc.cm.popretz {ra, s0-s10}, 112
# CHECK-ERROR: :[[@LINE+1]]:28: error: stack adjustment for register list must be a multiple of 16 bytes in the range [32, 80]
diff --git a/llvm/test/MC/RISCV/rv64zcmp-invalid.s b/llvm/test/MC/RISCV/rv64zcmp-invalid.s
index c66415cb49b34..e806da81ea6d3 100644
--- a/llvm/test/MC/RISCV/rv64zcmp-invalid.s
+++ b/llvm/test/MC/RISCV/rv64zcmp-invalid.s
@@ -10,7 +10,7 @@ cm.mvsa01 s0, s0
# CHECK-ERROR: :[[@LINE+1]]:11: error: invalid operand for instruction
cm.mva01s a1, a2
-# CHECK-ERROR: :[[@LINE+1]]:12: error: invalid register list, {ra, s0-s10} or {x1, x8-x9, x18-x26} is not supported
+# CHECK-ERROR: :[[@LINE+1]]:12: error: invalid register list, '{ra, s0-s10}' or '{x1, x8-x9, x18-x26}' is not supported
cm.popretz {ra, s0-s10}, 112
# CHECK-ERROR: :[[@LINE+1]]:25: error: stack adjustment for register list must be a multiple of 16 bytes in the range [32, 80]
@@ -31,23 +31,23 @@ cm.pop {ra, s0-s1}, -33
# CHECK-ERROR: :[[@LINE+1]]:9: error: register list must start from 'ra' or 'x1'
cm.pop {s0}, -40
-# CHECK-ERROR: :[[@LINE+1]]:13: error: continuous register list must start from 's0' or 'x8'
+# CHECK-ERROR: :[[@LINE+1]]:13: error: register must be 's0'
cm.pop {ra, t1}, -40
-# CHECK-ERROR: :[[@LINE+1]]:16: error: invalid register
+# CHECK-ERROR: :[[@LINE+1]]:16: error: register must be in the range 's1' to 's11'
cm.pop {ra, s0-t1}, -40
-# CHECK-ERROR: :[[@LINE+1]]:20: error: second contiguous registers pair of register list must start from 'x18'
-cm.pop {ra, x8-x9, x28}, -40
+# CHECK-ERROR: :[[@LINE+1]]:20: error: register must be 'x18'
+cm.pop {x1, x8-x9, x28}, -40
-# CHECK-ERROR: :[[@LINE+1]]:24: error: invalid register
-cm.pop {ra, x8-x9, x18-x28}, -40
+# CHECK-ERROR: :[[@LINE+1]]:24: error: register must be in the range 'x19' to 'x27'
+cm.pop {x1, x8-x9, x18-x28}, -40
-# CHECK-ERROR: :[[@LINE+1]]:24: error: invalid register
-cm.pop {ra, x8-x9, x18-x17}, -40
+# CHECK-ERROR: :[[@LINE+1]]:24: error: register must be in the range 'x19' to 'x27'
+cm.pop {x1, x8-x9, x18-x17}, -40
-# CHECK-ERROR: :[[@LINE+1]]:16: error: invalid register
-cm.pop {ra, x8-f8, x18-x17}, -40
+# CHECK-ERROR: :[[@LINE+1]]:16: error: register must be 'x9'
+cm.pop {x1, x8-f8, x18-x17}, -40
# CHECK-ERROR: :[[@LINE+1]]:15: error: stack adjustment is invalid for this instruction and register list
cm.pop {ra}, -x1
@@ -55,5 +55,47 @@ cm.pop {ra}, -x1
# CHECK-ERROR: :[[@LINE+1]]:15: error: stack adjustment is invalid for this instruction and register list
cm.push {ra}, x1
-# CHECK-ERROR: :[[@LINE+1]]:12: error: register list must end with '}'
+# CHECK-ERROR: :[[@LINE+1]]:12: error: register 'x1' cannot start a multiple register range
cm.push {x1-x9}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:12: error: register 'ra' cannot start a multiple register range
+cm.push {ra-s0}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:13: error: register must be 'x8'
+cm.push {x1,s0}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:13: error: register must be 's0'
+cm.push {ra,x8}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:16: error: register must be 'x9'
+cm.push {x1,x8-s1}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:16: error: register must be in the range 's1' to 's11'
+cm.push {ra,s0-x9}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:16: error: register must be 'x9'
+cm.push {x1,x8-x18}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:19: error: register must be 'x18'
+cm.push {x1,x8-x9,s2}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:19: error: too many register ranges
+cm.push {ra,s0-s1,x18}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:19: error: too many register ranges
+cm.push {ra,s0-s1,s2}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:23: error: register must be in the range 'x19' to 'x27'
+cm.push {x1,x8-x9,x18-s3}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:27: error: too many register ranges
+cm.push {x1,x8-x9,x18-x19,x20}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:19: error: too many register ranges
+cm.push {ra,s0-s1,s3}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:18: error: expected ',' or '}'
+cm.push {ra,s0-s1-s2}, -32
+
+# CHECK-ERROR: :[[@LINE+1]]:16: error: expected ',' or '}'
+cm.push {ra, s0+s11}, -32
|
Collaborator
Author
|
Ping |
lenary
approved these changes
Apr 14, 2025
Member
lenary
left a comment
lenary
left a comment
There was a problem hiding this comment.
LGTM. Two minor nits, both about the same check, but in two different places.
Comment on lines
2586
to
2587
| if (getTok().isNot(AsmToken::Identifier) || | ||
| (RegEnd == RISCV::X18 && isRVE())) |
Member
There was a problem hiding this comment.
These two conditions seem separate, can they be split for clarity? I don't know if the RegEnd check should be sunk, maybe closer to new line 2595?
Collaborator
Author
There was a problem hiding this comment.
Turns out that check isn't needed (and I think it was supposed to be X9 not X18). The equivalent was in the code before this patch because we were using matchRegisterName instead of matchRegisterNameHelper for the x18- case to avoid allowing s2-. The RVE check only exists in matchRegisterNameHelper so we had to do an additional check.
With this patch we always use matchRegisterNameHelper and explicitly check the registers consistently start with `x' so we don't need to handle RVE specially.
lenary
approved these changes
Apr 14, 2025
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/166/builds/985 Here is the relevant piece of the build log for the reference |
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/16024 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Structure the code into a loop that parses a series of ranges and gives an error when there are too many ranges.
Give errors when ABI and non-ABI names are mixed.
Properly diagnose 'x1-` starting a list.
Use a default bool argument to merge parseRegListCommon and parseRegList.