[RISCV 9/10] Add support for disassembly

[asb]

Summary

Disassembler support allows for 'round-trip' testing, and rv32i-valid.s has been updated appropriately.

[reames]

lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp

45

Style: Should these be in a header somewhere?

64

Can this static cast be lifted through the call chain to the entry point? Or are each of these used directly from table gen? This is more than a bit messy and concerning.

72

style: Is there a RegClass.max like value we could use here instead?

90

Not quite sure what we're going for here, but sign extending an unsigned immediate seems a bit odd. Possibly a clarifying comment?

102

Style: move this below the early return.

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

106

We need to assert the size of the immediate don't we? Also, didn't we have a checked cast function in one of the previous patches already?

108

Why the assert immediately followed by an unreachable

lib/Target/RISCV/RISCVInstrFormats.td

31

What is this? I don't see it being used anywhere in this patch?

[asb]

Refresh to reflect change made in previous patches in the series, and address review comments from @reames.

lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp

45

Actually this is redundant as they're already in RISCVMCTargetDesc.h which is already include - removed. Thanks!

64

I've lifted the cast to DecodeGPRRegisterClass, which is the entry-point from TableGen generated code, and in the end got rid of getReg entirely.

90

The immediate value is extracted from the instruction and passed as a uint64_t. The assert checks that it the bitwidth is as we expect, but we can sign-extend it from the appropriate bit position. SignExtend64 is written for this exact purpose (sign extends the bottom N bits of the uint64_t that it is passed).

I've added a comment to clarify what is going on that hopefully makes it easier to follow? I've also added a comment in the new decodeSImmOperandAndLsl1 which is perhaps even more confusing. Do let me know any suggestions for making it clearer.

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

106

I've now defined getImmOpValue rather than first defining getImm20OpValue only to change things in the next patch.

lib/Target/RISCV/RISCVInstrFormats.td

31

Tablegenning the disassembler fails without it. I've stolen an explanatory comment from the AMDGPU backend:

// SoftFail is a field the disassembler can use to provide a way for
// instructions to not match without killing the whole decode process. It is
// mainly used for ARM, but Tablegen expects this field to exist or it fails
// to build the decode table.

[theraven]

lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp

112

RISC-VC?

[jyknight]

test/MC/RISCV/rv32i-valid.s

5

Using the same file to test both directions seems like a nice method -- but strangely, almost unique amongst the tests for the backends.

I actually especially like the way it's done in test/MC/X86/mpx-encodings.s, where it uses the same CHECK: line for both the asm output of llvm-mc, and the output of llvm-objdump.

I'd suggest doing it that way, which would nicely resolve my comment in D23564 too.

[asb]

Tests now check both assembly printing and disassembly. I've added a comment/TODO regarding decode of instructions outside of the RV32/RV64 core that might be 16-bit, 48bit, 64bit, ...

[asb]

test/MC/RISCV/rv32i-valid.s

5

I went ahead with the CHECK-INST approach I described in D23564.

[asb]

Refresh to follow changes made in rL283702, putting TheRISCV32Target and TheRISCV64Target behind accessors.

[asb]

Fix rv32i-valid.s, so CHECK-INST is used rather than CHECK-DIS.

[asb]

The previous version of this patch made the mistake when decoding a GPR of assuming you can use the parsed RegNo as an index into the register class. In the general case, a register class will have the registers in preferred allocation order, meaning this will fail. Instead, we have a GPRDecoderTable to select the appropriate register based on the parsed RegNo.

lib/Target/RISCV/CMakeLists.txt

 set(LLVM_TARGET_DEFINITIONS RISCV.td)
 
 tablegen(LLVM RISCVGenRegisterInfo.inc -gen-register-info)
 tablegen(LLVM RISCVGenInstrInfo.inc -gen-instr-info)
 tablegen(LLVM RISCVGenMCCodeEmitter.inc -gen-emitter)
 tablegen(LLVM RISCVGenAsmMatcher.inc -gen-asm-matcher)
 tablegen(LLVM RISCVGenAsmWriter.inc -gen-asm-writer)
