[RISC-V][MC] Validate instructions prior to emission in AsmParser

[arichardson]

This ensures that broken AsmParser pseudo expansions trigger an error.
Noticed this while adding Y extension support where expanding pseudos
generated instructions that were failing predicates and/or using the
wrong register class, but since the encoded .o bit pattern as well as
the assembly output are the same for mode-dependent instructions, this
was not immediately obvious and only shows up with --show-inst.
Verify predicates and operand register classes prior to emission.

Ideally we would do this in a target-independent way, but for now this
only checks RISC-V outputs.

[llvmbot]

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

Author: Alexander Richardson (arichardson)

Changes

This ensures that broken AsmParser pseudo expansions trigger an error.
Noticed this while adding Y extension support where expanding pseudos
generated instructions that were failing predicates and/or using the
wrong register class, but since the encoded .o bit pattern as well as
the assmbly output are the same for mode-dependent instructions, this
was not immediately obvious and only shows up with --show-inst.
Verify predicates and operand register classes prior to emission.

Ideally we would do this in a target-independent way, but for now this
only checks RISC-V outputs.

---

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

1 Files Affected:

• (modified) llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp (+31)

diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 9bb3724c96c11..483d0a36966fe 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -128,6 +128,8 @@ class RISCVAsmParser : public MCTargetAsmParser {
   // Helper to actually emit an instruction to the MCStreamer. Also, when
   // possible, compression of the instruction is performed.
   void emitToStreamer(MCStreamer &S, const MCInst &Inst);
+  void validateInstructionPreEmit(const MCInst &Inst,
+                                  const MCSubtargetInfo &STI);
 
   // Helper to emit a combination of LUI, ADDI(W), and SLLI instructions that
   // synthesize the desired immediate value into the destination register.
@@ -3471,6 +3473,34 @@ bool RISCVAsmParser::parseDirectiveVariantCC() {
   return false;
 }
 
+void RISCVAsmParser::validateInstructionPreEmit(const MCInst &Inst, const MCSubtargetInfo &STI) {
+  // Ensure that we don't emit instructions with missing predicates or invalid
+  // register classes.
+  // TODO: ideally this code should be shared between targets.
+  RISCV_MC::verifyInstructionPredicates(Inst.getOpcode(), STI.getFeatureBits());
+  const MCRegisterInfo *MRI = getContext().getRegisterInfo();
+  unsigned HwMode = STI.getHwMode(MCSubtargetInfo::HwMode_RegInfo);
+  const MCInstrDesc &Desc = MII.get(Inst.getOpcode());
+  for (auto [I, Op] : enumerate(Inst.getOperands())) {
+    if (Op.isReg() && Op.getReg().isValid()) {
+      auto RCID = MII.getOpRegClassID(Desc.operands()[I], HwMode);
+      // Some instructions don't have a valid regclass, e.g. .insn_*
+      if (RCID == -1)
+        continue;
+      const MCRegisterClass &RegClass = MRI->getRegClass(RCID);
+      if (!RegClass.contains(Op.getReg())) {
+        getParser().printError(getLoc(), MII.getName(Inst.getOpcode()) + " operand " +
+                            Twine(I + 1) + " register " +
+                            MRI->getName(Op.getReg()) +
+                            " is not a member of register class " +
+                            MRI->getRegClassName(&RegClass));
+        reportFatalInternalError("Attempting to emit invalid instruction. "
+                                 "Incorrect pseudo expansion?");
+      }
+    }
+  }
+}
+
 void RISCVAsmParser::emitToStreamer(MCStreamer &S, const MCInst &Inst) {
   MCInst CInst;
   bool Res = false;
@@ -3479,6 +3509,7 @@ void RISCVAsmParser::emitToStreamer(MCStreamer &S, const MCInst &Inst) {
     Res = RISCVRVC::compress(CInst, Inst, STI);
   if (Res)
     ++RISCVNumInstrsCompressed;
+  validateInstructionPreEmit((Res ? CInst : Inst), STI);
   S.emitInstruction((Res ? CInst : Inst), STI);
 }
 

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[arichardson]

Found the following reg class mismatch: #171738 and has been very helpful for my RVY work.

[github-actions]

🐧 Linux x64 Test Results

• 187268 tests passed
• 4944 tests skipped

✅ The build succeeded and all tests passed.

[github-actions]

🪟 Windows x64 Test Results

• 128545 tests passed
• 2805 tests skipped

✅ The build succeeded and all tests passed.

[mshockwave]

I believe this should be stacked on top of #171738 ?

[arichardson]

I believe this should be stacked on top of #171738 ?

Yes I accidentally used spr diff --cherry-pick, but I've landed the dependency now so CI should pass.

[lenary]

IMO this should be behind a developer-oriented flag, like -verify-machineinstrs is for llc today.

The logic looks right, and yes sharing between targets would be good but can come later.

[arichardson]

IMO this should be behind a developer-oriented flag, like -verify-machineinstrs is for llc today.

The logic looks right, and yes sharing between targets would be good but can come later.

Thanks, that's a good suggestion. How about only doing it for assertions builds or EXPENSIVE_CHECKS? I haven't measured the performance impact here but since it's only for assembly inputs it shouldn't be on the critical path for most projects.

[lenary]

IMO this should be behind a developer-oriented flag, like -verify-machineinstrs is for llc today.
The logic looks right, and yes sharing between targets would be good but can come later.

Thanks, that's a good suggestion. How about only doing it for assertions builds or EXPENSIVE_CHECKS? I haven't measured the performance impact here but since it's only for assembly inputs it shouldn't be on the critical path for most projects.

I would have a command-line option that defaults to true when in EXPENSIVE_CHECKS, and otherwise is not enabled by default. This matches the -verify-machineinstrs logic, if I've read the right code.