[RISCV] Improve Errors for X1/X5/X1X5 Reg Classes

[lenary]

LLVM has functionality for producing a register-class-specific error message in the assembly parser, rather than just emitting the generic "invalid operand for instruction" error.

This starts the gradual adoption of this functionality for RISC-V, with some lesser-used shadow-stack register classes:

• GPRX1 (only contains ra)
• GPRX5 (only contains t0)
• GPRX1X5 (only contains ra and t0)

LLVM is reasonably conservative about when these errors are used, in particular you have to have all the features for the relevant mnemonic enabled before it will do, hence the test updates.

I also merged a pair of almost identical rv32/rv64 test files into a single file with one run line.

[llvmbot]

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-risc-v

Author: Sam Elliott (lenary)

Changes

LLVM has functionality for producing a register-class-specific error message in the assembly parser, rather than just emitting the generic "invalid operand for instruction" error.

This starts the gradual adoption of this functionality for RISC-V, with some lesser-used shadow-stack register classes:

• GPRX1 (only contains ra)
• GPRX5 (only contains t0)
• GPRX1X5 (only contains ra and t0)

LLVM is reasonably conservative about when these errors are used, in particular you have to have all the features for the relevant mnemonic enabled before it will do, hence the test updates.

I also merged a pair of almost identical rv32/rv64 test files into a single file with one run line.

---

Full diff: https://github.com/llvm/llvm-project/pull/126184.diff

5 Files Affected:

• (modified) llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp (+12)
• (modified) llvm/lib/Target/RISCV/RISCVRegisterInfo.td (+5)
• (removed) llvm/test/MC/RISCV/rv32zicfiss-invalid.s (-17)
• (removed) llvm/test/MC/RISCV/rv64zicfiss-invalid.s (-17)
• (added) llvm/test/MC/RISCV/zicfiss-invalid.s (+19)

diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index c51c4201ebd18ca..70ad8fba3db4a81 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -1723,6 +1723,18 @@ bool RISCVAsmParser::matchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
     SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
     return Error(ErrorLoc, "operands must be register and register");
   }
+  case Match_InvalidRegClassGPRX1: {
+    SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
+    return Error(ErrorLoc, "register must be ra (x1)");
+  }
+  case Match_InvalidRegClassGPRX5: {
+    SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
+    return Error(ErrorLoc, "register must be t0 (x5)");
+  }
+  case Match_InvalidRegClassGPRX1X5: {
+    SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
+    return Error(ErrorLoc, "register must be ra or t0 (x1 or x5)");
+  }
   }
 
   llvm_unreachable("Unknown match type detected!");
diff --git a/llvm/lib/Target/RISCV/RISCVRegisterInfo.td b/llvm/lib/Target/RISCV/RISCVRegisterInfo.td
index 7eb93973459c0d3..26f688d6af92398 100644
--- a/llvm/lib/Target/RISCV/RISCVRegisterInfo.td
+++ b/llvm/lib/Target/RISCV/RISCVRegisterInfo.td
@@ -247,7 +247,11 @@ def GPR : GPRRegisterClass<(add (sequence "X%u", 10, 17),
                                 (sequence "X%u", 0, 4))>;
 
 def GPRX0 : GPRRegisterClass<(add X0)>;
+
+let DiagnosticType = "InvalidRegClassGPRX1" in
 def GPRX1 : GPRRegisterClass<(add X1)>;
+
+let DiagnosticType = "InvalidRegClassGPRX5" in
 def GPRX5 : GPRRegisterClass<(add X5)>;
 
 def GPRNoX0 : GPRRegisterClass<(sub GPR, X0)>;
@@ -282,6 +286,7 @@ def SP : GPRRegisterClass<(add X2)>;
 def SR07 : GPRRegisterClass<(add (sequence "X%u", 8, 9),
                                  (sequence "X%u", 18, 23))>;
 
+let DiagnosticType = "InvalidRegClassGPRX1X5" in
 def GPRX1X5 :  GPRRegisterClass<(add X1, X5)>;
 
 //===----------------------------------------------------------------------===//
