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

[Philip Reames via llvm-commits]

reames added a subscriber: reames.

================
Comment at: lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:44
@@ +43,3 @@
+
+namespace llvm {
+extern Target TheRISCV32Target, TheRISCV64Target;
----------------
Style: Should these be in a header somewhere?

================
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();
----------------
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.

================
Comment at: lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:71
@@ +70,3 @@
+                                           const void *Decoder) {
+  if (RegNo > 31)
+    return MCDisassembler::Fail;
----------------
style: Is there a RegClass.max like value we could use here instead?

================
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)));
----------------
Not quite sure what we're going for here, but sign extending an unsigned immediate seems a bit odd.  Possibly a clarifying comment?

================
Comment at: lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:101
@@ +100,3 @@
+                                               raw_ostream &CS) const {
+  // Get the four bytes of the instruction.
+  Size = 4;
----------------
Style: move this below the early return.

================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:102
@@ +101,3 @@
+  if (MO.isImm())
+    return MO.getImm();
+
----------------
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?

================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:104
@@ +103,3 @@
+
+  assert(MO.isExpr() &&
+         "getImm20OpValue expects only expressions or immediates");
----------------
Why the assert immediately followed by an unreachable

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


https://reviews.llvm.org/D23567