[RISCV][NFC] Adopt DiagnosticString interface #126290
Merged
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
Where errors contain a constant string, TableGen allows us to specify the Diagnostic String with the rest of the Operand definition, which simplifies the C++ needed in the Asm Parser significantly.
I should be more sorry about this than I really am.
|
@llvm/pr-subscribers-backend-risc-v Author: Sam Elliott (lenary) ChangesWhere errors contain a constant string, TableGen allows us to specify the Diagnostic String with the rest of the Operand definition, which simplifies the C++ needed in the Asm Parser significantly. For our UImmN and SImmN operand classes, I have committed some TableGen crimes to automatically compute the message where it is easy to do so. Messages where the range is not just based on the (un)signed max I've avoided removing the custom messages from, on the basis that the logic for these is complex beyond what I think is reasonable to do in TableGen. I kept the commits separate in case we just want to drop the second one. The first one has avoided TableGen crimes. Full diff: https://github.com/llvm/llvm-project/pull/126290.diff 7 Files Affected:
diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index c51c4201ebd18ca..a2e9237b0e813d6 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -280,7 +280,7 @@ class RISCVAsmParser : public MCTargetAsmParser {
std::unique_ptr<RISCVOperand> defaultFRMArgLegacyOp() const;
public:
- enum RISCVMatchResultTy {
+ enum RISCVMatchResultTy : unsigned {
Match_Dummy = FIRST_TARGET_MATCH_RESULT_TY,
#define GET_OPERAND_DIAGNOSTIC_TYPES
#include "RISCVGenAsmMatcher.inc"
@@ -1524,10 +1524,6 @@ bool RISCVAsmParser::matchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
std::numeric_limits<uint32_t>::max(),
"operand either must be a bare symbol name or an immediate integer in "
"the range");
- case Match_InvalidImmZero: {
- SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
- return Error(ErrorLoc, "immediate must be zero");
- }
case Match_InvalidUImmLog2XLen:
if (isRV64())
return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 6) - 1);
@@ -1536,45 +1532,15 @@ bool RISCVAsmParser::matchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
if (isRV64())
return generateImmOutOfRangeError(Operands, ErrorInfo, 1, (1 << 6) - 1);
return generateImmOutOfRangeError(Operands, ErrorInfo, 1, (1 << 5) - 1);
- case Match_InvalidUImm1:
- return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 1) - 1);
- case Match_InvalidUImm2:
- return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 2) - 1);
case Match_InvalidUImm2Lsb0:
return generateImmOutOfRangeError(Operands, ErrorInfo, 0, 2,
"immediate must be one of");
- case Match_InvalidUImm3:
- return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 3) - 1);
- case Match_InvalidUImm4:
- return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 4) - 1);
- case Match_InvalidUImm5:
- return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 5) - 1);
case Match_InvalidUImm5NonZero:
return generateImmOutOfRangeError(Operands, ErrorInfo, 1, (1 << 5) - 1);
case Match_InvalidUImm5GT3:
return generateImmOutOfRangeError(Operands, ErrorInfo, 4, (1 << 5) - 1);
- case Match_InvalidUImm6:
- return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 6) - 1);
- case Match_InvalidUImm7:
- return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 7) - 1);
- case Match_InvalidUImm8:
- return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 8) - 1);
case Match_InvalidUImm8GE32:
return generateImmOutOfRangeError(Operands, ErrorInfo, 32, (1 << 8) - 1);
- case Match_InvalidSImm5:
- return generateImmOutOfRangeError(Operands, ErrorInfo, -(1 << 4),
- (1 << 4) - 1);
- case Match_InvalidSImm6:
- return generateImmOutOfRangeError(Operands, ErrorInfo, -(1 << 5),
- (1 << 5) - 1);
- case Match_InvalidSImm6NonZero:
- return generateImmOutOfRangeError(
- Operands, ErrorInfo, -(1 << 5), (1 << 5) - 1,
- "immediate must be non-zero in the range");
- case Match_InvalidCLUIImm:
- return generateImmOutOfRangeError(
- Operands, ErrorInfo, 1, (1 << 5) - 1,
- "immediate must be in [0xfffe0, 0xfffff] or");
case Match_InvalidUImm5Lsb0:
return generateImmOutOfRangeError(
Operands, ErrorInfo, 0, (1 << 5) - 2,
@@ -1611,10 +1577,6 @@ bool RISCVAsmParser::matchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
return generateImmOutOfRangeError(
Operands, ErrorInfo, -(1 << 9), (1 << 9) - 16,
"immediate must be a multiple of 16 bytes and non-zero in the range");
- case Match_InvalidUImm10:
- return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 10) - 1);
- case Match_InvalidUImm11:
- return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 11) - 1);
case Match_InvalidSImm12:
return generateImmOutOfRangeError(
Operands, ErrorInfo, -(1 << 11), (1 << 11) - 1,
@@ -1637,8 +1599,6 @@ bool RISCVAsmParser::matchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
"operand must be a symbol with "
"%hi/%tprel_hi modifier or an integer in "
"the range");
- case Match_InvalidUImm20:
- return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 20) - 1);
case Match_InvalidUImm20AUIPC:
return generateImmOutOfRangeError(
Operands, ErrorInfo, 0, (1 << 20) - 1,
@@ -1649,80 +1609,23 @@ bool RISCVAsmParser::matchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
return generateImmOutOfRangeError(
Operands, ErrorInfo, -(1 << 20), (1 << 20) - 2,
"immediate must be a multiple of 2 bytes in the range");
- case Match_InvalidCSRSystemRegister: {
- return generateImmOutOfRangeError(Operands, ErrorInfo, 0, (1 << 12) - 1,
- "operand must be a valid system register "
- "name or an integer in the range");
- }
- case Match_InvalidLoadFPImm: {
- SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
- return Error(ErrorLoc, "operand must be a valid floating-point constant");
- }
- case Match_InvalidBareSymbol: {
- SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
- return Error(ErrorLoc, "operand must be a bare symbol name");
- }
- case Match_InvalidPseudoJumpSymbol: {
- SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
- return Error(ErrorLoc, "operand must be a valid jump target");
- }
- case Match_InvalidCallSymbol: {
- SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
- return Error(ErrorLoc, "operand must be a bare symbol name");
- }
- case Match_InvalidTPRelAddSymbol: {
- SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
- return Error(ErrorLoc, "operand must be a symbol with %tprel_add modifier");
- }
- case Match_InvalidTLSDESCCallSymbol: {
- SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
- return Error(ErrorLoc,
- "operand must be a symbol with %tlsdesc_call modifier");
- }
- case Match_InvalidRTZArg: {
- SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
- return Error(ErrorLoc, "operand must be 'rtz' floating-point rounding mode");
- }
case Match_InvalidVTypeI: {
SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
return generateVTypeError(ErrorLoc);
}
- case Match_InvalidVMaskRegister: {
- SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
- return Error(ErrorLoc, "operand must be v0.t");
- }
- case Match_InvalidVMaskCarryInRegister: {
- SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
- return Error(ErrorLoc, "operand must be v0");
- }
case Match_InvalidSImm5Plus1: {
return generateImmOutOfRangeError(Operands, ErrorInfo, -(1 << 4) + 1,
(1 << 4),
"immediate must be in the range");
}
- case Match_InvalidSImm26:
- return generateImmOutOfRangeError(Operands, ErrorInfo, -(1 << 25),
- (1 << 25) - 1);
- case Match_InvalidRlist: {
- SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
- return Error(
- ErrorLoc,
- "operand must be {ra [, s0[-sN]]} or {x1 [, x8[-x9][, x18[-xN]]]}");
- }
- case Match_InvalidStackAdj: {
- SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
- return Error(
- ErrorLoc,
- "stack adjustment is invalid for this instruction and register list; "
- "refer to Zc spec for a detailed range of stack adjustment");
- }
case Match_InvalidRnumArg: {
return generateImmOutOfRangeError(Operands, ErrorInfo, 0, 10);
}
- case Match_InvalidRegReg: {
- SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
- return Error(ErrorLoc, "operands must be register and register");
}
+
+ if (const char* MatchDiag = getMatchKindDiag((RISCVMatchResultTy)Result)) {
+ SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
+ return Error(ErrorLoc, MatchDiag);
}
llvm_unreachable("Unknown match type detected!");
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.td b/llvm/lib/Target/RISCV/RISCVInstrInfo.td
index 54fee1ac3130e1b..fc07f815945fd81 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.td
@@ -125,6 +125,7 @@ def ImmZeroAsmOperand : AsmOperandClass {
let Name = "ImmZero";
let RenderMethod = "addImmOperands";
let DiagnosticType = !strconcat("Invalid", Name);
+ let DiagnosticString = "immediate must be zero";
}
// A parse method for (${gpr}) or 0(${gpr}), where the 0 is be silently ignored.
@@ -150,12 +151,26 @@ def SPMem : MemOperand<SP>;
def GPRCMem : MemOperand<GPRC>;
-class SImmAsmOperand<int width, string suffix = "">
+class RISCVRangedImmDiagnosticString<int lo, int hi, string prefix = "immediate must be an integer in the range"> {
+ string Message = prefix # " [" # lo # ", " # hi # "]";
+}
+
+class RISCVSImmDiagnosticString<int bitsNum, string prefix = "immediate must be an integer in the range"> :
+ RISCVRangedImmDiagnosticString<!sub(0, !shl(1, !sub(bitsNum, 1))), !sub(!shl(1, !sub(bitsNum, 1)), 1), prefix> {
+}
+
+class RISCVUImmDiagnosticString<int bitsNum, string prefix = "immediate must be an integer in the range"> :
+ RISCVRangedImmDiagnosticString<0, !sub(!shl(1, bitsNum), 1), prefix> {
+}
+
+class SImmAsmOperand<int width, string suffix = "", string messagePrefix = "immediate must be an integer in the range">
: ImmAsmOperand<"S", width, suffix> {
+ let DiagnosticString = RISCVSImmDiagnosticString<width, messagePrefix>.Message;
}
-class UImmAsmOperand<int width, string suffix = "">
+class UImmAsmOperand<int width, string suffix = "", string messagePrefix = "immediate must be an integer in the range">
: ImmAsmOperand<"U", width, suffix> {
+ let DiagnosticString = RISCVUImmDiagnosticString<width, messagePrefix>.Message;
}
class RISCVOp<ValueType vt = XLenVT> : Operand<vt> {
@@ -323,6 +338,7 @@ def BareSymbol : AsmOperandClass {
let Name = "BareSymbol";
let RenderMethod = "addImmOperands";
let DiagnosticType = "InvalidBareSymbol";
+ let DiagnosticString = "operand must be a bare symbol name";
let ParserMethod = "parseBareSymbol";
}
@@ -335,6 +351,7 @@ def CallSymbol : AsmOperandClass {
let Name = "CallSymbol";
let RenderMethod = "addImmOperands";
let DiagnosticType = "InvalidCallSymbol";
+ let DiagnosticString = "operand must be a bare symbol name";
let ParserMethod = "parseCallSymbol";
}
@@ -347,6 +364,7 @@ def PseudoJumpSymbol : AsmOperandClass {
let Name = "PseudoJumpSymbol";
let RenderMethod = "addImmOperands";
let DiagnosticType = "InvalidPseudoJumpSymbol";
+ let DiagnosticString = "operand must be a valid jump target";
let ParserMethod = "parsePseudoJumpSymbol";
}
@@ -359,6 +377,7 @@ def TPRelAddSymbol : AsmOperandClass {
let Name = "TPRelAddSymbol";
let RenderMethod = "addImmOperands";
let DiagnosticType = "InvalidTPRelAddSymbol";
+ let DiagnosticString = "operand must be a symbol with %tprel_add modifier";
let ParserMethod = "parseOperandWithModifier";
}
@@ -371,6 +390,8 @@ def CSRSystemRegister : AsmOperandClass {
let Name = "CSRSystemRegister";
let ParserMethod = "parseCSRSystemRegister";
let DiagnosticType = "InvalidCSRSystemRegister";
+ let DiagnosticString = RISCVUImmDiagnosticString<12, "operand must be a valid system register "
+ "name or an integer in the range">.Message;
}
def csr_sysreg : RISCVOp, TImmLeaf<XLenVT, "return isUInt<12>(Imm);"> {
@@ -1779,6 +1800,7 @@ def TLSDESCCallSymbol : AsmOperandClass {
let Name = "TLSDESCCallSymbol";
let RenderMethod = "addImmOperands";
let DiagnosticType = "InvalidTLSDESCCallSymbol";
+ let DiagnosticString = "operand must be a symbol with %tlsdesc_call modifier";
let ParserMethod = "parseOperandWithModifier";
}
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoC.td b/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
index 0f320d2375ec253..dd3e93a54df72f9 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoC.td
@@ -47,7 +47,7 @@ def simm6 : RISCVSImmLeafOp<6> {
def simm6nonzero : RISCVOp,
ImmLeaf<XLenVT, [{return (Imm != 0) && isInt<6>(Imm);}]> {
- let ParserMatchClass = SImmAsmOperand<6, "NonZero">;
+ let ParserMatchClass = SImmAsmOperand<6, "NonZero", "immediate must be non-zero in the range">;
let EncoderMethod = "getImmOpValue";
let DecoderMethod = "decodeSImmNonZeroOperand<6>";
let OperandType = "OPERAND_SIMM6_NONZERO";
@@ -69,6 +69,7 @@ def CLUIImmAsmOperand : AsmOperandClass {
let Name = "CLUIImm";
let RenderMethod = "addImmOperands";
let DiagnosticType = !strconcat("Invalid", Name);
+ let DiagnosticString = "immediate must be in [0xfffe0, 0xfffff] or [1, 31]";
}
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoV.td b/llvm/lib/Target/RISCV/RISCVInstrInfoV.td
index 671e493fb3763ac..fdb2334b131da04 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoV.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoV.td
@@ -48,6 +48,7 @@ def VMaskAsmOperand : AsmOperandClass {
let IsOptional = 1;
let DefaultMethod = "defaultMaskRegOp";
let DiagnosticType = "InvalidVMaskRegister";
+ let DiagnosticString = "operand must be v0.t";
}
def VMaskCarryInAsmOperand : AsmOperandClass {
@@ -55,6 +56,7 @@ def VMaskCarryInAsmOperand : AsmOperandClass {
let RenderMethod = "addRegOperands";
let PredicateMethod = "isV0Reg";
let DiagnosticType = "InvalidVMaskCarryInRegister";
+ let DiagnosticString = "operand must be v0";
}
def VMaskOp : RegisterOperand<VMV0> {
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td b/llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td
index b98934d8c639646..bade4863ad348bb 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoXCV.td
@@ -514,6 +514,7 @@ def CVrrAsmOperand : AsmOperandClass {
let Name = "RegReg";
let ParserMethod = "parseRegReg";
let DiagnosticType = "InvalidRegReg";
+ let DiagnosticString = "operands must be register and register";
}
def CVrr : Operand<i32>,
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
index 5cc16765d4ae2aa..9dfbcf678d6ebab 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
@@ -39,12 +39,15 @@ def RlistAsmOperand : AsmOperandClass {
let Name = "Rlist";
let ParserMethod = "parseReglist";
let DiagnosticType = "InvalidRlist";
+ let DiagnosticString = "operand must be {ra [, s0[-sN]]} or {x1 [, x8[-x9][, x18[-xN]]]}";
}
def StackAdjAsmOperand : AsmOperandClass {
let Name = "StackAdj";
let ParserMethod = "parseZcmpStackAdj";
let DiagnosticType = "InvalidStackAdj";
+ let DiagnosticString = "stack adjustment is invalid for this instruction and register list; "
+ "refer to Zc spec for a detailed range of stack adjustment";
let PredicateMethod = "isSpimm";
let RenderMethod = "addSpimmOperands";
}
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td
index ab54b45f4de9321..a539ca82b7462f6 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZfa.td
@@ -30,6 +30,7 @@ def LoadFPImmOperand : AsmOperandClass {
let ParserMethod = "parseFPImm";
let RenderMethod = "addFPImmOperands";
let DiagnosticType = "InvalidLoadFPImm";
+ let DiagnosticString = "operand must be a valid floating-point constant";
}
def loadfpimm : Operand<XLenVT> {
@@ -43,6 +44,7 @@ def RTZArg : AsmOperandClass {
let Name = "RTZArg";
let RenderMethod = "addFRMArgOperands";
let DiagnosticType = "InvalidRTZArg";
+ let DiagnosticString = "operand must be 'rtz' floating-point rounding mode";
let ParserMethod = "parseFRMArg";
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
topperc
reviewed
Feb 7, 2025
| @@ -150,12 +151,26 @@ def SPMem : MemOperand<SP>; | |||
|
|
|||
| def GPRCMem : MemOperand<GPRC>; | |||
|
|
|||
There was a problem hiding this comment.
I guess this makes a bunch of similar strings in the final binary that can no longer be shared?
There was a problem hiding this comment.
That's my expectation, yes. TableGen crimes have consequences, it turns out.
There was a problem hiding this comment.
I wonder how much work it be to introduce "DiagnosticCode" to allow us to programmatically generate the string like we do now
There was a problem hiding this comment.
I thought these (materialized) strings from TableGen code will eventually be put as normal C strings, so in the final compiler binaries similar string will still be deduplicated. Though these strings wouldn't be deduplicated in the *.inc files generated from TableGen I believe (though I think we can somehow solve this by changing how TableGen generates this part of the code)
There was a problem hiding this comment.
@mshockwave the range strings will all have a similar beginning and end but the lower and upper bounds are part of the string and will be different values. Without tablegen, the prefix and suffix are separate strings and the lower and upper bounds are converted to strings at runtime.
There was a problem hiding this comment.
range strings will all have a similar beginning and end
oh right, I thought we're only talking about fixed strings.
There was a problem hiding this comment.
I wonder how much work it be to introduce "DiagnosticCode" to allow us to programmatically generate the string like we do now
This feels like overenginnering. I'm like down three side-rabbit-holes now, and going to retreat a bit.
I will undo the tablegen crimes, the C++ is better for craig's reasons, but where we have fixed strings this should make things easier.
This reverts commit 70498da.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/14326 Here is the relevant piece of the build log for the reference |
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Where errors contain a constant string, TableGen allows us to specify the Diagnostic String with the rest of the Operand definition, which simplifies the C++ needed in the Asm Parser significantly.