[RISCV] Add MachineInstr immediate verification
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 `MCOperandPredicate`s. Since the `MachineInstr` and `MCInstr` notions of immediates are fundamentally different, this is currently necessary.
Reviewers: asb, lenary
Reviewed By: lenary
Subscribers: hiraditya, rbar, johnrusso, simoncook, apazos, sabuasal, niosHD, kito-cheng, shiva0217, jrtc27, MaskRay, zzheng, edward-jones, rogfer01, MartinMosbeck, brucehoult, the_o, rkruppe, PkmX, jocewei, psnobl, benna, Jim, s.egerton, pzheng, llvm-commits
Tags: #llvm
Differential Revision: https://reviews.llvm.org/D67397
git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@375006 91177308-0d34-0410-b5e6-96231b3b80d8
diff --git a/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp b/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
index 082bd33..5a4c86e 100644
--- a/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
+++ b/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp
@@ -16,6 +16,7 @@
#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"
diff --git a/lib/Target/RISCV/RISCVInstrInfo.cpp b/lib/Target/RISCV/RISCVInstrInfo.cpp
index a71b875..0848392 100644
--- a/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -29,8 +29,9 @@
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 {
@@ -486,3 +487,58 @@
}
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;
+}
diff --git a/lib/Target/RISCV/RISCVInstrInfo.h b/lib/Target/RISCV/RISCVInstrInfo.h
index fa930dd..d3ae04a 100644
--- a/lib/Target/RISCV/RISCVInstrInfo.h
+++ b/lib/Target/RISCV/RISCVInstrInfo.h
@@ -21,10 +21,12 @@
namespace llvm {
+class RISCVSubtarget;
+
class RISCVInstrInfo : public RISCVGenInstrInfo {
public:
- RISCVInstrInfo();
+ explicit RISCVInstrInfo(RISCVSubtarget &STI);
unsigned isLoadFromStackSlot(const MachineInstr &MI,
int &FrameIndex) const override;
@@ -80,6 +82,12 @@
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
diff --git a/lib/Target/RISCV/RISCVInstrInfo.td b/lib/Target/RISCV/RISCVInstrInfo.td
index fd130d1..db2ecc4 100644
--- a/lib/Target/RISCV/RISCVInstrInfo.td
+++ b/lib/Target/RISCV/RISCVInstrInfo.td
@@ -93,6 +93,8 @@
let ParserMatchClass = FenceArg;
let PrintMethod = "printFenceArg";
let DecoderMethod = "decodeUImmOperand<4>";
+ let OperandType = "OPERAND_UIMM4";
+ let OperandNamespace = "RISCVOp";
}
def UImmLog2XLenAsmOperand : AsmOperandClass {
@@ -117,11 +119,15 @@
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);}]> {
@@ -134,6 +140,8 @@
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.
@@ -147,6 +155,8 @@
return isShiftedInt<12, 1>(Imm);
return MCOp.isBareSymbolRef();
}];
+ let OperandType = "OPERAND_SIMM13_LSB0";
+ let OperandNamespace = "RISCVOp";
}
class UImm20Operand : Operand<XLenVT> {
@@ -158,6 +168,8 @@
return isUInt<20>(Imm);
return MCOp.isBareSymbolRef();
}];
+ let OperandType = "OPERAND_UIMM20";
+ let OperandNamespace = "RISCVOp";
}
def uimm20_lui : UImm20Operand {
@@ -182,6 +194,8 @@
return isShiftedInt<20, 1>(Imm);
return MCOp.isBareSymbolRef();
}];
+ let OperandType = "OPERAND_SIMM21_LSB0";
+ let OperandNamespace = "RISCVOp";
}
def BareSymbol : AsmOperandClass {
@@ -230,6 +244,8 @@
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.
diff --git a/lib/Target/RISCV/RISCVSubtarget.cpp b/lib/Target/RISCV/RISCVSubtarget.cpp
index 5673f22..f114c6a 100644
--- a/lib/Target/RISCV/RISCVSubtarget.cpp
+++ b/lib/Target/RISCV/RISCVSubtarget.cpp
@@ -51,7 +51,7 @@
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));
diff --git a/lib/Target/RISCV/Utils/RISCVBaseInfo.h b/lib/Target/RISCV/Utils/RISCVBaseInfo.h
index c33c72f..30e475e 100644
--- a/lib/Target/RISCV/Utils/RISCVBaseInfo.h
+++ b/lib/Target/RISCV/Utils/RISCVBaseInfo.h
@@ -16,6 +16,7 @@
#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 {
@@ -63,6 +64,21 @@
};
} // 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 {
diff --git a/test/CodeGen/RISCV/verify-instr.mir b/test/CodeGen/RISCV/verify-instr.mir
new file mode 100644
index 0000000..ed31126
--- /dev/null
+++ b/test/CodeGen/RISCV/verify-instr.mir
@@ -0,0 +1,11 @@
+# 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
+...