diff --git a/llvm/test/MC/RISCV/rv32zicfiss-invalid.s b/llvm/test/MC/RISCV/rv32zicfiss-invalid.s
deleted file mode 100644
index 048df67e8a6461b..000000000000000
--- a/llvm/test/MC/RISCV/rv32zicfiss-invalid.s
+++ /dev/null
@@ -1,17 +0,0 @@
-# RUN: not llvm-mc %s -triple=riscv32 -mattr=+experimental-zicfiss,+c -M no-aliases -show-encoding \
-# RUN:     2>&1 | FileCheck -check-prefixes=CHECK-ERR %s
-
-# CHECK-ERR: error: invalid operand for instruction
-sspopchk a1
-
-# CHECK-ERR: error: invalid operand for instruction
-c.sspush t0
-
-# CHECK-ERR: error: invalid operand for instruction
-c.sspopchk ra
-
-# CHECK-ERR: error: invalid operand for instruction
-sspush a0
-
-# CHECK-ERR: error: invalid operand for instruction
-ssrdp zero
diff --git a/llvm/test/MC/RISCV/rv64zicfiss-invalid.s b/llvm/test/MC/RISCV/rv64zicfiss-invalid.s
deleted file mode 100644
index fc69c68a477d6cc..000000000000000
--- a/llvm/test/MC/RISCV/rv64zicfiss-invalid.s
+++ /dev/null
@@ -1,17 +0,0 @@
-# RUN: not llvm-mc %s -triple=riscv64 -mattr=+experimental-zicfiss,+c -M no-aliases -show-encoding \
-# RUN:     2>&1 | FileCheck -check-prefixes=CHECK-ERR %s
-
-# CHECK-ERR: error: invalid operand for instruction
-sspopchk a1
-
-# CHECK-ERR: error: invalid operand for instruction
-c.sspush t0
-
-# CHECK-ERR: error: invalid operand for instruction
-c.sspopchk ra
-
-# CHECK-ERR: error: invalid operand for instruction
-sspush a0
-
-# CHECK-ERR: error: invalid operand for instruction
-ssrdp zero
diff --git a/llvm/test/MC/RISCV/zicfiss-invalid.s b/llvm/test/MC/RISCV/zicfiss-invalid.s
new file mode 100644
index 000000000000000..a5ab9240f3faddd
--- /dev/null
+++ b/llvm/test/MC/RISCV/zicfiss-invalid.s
@@ -0,0 +1,19 @@
+# RUN: not llvm-mc %s -triple=riscv32 -mattr=+experimental-zicfiss,+zcmop,+c -M no-aliases -show-encoding \
+# RUN:     2>&1 | FileCheck -check-prefixes=CHECK-ERR %s
+# RUN: not llvm-mc %s -triple=riscv64 -mattr=+experimental-zicfiss,+zcmop,+c -M no-aliases -show-encoding \
+# RUN:     2>&1 | FileCheck -check-prefixes=CHECK-ERR %s
+
+# CHECK-ERR: error: register must be ra or t0 (x1 or x5)
+sspopchk a1
+
+# CHECK-ERR: error: register must be ra (x1)
+c.sspush t0
+
+# CHECK-ERR: error: register must be t0 (x5)
+c.sspopchk ra
+
+# CHECK-ERR: error: register must be ra or t0 (x1 or x5)
+sspush a0
+
+# CHECK-ERR: error: invalid operand for instruction
+ssrdp zero

[wangpc-pp]

This can be done automatically:

class GPRRegisterClassWithDiagnostic<dag regList>: GPRRegisterClass<regList> {
  let DiagnosticType= "InvalidRegClass" # NAME;
}
def GPRX1 : GPRRegisterClassWithDiagnostic<(add X1)>;

And finally we can remove GPRRegisterClassWithDiagnostic and move override of DiagnosticType to a base class.

[wangpc-pp]

And also, why can't we just override DiagnosticString so that we don't even need to handle it in C++ code:

  // A diagnostic message to emit when an invalid value is provided for this
  // register class when it is being used as an assembly operand. If this is
  // non-empty, an anonymous diagnostic type enum value will be generated, and
  // the assembly matcher will provide a function to map from diagnostic types
  // to message strings.
  string DiagnosticString = "";

The DiagnosticString can be an auto-generated string from the list regList.

[lenary]

In response to the first comment:

I was thinking of adding a let DiagnosticType= "InvalidRegClass" # NAME; in RISCVRegisterClass, but didn't want to change a lot of error messages at the same time. I hadn't worked out your suggestion, but I think that only works for GPRRegisterClass, and I want to do all RISCVRegisterClasses.

I do want to get to the point where RISCVRegisterClass has let DiagnosticType= "InvalidRegClass" # NAME; but that's a long way off. I'm also not sure what to do with things like GPRAll which shouldn't ever be used in the AsmParser.

As for the two points in the second comment:

• DiagnosticString needs more hooking up in C++, but I agree we might want to head that direction. It generates a getMatchKindDiag function which returns the string, which then needs a bit more processing to get the location and emit it (the Arm backend is the only one to use this mechanism upstream).

• I don't think we'll get good error messages if we auto-generate strings from the included regList, I think it's better to explain them as groups, rather than just listing (up to) 32 registers that are allowed there. These three are small and there's no specific semantic group like there is with e.g. GPRNoX0.

I'll look at moving some of the existing diagnostics into a DiagnosticString, to see how that goes (as we also have the same thing on AsmOperandClass). I think that can go in parallel to this, maybe?

[lenary]

#126290 is a (NFC) PR to adopt DiagnosticString. I will rebase this change on that if/when it lands.

[topperc]

LGTM

[wangpc-pp]

LGTM.