[PATCH] D23567: [RISCV 9/10] Add support for disassembly

[Alex Bradbury via llvm-commits]

asb added inline comments.

================
Comment at: lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:44
@@ +43,3 @@
+
+namespace llvm {
+extern Target TheRISCV32Target, TheRISCV64Target;
----------------
reames wrote:
> Style: Should these be in a header somewhere?
Actually this is redundant as they're already in RISCVMCTargetDesc.h which is already include - removed. Thanks!

================
Comment at: lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:63
@@ +62,3 @@
+static unsigned getReg(const void *D, unsigned RC, unsigned RegNo) {
+  const RISCVDisassembler *Dis = static_cast<const RISCVDisassembler *>(D);
+  const MCRegisterInfo *RegInfo = Dis->getContext().getRegisterInfo();
----------------
reames wrote:
> 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.
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.

================
Comment at: lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:89
@@ +88,3 @@
+                                      int64_t Address, const void *Decoder) {
+  assert(isUInt<N>(Imm) && "Invalid immediate");
+  Inst.addOperand(MCOperand::createImm(SignExtend64<N>(Imm)));
----------------
reames wrote:
> Not quite sure what we're going for here, but sign extending an unsigned immediate seems a bit odd.  Possibly a clarifying comment?
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.

================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:102
@@ +101,3 @@
+  if (MO.isImm())
+    return MO.getImm();
+
----------------
reames wrote:
> 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?
I've now defined getImmOpValue rather than first defining getImm20OpValue only to change things in the next patch.

================
Comment at: lib/Target/RISCV/RISCVInstrFormats.td:13
@@ -12,2 +12,3 @@
   field bits<32> Inst;
+  field bits<32> SoftFail = 0;
   let Size = 4;
----------------
reames wrote:
> What is this?  I don't see it being used anywhere in this patch?
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.



https://reviews.llvm.org/D23567