+tablegen(LLVM RISCVGenSubtargetInfo.inc -gen-subtarget)
+tablegen(LLVM RISCVGenDisassemblerTables.inc -gen-disassembler)
 
 add_public_tablegen_target(RISCVCommonTableGen)
 
 add_llvm_target(RISCVCodeGen
   RISCVTargetMachine.cpp
   )
 
 add_subdirectory(AsmParser)
+add_subdirectory(Disassembler)
 add_subdirectory(InstPrinter)
 add_subdirectory(MCTargetDesc)
 add_subdirectory(TargetInfo)

lib/Target/RISCV/Disassembler/CMakeLists.txt

+add_llvm_library(LLVMRISCVDisassembler
+  RISCVDisassembler.cpp
+  )

lib/Target/RISCV/Disassembler/LLVMBuild.txt

+;===- ./lib/Target/RISCV/Disassembler/LLVMBuild.txt ------------*- Conf -*--===;
+;
+;                     The LLVM Compiler Infrastructure
+;
+; This file is distributed under the University of Illinois Open Source
+; License. See LICENSE.TXT for details.
+;
+;===------------------------------------------------------------------------===;
+;
+; This is an LLVMBuild description file for the components in this subdirectory.
+;
+; For more information on the LLVMBuild system, please see:
+;
+;   http://llvm.org/docs/LLVMBuild.html
+;
+;===------------------------------------------------------------------------===;
+
+[component_0]
+type = Library
+name = RISCVDisassembler
+parent = RISCV
+required_libraries = MCDisassembler RISCVInfo Support
+add_to_library_groups = RISCV
+

lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp

