From 1893f9a45813d7b486576d88642e0e1cc5bc92fb Mon Sep 17 00:00:00 2001 From: Luis Marques Date: Wed, 16 Oct 2019 15:06:02 +0000 Subject: [PATCH] [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 llvm-svn: 375006 --- .../RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp | 1 + llvm/lib/Target/RISCV/RISCVInstrInfo.cpp | 60 ++++++++++++++++++- llvm/lib/Target/RISCV/RISCVInstrInfo.h | 10 +++- llvm/lib/Target/RISCV/RISCVInstrInfo.td | 16 +++++ llvm/lib/Target/RISCV/RISCVSubtarget.cpp | 2 +- llvm/lib/Target/RISCV/Utils/RISCVBaseInfo.h | 16 +++++ llvm/test/CodeGen/RISCV/verify-instr.mir | 11 ++++ 7 files changed, 112 insertions(+), 4 deletions(-) create mode 100644 llvm/test/CodeGen/RISCV/verify-instr.mir diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp index 082bd331d45d..5a4c86e48f1e 100644 --- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp +++ b/llvm/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/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp index a71b8759f1cd..084839299530 100644 --- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp +++ b/llvm/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 @@ bool RISCVInstrInfo::isAsCheapAsAMove(const MachineInstr &MI) const { } 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/llvm/lib/Target/RISCV/RISCVInstrInfo.h b/llvm/lib/Target/RISCV/RISCVInstrInfo.h index fa930dd6cb49..d3ae04aefe04 100644 --- a/llvm/lib/Target/RISCV/RISCVInstrInfo.h +++ b/llvm/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 @@ public: 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/llvm/lib/Target/RISCV/RISCVInstrInfo.td b/llvm/lib/Target/RISCV/RISCVInstrInfo.td index fd130d12d97b..db2ecc49d14e 100644 --- a/llvm/lib/Target/RISCV/RISCVInstrInfo.td +++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.td @@ -93,6 +93,8 @@ def fencearg : Operand { let ParserMatchClass = FenceArg; let PrintMethod = "printFenceArg"; let DecoderMethod = "decodeUImmOperand<4>"; + let OperandType = "OPERAND_UIMM4"; + let OperandNamespace = "RISCVOp"; } def UImmLog2XLenAsmOperand : AsmOperandClass { @@ -117,11 +119,15 @@ def uimmlog2xlen : Operand, ImmLeaf(Imm); return isUInt<5>(Imm); }]; + let OperandType = "OPERAND_UIMMLOG2XLEN"; + let OperandNamespace = "RISCVOp"; } def uimm5 : Operand, ImmLeaf(Imm);}]> { let ParserMatchClass = UImmAsmOperand<5>; let DecoderMethod = "decodeUImmOperand<5>"; + let OperandType = "OPERAND_UIMM5"; + let OperandNamespace = "RISCVOp"; } def simm12 : Operand, ImmLeaf(Imm);}]> { @@ -134,6 +140,8 @@ def simm12 : Operand, ImmLeaf(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. @@ -147,6 +155,8 @@ def simm13_lsb0 : Operand { return isShiftedInt<12, 1>(Imm); return MCOp.isBareSymbolRef(); }]; + let OperandType = "OPERAND_SIMM13_LSB0"; + let OperandNamespace = "RISCVOp"; } class UImm20Operand : Operand { @@ -158,6 +168,8 @@ class UImm20Operand : Operand { return isUInt<20>(Imm); return MCOp.isBareSymbolRef(); }]; + let OperandType = "OPERAND_UIMM20"; + let OperandNamespace = "RISCVOp"; } def uimm20_lui : UImm20Operand { @@ -182,6 +194,8 @@ def simm21_lsb0_jal : Operand { return isShiftedInt<20, 1>(Imm); return MCOp.isBareSymbolRef(); }]; + let OperandType = "OPERAND_SIMM21_LSB0"; + let OperandNamespace = "RISCVOp"; } def BareSymbol : AsmOperandClass { @@ -230,6 +244,8 @@ def csr_sysreg : Operand { 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/llvm/lib/Target/RISCV/RISCVSubtarget.cpp b/llvm/lib/Target/RISCV/RISCVSubtarget.cpp index 5673f2205a6d..f114c6ac1925 100644 --- a/llvm/lib/Target/RISCV/RISCVSubtarget.cpp +++ b/llvm/lib/Target/RISCV/RISCVSubtarget.cpp @@ -51,7 +51,7 @@ 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)); diff --git a/llvm/lib/Target/RISCV/Utils/RISCVBaseInfo.h b/llvm/lib/Target/RISCV/Utils/RISCVBaseInfo.h index c33c72f24319..30e475e80a01 100644 --- a/llvm/lib/Target/RISCV/Utils/RISCVBaseInfo.h +++ b/llvm/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 @@ enum { }; } // 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/llvm/test/CodeGen/RISCV/verify-instr.mir b/llvm/test/CodeGen/RISCV/verify-instr.mir new file mode 100644 index 000000000000..ed31126b53d4 --- /dev/null +++ b/llvm/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 +...