-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[TableGen] Report a better error when an InstAlias does not use a RegClass #168444
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
base: main
Are you sure you want to change the base?
Conversation
Created using spr 1.3.8-beta.1
|
@llvm/pr-subscribers-tablegen Author: Alexander Richardson (arichardson) ChangesAlso fix the missing space in the error message. I notice while changing Full diff: https://github.com/llvm/llvm-project/pull/168444.diff 1 Files Affected:
diff --git a/llvm/utils/TableGen/Common/CodeGenInstAlias.cpp b/llvm/utils/TableGen/Common/CodeGenInstAlias.cpp
index 6c1a3e9977c28..2de27986fa04a 100644
--- a/llvm/utils/TableGen/Common/CodeGenInstAlias.cpp
+++ b/llvm/utils/TableGen/Common/CodeGenInstAlias.cpp
@@ -55,9 +55,10 @@ static Expected<ResultOperand> matchSimpleOperand(const Init *Arg,
// Match 'RegClass:$name' or 'RegOp:$name'.
if (const Record *ArgRC = T.getInitValueAsRegClassLike(Arg)) {
if (ArgRC->isSubClassOf("RegisterClass")) {
- if (!T.getRegisterClass(OpRC).hasSubClass(&T.getRegisterClass(ArgRC)))
+ if (!OpRC->isSubClassOf("RegisterClass") ||
+ !T.getRegisterClass(OpRC).hasSubClass(&T.getRegisterClass(ArgRC)))
return createStringError(
- "argument register class" + ArgRC->getName() +
+ "argument register class " + ArgRC->getName() +
" is not a subclass of operand register class " +
OpRC->getName());
if (!ArgName)
|
| return createStringError( | ||
| "argument register class" + ArgRC->getName() + | ||
| "argument register class " + ArgRC->getName() + |
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.
Considering the typo wasn't noticed there does not seem to be a test for this error case. If there's an existing test I can add this to easily I can do so, otherwise I'll have to try create a minimal testcase from scratch.
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.
There are no tests for this, as far as I remember.
| @@ -55,9 +55,10 @@ static Expected<ResultOperand> matchSimpleOperand(const Init *Arg, | |||
| // Match 'RegClass:$name' or 'RegOp:$name'. | |||
| if (const Record *ArgRC = T.getInitValueAsRegClassLike(Arg)) { | |||
| if (ArgRC->isSubClassOf("RegisterClass")) { | |||
| if (!T.getRegisterClass(OpRC).hasSubClass(&T.getRegisterClass(ArgRC))) | |||
| if (!OpRC->isSubClassOf("RegisterClass") || | |||
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.
Maybe instead if (OpRC->isSubclassOf("RegClassByHwMode")) then report "HwMode support is not implemented"?
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.
Yeah that would give slightly better results but may be too restrictive. It does seem to work with HWModes as long as the RegClassByHwMode argument is the same, but it doesn't handle the subset case.
🐧 Linux x64 Test Results
|
Also fix the missing space in the error message. I notice while changing
RISC-V's loads and stores to use RegClassByHwMode and got a non-descriptive
error from
T.getRegisterClass(OpRC)when parsing the InstAliases.