+//===-- RISCVDisassembler.cpp - Disassembler for RISCV --------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+//
+// This file implements the RISCVDisassembler class.
+//
+//===----------------------------------------------------------------------===//
+
+#include "MCTargetDesc/RISCVMCTargetDesc.h"
+#include "llvm/MC/MCContext.h"
+#include "llvm/MC/MCDisassembler/MCDisassembler.h"
+#include "llvm/MC/MCFixedLenDisassembler.h"
+#include "llvm/MC/MCInst.h"
+#include "llvm/MC/MCRegisterInfo.h"
+#include "llvm/MC/MCSubtargetInfo.h"
+#include "llvm/Support/Endian.h"
+#include "llvm/Support/TargetRegistry.h"
+
+using namespace llvm;
+
+#define DEBUG_TYPE "riscv-disassembler"
+
+typedef MCDisassembler::DecodeStatus DecodeStatus;
+
+namespace {
+class RISCVDisassembler : public MCDisassembler {
+
+public:
+  RISCVDisassembler(const MCSubtargetInfo &STI, MCContext &Ctx)
+      : MCDisassembler(STI, Ctx) {}
+
+  DecodeStatus getInstruction(MCInst &Instr, uint64_t &Size,
+                              ArrayRef<uint8_t> Bytes, uint64_t Address,
+                              raw_ostream &VStream,
+                              raw_ostream &CStream) const override;
+};
+} // end anonymous namespace
+
+static MCDisassembler *createRISCVDisassembler(const Target &T,
+                                               const MCSubtargetInfo &STI,
+                                               MCContext &Ctx) {
+  return new RISCVDisassembler(STI, Ctx);
+}
+
+extern "C" void LLVMInitializeRISCVDisassembler() {
+  // Register the disassembler for each target.
+  TargetRegistry::RegisterMCDisassembler(getTheRISCV32Target(),
+                                         createRISCVDisassembler);
+  TargetRegistry::RegisterMCDisassembler(getTheRISCV64Target(),
+                                         createRISCVDisassembler);
+}
+
+static const unsigned GPRDecoderTable[] = {
+  RISCV::X0_32,  RISCV::X1_32,  RISCV::X2_32,  RISCV::X3_32,
+  RISCV::X4_32,  RISCV::X5_32,  RISCV::X6_32,  RISCV::X7_32,
+  RISCV::X8_32,  RISCV::X9_32,  RISCV::X10_32, RISCV::X11_32,
+  RISCV::X12_32, RISCV::X13_32, RISCV::X14_32, RISCV::X15_32,
+  RISCV::X16_32, RISCV::X17_32, RISCV::X18_32, RISCV::X19_32,
+  RISCV::X20_32, RISCV::X21_32, RISCV::X22_32, RISCV::X23_32,
+  RISCV::X24_32, RISCV::X25_32, RISCV::X26_32, RISCV::X27_32,
+  RISCV::X28_32, RISCV::X29_32, RISCV::X30_32, RISCV::X31_32
+};
+
+static DecodeStatus DecodeGPRRegisterClass(MCInst &Inst, uint64_t RegNo,
+                                           uint64_t Address,
+                                           const void *Decoder) {
+   if (RegNo > sizeof(GPRDecoderTable)) {
+     return MCDisassembler::Fail;
+   }
+
+   // We must define our own mapping from RegNo to register identifier.
+   // Accessing index RegNo in the register class will work in the case that
+   // registers were added in ascending order, but not in general.
+   unsigned Reg = GPRDecoderTable[RegNo];
+   Inst.addOperand(MCOperand::createReg(Reg));
+   return MCDisassembler::Success;
+}
+
+template <unsigned N>
+static DecodeStatus decodeUImmOperand(MCInst &Inst, uint64_t Imm,
+                                      int64_t Address, const void *Decoder) {
+  assert(isUInt<N>(Imm) && "Invalid immediate");
+  Inst.addOperand(MCOperand::createImm(Imm));
+  return MCDisassembler::Success;
+}
+
+template <unsigned N>
+static DecodeStatus decodeSImmOperand(MCInst &Inst, uint64_t Imm,
+                                      int64_t Address, const void *Decoder) {
+  assert(isUInt<N>(Imm) && "Invalid immediate");
+  // Sign-extend the number in the bottom N bits of Imm
+  Inst.addOperand(MCOperand::createImm(SignExtend64<N>(Imm)));
+  return MCDisassembler::Success;
+}
+
+template <unsigned N>
+static DecodeStatus decodeSImmOperandAndLsl1(MCInst &Inst, uint64_t Imm,
+                                             int64_t Address,
+                                             const void *Decoder) {
+  assert(isUInt<N>(Imm) && "Invalid immediate");
+  // Sign-extend the number in the bottom N bits of Imm after accounting for
+  // the fact that the N bit immediate is stored in N-1 bits (the LSB is
+  // always zero)
+  Inst.addOperand(MCOperand::createImm(SignExtend64<N>(Imm << 1)));
+  return MCDisassembler::Success;
+}
+
+#include "RISCVGenDisassemblerTables.inc"
+
+DecodeStatus RISCVDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
+                                               ArrayRef<uint8_t> Bytes,
+                                               uint64_t Address,
+                                               raw_ostream &OS,
+                                               raw_ostream &CS) const {
+  // TODO: although assuming 4-byte instructions is sufficient for RV32 and
+  // RV64, this will need modification when supporting the compressed
+  // instruction set extension (RVC) which uses 16-bit instructions. Other
+  // instruction set extensions have the option of defining instructions up to
+  // 176 bits wide.
+  Size = 4;
+  if (Bytes.size() < 4) {
+    Size = 0;
+    return MCDisassembler::Fail;
+  }
+
+  // Get the four bytes of the instruction.
+  uint32_t Inst = support::endian::read32le(Bytes.data());
+
+  return decodeInstruction(DecoderTable32, MI, Inst, Address, this, STI);
+}

lib/Target/RISCV/LLVMBuild.txt

 ;
 ; For more information on the LLVMBuild system, please see:
 ;
 ;   http://llvm.org/docs/LLVMBuild.html
 ;
 ;===------------------------------------------------------------------------===;
 
 [common]
-subdirectories = AsmParser InstPrinter TargetInfo MCTargetDesc
+subdirectories = AsmParser Disassembler InstPrinter TargetInfo MCTargetDesc
 
 [component_0]
 type = TargetGroup
 name = RISCV
 parent = Target
 has_asmparser = 1
 has_asmprinter = 1
+has_disassembler = 1
 
 [component_1]
 type = Library
 name = RISCVCodeGen
 parent = RISCV
 required_libraries = AsmPrinter Core CodeGen MC RISCVAsmPrinter RISCVDesc RISCVInfo Support Target
 add_to_library_groups = RISCV

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp

public:
   /// relocation, record the relocation and return zero.
   unsigned getMachineOpValue(const MCInst &MI, const MCOperand &MO,
                              SmallVectorImpl<MCFixup> &Fixups,
                              const MCSubtargetInfo &STI) const;
 
   unsigned getImmOpValueAsr1(const MCInst &MI, unsigned OpNo,
                              SmallVectorImpl<MCFixup> &Fixups,
                              const MCSubtargetInfo &STI) const;
+  unsigned getImmOpValue(const MCInst &MI, unsigned OpNo,
+                         SmallVectorImpl<MCFixup> &Fixups,
+                         const MCSubtargetInfo &STI) const;
 };
 } // end anonymous namespace
 
 MCCodeEmitter *llvm::createRISCVMCCodeEmitter(const MCInstrInfo &MCII,
                                               const MCRegisterInfo &MRI,
                                               MCContext &Ctx) {
   return new RISCVMCCodeEmitter(Ctx);
 }

 unsigned
 RISCVMCCodeEmitter::getImmOpValueAsr1(const MCInst &MI, unsigned OpNo,
                                       SmallVectorImpl<MCFixup> &Fixups,
                                       const MCSubtargetInfo &STI) const {
   const MCOperand &MO = MI.getOperand(OpNo);
 
   if (MO.isImm()) {
     unsigned Res = MO.getImm();
     assert((Res & 1) == 0 && "LSB is non-zero");
     return Res >> 1;
   }
 
   llvm_unreachable("Unhandled expression!");
+
+  return 0;
+}
+
+unsigned RISCVMCCodeEmitter::getImmOpValue(const MCInst &MI, unsigned OpNo,
+                                           SmallVectorImpl<MCFixup> &Fixups,
+                                           const MCSubtargetInfo &STI) const {
+
+  const MCOperand &MO = MI.getOperand(OpNo);
+
+  // If the destination is an immediate, there is nothing to do
+  if (MO.isImm())
+    return MO.getImm();
+
+  llvm_unreachable("Unhandled expression!");
+
+  return 0;
 }
 
 #include "RISCVGenMCCodeEmitter.inc"

lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.h

 // Defines symbolic names for RISC-V registers.
 #define GET_REGINFO_ENUM
 #include "RISCVGenRegisterInfo.inc"
 
 // Defines symbolic names for RISC-V instructions.
 #define GET_INSTRINFO_ENUM
 #include "RISCVGenInstrInfo.inc"
 
+#define GET_SUBTARGETINFO_ENUM
+#include "RISCVGenSubtargetInfo.inc"
+
 #endif

lib/Target/RISCV/MCTargetDesc/RISCVMCTargetDesc.cpp

 #include "llvm/Support/TargetRegistry.h"
 
 #define GET_INSTRINFO_MC_DESC
 #include "RISCVGenInstrInfo.inc"
 
 #define GET_REGINFO_MC_DESC
 #include "RISCVGenRegisterInfo.inc"
 
+#define GET_SUBTARGETINFO_MC_DESC
+#include "RISCVGenSubtargetInfo.inc"
+
 using namespace llvm;
 
 static MCInstrInfo *createRISCVMCInstrInfo() {
   MCInstrInfo *X = new MCInstrInfo();
   InitRISCVMCInstrInfo(X);
   return X;
 }
 

 extern "C" void LLVMInitializeRISCVTargetMC() {
   for (Target *T : {&getTheRISCV32Target(), &getTheRISCV64Target()}) {
     TargetRegistry::RegisterMCAsmInfo(*T, createRISCVMCAsmInfo);
     TargetRegistry::RegisterMCInstrInfo(*T, createRISCVMCInstrInfo);
     TargetRegistry::RegisterMCRegInfo(*T, createRISCVMCRegisterInfo);
     TargetRegistry::RegisterMCAsmBackend(*T, createRISCVAsmBackend);
     TargetRegistry::RegisterMCCodeEmitter(*T, createRISCVMCCodeEmitter);
     TargetRegistry::RegisterMCInstPrinter(*T, createRISCVMCInstPrinter);
+    TargetRegistry::RegisterMCSubtargetInfo(*T, createRISCVMCSubtargetInfoImpl);
   }
 }

