[RISCV] Add MachineInstr immediate verification

[luismarques]

Summary

This patch implements the TargetInstrInfo::verifyInstruction hook for RISC-V. Currently the hook verifies the machine instruction's immediate operands, to check if the immediates are within the expected bounds. Without the hook invalid immediates are not detected except when doing assembly parsing, so they are silently emitted (including being truncated when emitting object code).

The bounds information is specified in tablegen by using the OperandType definition, which sets the MCOperandInfo's OperandType field. Several RISC-V-specific immediate operand types were created, which extend the MCInstrDesc's OperandType enum.

To have the hook called with llc pass it the -verify-machineinstrs option. For Clang add the cmake build config -DLLVM_ENABLE_EXPENSIVE_CHECKS=True, or temporarily patch TargetPassConfig::addVerifyPass.

Review concerns:

• The patch adds immediate operand type checks that cover at least the base ISA. There are several other operand types for the C extension and one type for the F/D extensions that were left out of this initial patch because they introduced further design concerns that I felt were best evaluated separately.

• Invalid register classes (e.g. passing a GPR register where a GPRC is expected) are already caught, so were not included.

• This design makes the more abstract MachineInstr verification depend on MC layer definitions, which arguably is not the cleanest design, but is in line with how things are done in other parts of the target and LLVM in general.

• There is some duplication of logic already present in the MCOperandPredicates. Since the MachineInstr and MCInstr notions of immediates are fundamentally different, this is currently necessary.

[lenary]

Is there a way to write a test for this? I realise any assembly goes through the parser, so will be caught before it hits this code. Is there another way of making this work? a MIR-based test?

The implementation looks good to me. I like that we haven't had to make any target-independent changes, and the implementation is fairly small.

[luismarques]

In D67397#1664695, @lenary wrote:

Is there a way to write a test for this? I realise any assembly goes through the parser, so will be caught before it hits this code. Is there another way of making this work? a MIR-based test?

I tested this by temporarily introducing invalid immediates in the existing BuildMIs, since I forgot to check if a MIR test would work. I'll check that option, thanks!

[luismarques]

Adds a MIR test. Changes the operand enum to a new namespace.

[jrtc27]

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

497