lib/Target/RISCV/RISCVInstrFormats.td

 //  a 21-bit value (where the LSB is always zero), we describe it as an imm20
 //  to match how it is encoded.
 //
 //===----------------------------------------------------------------------===//
 
 class RISCVInst<dag outs, dag ins, string asmstr, list<dag> pattern>
     : Instruction {
   field bits<32> Inst;
+  // SoftFail is a field the disassembler can use to provide a way for
+  // instructions to not match without killing the whole decode process. It is
+  // mainly used for ARM, but Tablegen expects this field to exist or it fails
+  // to build the decode table.
+  field bits<32> SoftFail = 0;
   let Size = 4;
 
   bits<7> Opcode = 0;
 
   let Inst{6-0} = Opcode;
 
   let Namespace = "RISCV";
 

lib/Target/RISCV/RISCVInstrInfo.td

 }
 
 class UImmAsmOperand<int width, string suffix = "">
   : ImmAsmOperand<"U", width, suffix> {
 }
 
 def uimm4 : Operand<i32> {
   let ParserMatchClass = UImmAsmOperand<4>;
+  let DecoderMethod = "decodeUImmOperand<4>";
 }
 
 def uimm5 : Operand<i32> {
   let ParserMatchClass = UImmAsmOperand<5>;
+  let DecoderMethod = "decodeUImmOperand<5>";
 }
 
 def simm12 : Operand<i32> {
   let ParserMatchClass = SImmAsmOperand<12>;
+  let DecoderMethod = "decodeSImmOperand<12>";
 }
 
 def uimm12 : Operand<i32> {
   let ParserMatchClass = UImmAsmOperand<12>;
+  let DecoderMethod = "decodeUImmOperand<12>";
 }
 
 // A 13-bit signed immediate where the least significant bit is zero.
 def simm13_lsb0 : Operand<i32> {
   let ParserMatchClass = SImmAsmOperand<13, "Lsb0">;
   let EncoderMethod = "getImmOpValueAsr1";
+  let DecoderMethod = "decodeSImmOperandAndLsl1<13>";
 }
 
 def uimm20 : Operand<i32> {
   let ParserMatchClass = UImmAsmOperand<20>;
+  let EncoderMethod = "getImmOpValue";
+  let DecoderMethod = "decodeUImmOperand<20>";
 }
 
 // A 21-bit signed immediate where the least significant bit is zero.
 def simm21_lsb0 : Operand<i32> {
   let ParserMatchClass = SImmAsmOperand<21, "Lsb0">;
   let EncoderMethod = "getImmOpValueAsr1";
+  let DecoderMethod = "decodeSImmOperandAndLsl1<21>";
 }
 
 // As noted in RISCVRegisterInfo.td, the hope is that support for
 // variable-sized register classes will mean that instruction definitions do
 // not need to be duplicated for 32-bit and 64-bit register classes. For now
 // we use 'GPR', which is 32-bit. When codegen for both RV32 and RV64 is
 // added, we will need to duplicate instruction definitions unless a proposal
 // like <http://lists.llvm.org/pipermail/llvm-dev/2016-September/105027.html>

test/MC/RISCV/rv32i-valid.s

 # RUN: llvm-mc %s -triple=riscv32 -show-encoding \
 # RUN:     | FileCheck -check-prefixes=CHECK,CHECK-INST %s
 # RUN: llvm-mc %s -triple=riscv64 -show-encoding \
 # RUN:     | FileCheck -check-prefixes=CHECK,CHECK-INST %s
+# RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
+# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=CHECK-INST %s
+# RUN: llvm-mc -filetype=obj -triple riscv64 < %s \
+# RUN:     | llvm-objdump -d - | FileCheck -check-prefix=CHECK-INST %s
 
 # CHECK-INST: lui a0, 2
 # CHECK: encoding: [0x37,0x25,0x00,0x00]
 lui a0, 2
 # CHECK-INST: lui s11, 552960
 # CHECK: encoding: [0xb7,0x0d,0x00,0x87]
 lui s11, (0x87000000>>12)
 # CHECK-INST: lui t0, 1048575