Combining foreach and an index like this is a bit ugly due to having the counter increment at the end (and it's brittle if someone introduces a continue into the loop for whatever reason). AMDGPU takes the alternate approach of for (int i = 0, e = Desc.getNumOperands(); i != e; ++i); then, the only change you need to the body is unsigned OpType = Desc.OpInfo[i].OperandType. Alternatively you can keep track of the start, current and end iterators, calculating the index by subtracting the current from the start, or just expand out the foreach and include i in the for loop itself.

[lenary]

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

497

There's an enumerate function in STLExtras.h that will give you back a pair of the index and the object in the iterator. I think that would probably be even cleaner. (See comment at about line 1485 in STLExtras.h)

[luismarques]

Rebase and address iteration issue (hat tip to both commenters!).

[lenary]

LGTM. Good changes, thanks!

llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp

 //===----------------------------------------------------------------------===//
 
 #include "RISCVMCTargetDesc.h"
 #include "RISCVELFStreamer.h"
 #include "RISCVInstPrinter.h"
 #include "RISCVMCAsmInfo.h"
 #include "RISCVTargetStreamer.h"
 #include "TargetInfo/RISCVTargetInfo.h"
+#include "Utils/RISCVBaseInfo.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/CodeGen/Register.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCRegisterInfo.h"
 #include "llvm/MC/MCStreamer.h"
 #include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Support/ErrorHandling.h"

llvm/lib/Target/RISCV/RISCVInstrInfo.h

 #include "RISCVRegisterInfo.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 
 #define GET_INSTRINFO_HEADER
 #include "RISCVGenInstrInfo.inc"
 
 namespace llvm {
 
+class RISCVSubtarget;
+
 class RISCVInstrInfo : public RISCVGenInstrInfo {
 
 public:
-  RISCVInstrInfo();
+  explicit RISCVInstrInfo(RISCVSubtarget &STI);
 
   unsigned isLoadFromStackSlot(const MachineInstr &MI,
                                int &FrameIndex) const override;
   unsigned isStoreToStackSlot(const MachineInstr &MI,
                               int &FrameIndex) const override;
 
   void copyPhysReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
                    const DebugLoc &DL, unsigned DstReg, unsigned SrcReg,
public:
   reverseBranchCondition(SmallVectorImpl<MachineOperand> &Cond) const override;
 
   MachineBasicBlock *getBranchDestBlock(const MachineInstr &MI) const override;
 
   bool isBranchOffsetInRange(unsigned BranchOpc,
                              int64_t BrOffset) const override;
 
   bool isAsCheapAsAMove(const MachineInstr &MI) const override;
+
+  bool verifyInstruction(const MachineInstr &MI,
+                         StringRef &ErrInfo) const override;
+
+protected:
+  const RISCVSubtarget &STI;
 };
 }
 #endif

llvm/lib/Target/RISCV/RISCVInstrInfo.cpp

 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TargetRegistry.h"
 
 #define GET_INSTRINFO_CTOR_DTOR
 #include "RISCVGenInstrInfo.inc"
 
 using namespace llvm;
 
-RISCVInstrInfo::RISCVInstrInfo()
-    : RISCVGenInstrInfo(RISCV::ADJCALLSTACKDOWN, RISCV::ADJCALLSTACKUP) {}
+RISCVInstrInfo::RISCVInstrInfo(RISCVSubtarget &STI)
+    : RISCVGenInstrInfo(RISCV::ADJCALLSTACKDOWN, RISCV::ADJCALLSTACKUP),
+      STI(STI) {}
 
 unsigned RISCVInstrInfo::isLoadFromStackSlot(const MachineInstr &MI,
                                              int &FrameIndex) const {
   switch (MI.getOpcode()) {
   default:
     return 0;
   case RISCV::LB:
   case RISCV::LBU:
default:
       break;
     case RISCV::ADDI:
     case RISCV::ORI:
     case RISCV::XORI:
       return (MI.getOperand(1).isReg() && MI.getOperand(1).getReg() == RISCV::X0);
   }
   return MI.isAsCheapAsAMove();
 }
+
+bool RISCVInstrInfo::verifyInstruction(const MachineInstr &MI,
+                                       StringRef &ErrInfo) const {
+  const MCInstrInfo *MCII = STI.getInstrInfo();
+  MCInstrDesc const &Desc = MCII->get(MI.getOpcode());
+
+  for (auto &OI : enumerate(Desc.operands())) {
+    unsigned OpType = OI.value().OperandType;
+    if (OpType >= RISCVOp::OPERAND_FIRST_RISCV_IMM &&
+        OpType <= RISCVOp::OPERAND_LAST_RISCV_IMM) {
+      const MachineOperand &MO = MI.getOperand(OI.index());
+      if (MO.isImm()) {
+        int64_t Imm = MO.getImm();
+        bool Ok;
+        switch (OpType) {
+        default:
+          llvm_unreachable("Unexpected operand type");
+        case RISCVOp::OPERAND_UIMM4:
+          Ok = isUInt<4>(Imm);
+          break;
+        case RISCVOp::OPERAND_UIMM5:
+          Ok = isUInt<5>(Imm);
+          break;
+        case RISCVOp::OPERAND_UIMM12:
+          Ok = isUInt<12>(Imm);
+          break;
+        case RISCVOp::OPERAND_SIMM12:
+          Ok = isInt<12>(Imm);
+          break;
+        case RISCVOp::OPERAND_SIMM13_LSB0:
+          Ok = isShiftedInt<12, 1>(Imm);
+          break;
+        case RISCVOp::OPERAND_UIMM20:
+          Ok = isUInt<20>(Imm);
+          break;
+        case RISCVOp::OPERAND_SIMM21_LSB0:
+          Ok = isShiftedInt<20, 1>(Imm);
+          break;
+        case RISCVOp::OPERAND_UIMMLOG2XLEN:
+          if (STI.getTargetTriple().isArch64Bit())
+            Ok = isUInt<6>(Imm);
+          else
+            Ok = isUInt<5>(Imm);
+          break;
+        }
+        if (!Ok) {
+          ErrInfo = "Invalid immediate";
+          return false;
+        }
+      }
+    }
+  }
+
+  return true;
+}

llvm/lib/Target/RISCV/RISCVInstrInfo.td

def FenceArg : AsmOperandClass {
   let RenderMethod = "addFenceArgOperands";
   let DiagnosticType = "InvalidFenceArg";
 }
 
 def fencearg : Operand<XLenVT> {
   let ParserMatchClass = FenceArg;
   let PrintMethod = "printFenceArg";
   let DecoderMethod = "decodeUImmOperand<4>";
+  let OperandType = "OPERAND_UIMM4";
+  let OperandNamespace = "RISCVOp";
 }
 
 def UImmLog2XLenAsmOperand : AsmOperandClass {
   let Name = "UImmLog2XLen";
   let RenderMethod = "addImmOperands";
   let DiagnosticType = "InvalidUImmLog2XLen";
 }
 
 def uimmlog2xlen : Operand<XLenVT>, ImmLeaf<XLenVT, [{
   if (Subtarget->is64Bit())
     return isUInt<6>(Imm);
   return isUInt<5>(Imm);
 }]> {
   let ParserMatchClass = UImmLog2XLenAsmOperand;
   // TODO: should ensure invalid shamt is rejected when decoding.
   let DecoderMethod = "decodeUImmOperand<6>";
   let MCOperandPredicate = [{
     int64_t Imm;
     if (!MCOp.evaluateAsConstantImm(Imm))
       return false;
     if (STI.getTargetTriple().isArch64Bit())
       return  isUInt<6>(Imm);
     return isUInt<5>(Imm);
   }];
+  let OperandType = "OPERAND_UIMMLOG2XLEN";
+  let OperandNamespace = "RISCVOp";
 }
 
 def uimm5 : Operand<XLenVT>, ImmLeaf<XLenVT, [{return isUInt<5>(Imm);}]> {
   let ParserMatchClass = UImmAsmOperand<5>;
   let DecoderMethod = "decodeUImmOperand<5>";
+  let OperandType = "OPERAND_UIMM5";
+  let OperandNamespace = "RISCVOp";
 }
 
 def simm12 : Operand<XLenVT>, ImmLeaf<XLenVT, [{return isInt<12>(Imm);}]> {
   let ParserMatchClass = SImmAsmOperand<12>;
   let EncoderMethod = "getImmOpValue";
   let DecoderMethod = "decodeSImmOperand<12>";
   let MCOperandPredicate = [{
     int64_t Imm;
     if (MCOp.evaluateAsConstantImm(Imm))
       return isInt<12>(Imm);
     return MCOp.isBareSymbolRef();
   }];
+  let OperandType = "OPERAND_SIMM12";
+  let OperandNamespace = "RISCVOp";
 }
 
 // A 13-bit signed immediate where the least significant bit is zero.
 def simm13_lsb0 : Operand<OtherVT> {
   let ParserMatchClass = SImmAsmOperand<13, "Lsb0">;
   let EncoderMethod = "getImmOpValueAsr1";
   let DecoderMethod = "decodeSImmOperandAndLsl1<13>";
   let MCOperandPredicate = [{
     int64_t Imm;
     if (MCOp.evaluateAsConstantImm(Imm))
       return isShiftedInt<12, 1>(Imm);
     return MCOp.isBareSymbolRef();
   }];
+  let OperandType = "OPERAND_SIMM13_LSB0";
+  let OperandNamespace = "RISCVOp";
 }
 
 class UImm20Operand : Operand<XLenVT> {
   let EncoderMethod = "getImmOpValue";
   let DecoderMethod = "decodeUImmOperand<20>";
   let MCOperandPredicate = [{
     int64_t Imm;
     if (MCOp.evaluateAsConstantImm(Imm))
       return isUInt<20>(Imm);
     return MCOp.isBareSymbolRef();
   }];
+  let OperandType = "OPERAND_UIMM20";
+  let OperandNamespace = "RISCVOp";
 }
 
 def uimm20_lui : UImm20Operand {
   let ParserMatchClass = UImmAsmOperand<20, "LUI">;
 }
 def uimm20_auipc : UImm20Operand {
   let ParserMatchClass = UImmAsmOperand<20, "AUIPC">;
 }
 
 def Simm21Lsb0JALAsmOperand : SImmAsmOperand<21, "Lsb0JAL"> {
   let ParserMethod = "parseJALOffset";
 }
 
 // A 21-bit signed immediate where the least significant bit is zero.
 def simm21_lsb0_jal : Operand<OtherVT> {
   let ParserMatchClass = Simm21Lsb0JALAsmOperand;
   let EncoderMethod = "getImmOpValueAsr1";
   let DecoderMethod = "decodeSImmOperandAndLsl1<21>";
   let MCOperandPredicate = [{
     int64_t Imm;
     if (MCOp.evaluateAsConstantImm(Imm))
       return isShiftedInt<20, 1>(Imm);
     return MCOp.isBareSymbolRef();
   }];
+  let OperandType = "OPERAND_SIMM21_LSB0";
+  let OperandNamespace = "RISCVOp";
 }
 
 def BareSymbol : AsmOperandClass {
   let Name = "BareSymbol";
   let RenderMethod = "addImmOperands";
   let DiagnosticType = "InvalidBareSymbol";
   let ParserMethod = "parseBareSymbol";
 }
def CSRSystemRegister : AsmOperandClass {
   let ParserMethod = "parseCSRSystemRegister";
   let DiagnosticType = "InvalidCSRSystemRegister";
 }
 
 def csr_sysreg : Operand<XLenVT> {
   let ParserMatchClass = CSRSystemRegister;
   let PrintMethod = "printCSRSystemRegister";
   let DecoderMethod = "decodeUImmOperand<12>";
+  let OperandType = "OPERAND_UIMM12";
+  let OperandNamespace = "RISCVOp";
 }
 
 // A parameterized register class alternative to i32imm/i64imm from Target.td.
 def ixlenimm : Operand<XLenVT>;
 
 def ixlenimm_li : Operand<XLenVT> {
   let ParserMatchClass = ImmXLenAsmOperand<"", "LI">;
 }

llvm/lib/Target/RISCV/RISCVSubtarget.cpp

RISCVSubtarget &RISCVSubtarget::initializeSubtargetDependencies(
   RISCVFeatures::validate(TT, getFeatureBits());
   return *this;
 }
 
 RISCVSubtarget::RISCVSubtarget(const Triple &TT, StringRef CPU, StringRef FS,
                                StringRef ABIName, const TargetMachine &TM)
     : RISCVGenSubtargetInfo(TT, CPU, FS),
       FrameLowering(initializeSubtargetDependencies(TT, CPU, FS, ABIName)),
-      InstrInfo(), RegInfo(getHwMode()), TLInfo(TM, *this) {
+      InstrInfo(*this), RegInfo(getHwMode()), TLInfo(TM, *this) {
   CallLoweringInfo.reset(new RISCVCallLowering(*getTargetLowering()));
   Legalizer.reset(new RISCVLegalizerInfo(*this));
 
   auto *RBI = new RISCVRegisterBankInfo(*getRegisterInfo());
   RegBankInfo.reset(RBI);
   InstSelector.reset(createRISCVInstructionSelector(
       *static_cast<const RISCVTargetMachine *>(&TM), *this, *RBI));
 }

llvm/lib/Target/RISCV/Utils/RISCVBaseInfo.h

 //
 //===----------------------------------------------------------------------===//
 #ifndef LLVM_LIB_TARGET_RISCV_MCTARGETDESC_RISCVBASEINFO_H
 #define LLVM_LIB_TARGET_RISCV_MCTARGETDESC_RISCVBASEINFO_H
 
 #include "MCTargetDesc/RISCVMCTargetDesc.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
+#include "llvm/MC/MCInstrDesc.h"
 #include "llvm/MC/SubtargetFeature.h"
 
 namespace llvm {
 
 // RISCVII - This namespace holds all of the target specific flags that
 // instruction info tracks. All definitions must match RISCVInstrFormats.td.
 namespace RISCVII {
 enum {
enum {
   MO_TPREL_LO,
   MO_TPREL_HI,
   MO_TPREL_ADD,
   MO_TLS_GOT_HI,
   MO_TLS_GD_HI,
 };
 } // namespace RISCVII
 
+namespace RISCVOp {
+enum OperandType : unsigned {
+  OPERAND_FIRST_RISCV_IMM = MCOI::OPERAND_FIRST_TARGET,
+  OPERAND_UIMM4 = OPERAND_FIRST_RISCV_IMM,
+  OPERAND_UIMM5,
+  OPERAND_UIMM12,
+  OPERAND_SIMM12,
+  OPERAND_SIMM13_LSB0,
+  OPERAND_UIMM20,
+  OPERAND_SIMM21_LSB0,
+  OPERAND_UIMMLOG2XLEN,
+  OPERAND_LAST_RISCV_IMM = OPERAND_UIMMLOG2XLEN
+};
+} // namespace RISCVOp
+
 // Describes the predecessor/successor bits used in the FENCE instruction.
 namespace RISCVFenceField {
 enum FenceField {
   I = 8,
   O = 4,
   R = 2,
   W = 1
 };

llvm/test/CodeGen/RISCV/verify-instr.mir

+# RUN: not llc -march=riscv32 -run-pass machineverifier %s -o - 2>&1 | FileCheck %s
+
+# CHECK: *** Bad machine code: Invalid immediate ***
+# CHECK: - instruction: $x2 = ADDI $x1, 10000
+
+---
+name: verify_instr
+body: |
+  bb.0:
+    $x2 = ADDI $x1, 10000
+...