[llvm-commits] [LLVM] [MC Disassembler] Addding support for dissassembler for MIPS

[Akira Hatanaka]

Hi Vladimir,

I just have a few questions and comments.
Please see them below.

1. Do you have to forward declare these functions? It looks like you
can just move the definitions of the functions above #include
"MipsGenDisassemblerTables.inc".

+// Forward declare these because the autogenerated code will reference them.
+// Definitions are further down.
+static DecodeStatus DecodeCPU64RegsRegisterClass(MCInst &Inst, unsigned RegNo,
+                               uint64_t Address, const void *Decoder);
+

2. Indentation is not right here. Also, you can remove the blank first
line. Please fix all coding style violations in the patch.
+MipsDisassembler::getInstruction(MCInst &instr,
+                                       uint64_t &Size,
+                                       const MemoryObject &Region,
+                                       uint64_t Address,
+                                       raw_ostream &vStream,
+                                       raw_ostream &cStream) const {
+
+   uint8_t Bytes[4];

3. MipsDisassembler::getInstruction and
Mips64Disassembler::getInstruction have identical code here. Would it
make sense to define a function that is called from both functions and
sets Insn?

   uint8_t Bytes[4];
+   uint32_t Insn;
+
+  // We want to read exactly 4 Bytes of data.
+  if (Region.readBytes(Address, 4, (uint8_t*)Bytes, NULL) == -1) {
+    Size = 0;
+    return MCDisassembler::Fail;
+  }
+
+  if (isBigEndian) {
+    // Encoded as a big-endian 32-bit word in the stream.
+    Insn = (Bytes[3] <<  0) |
+           (Bytes[2] <<  8) |
+           (Bytes[1] << 16) |
+           (Bytes[0] << 24);
+  }
+  else {
+    // Encoded as a small-endian 32-bit word in the stream.
+    Insn = (Bytes[0] <<  0) |
+           (Bytes[1] <<  8) |
+           (Bytes[2] << 16) |
+           (Bytes[3] << 24);
+  }

4. DecodeFGR64RegisterClass and DecodeAFGR64RegisterClass looks the
same. Rather than defining identical function, can
DecodeFGR64RegisterClass be called inside DecodeAFGR64RegisterClass?

5. Can SignExtend32 or 64 in Support/MathExtras.h be used here?
static int signedExtend(int num,uint8_t bit) {
+  int Mask = (0x1)<<(bit-1);
+  return ((num ^ Mask) - Mask);
+}
+

6. Indentation seems wrong here (and in other places).
+def SCD_P8 : SCBase<0x3c, "scd", CPU64Regs, mem64>, Requires<[IsN64]>{
+                     let isCodeGenOnly = 1;
+                     }

I think this looks better:
def SCD_P8 : SCBase<0x3c, "scd", CPU64Regs, mem64>, Requires<[IsN64]>{
   let isCodeGenOnly = 1;
}

7. If this testing little endian code, the triple should be
mips64el-unknown-linux.
--- test/MC/Disassembler/Mips/mips64_le.txt	(revision 0)
+++ test/MC/Disassembler/Mips/mips64_le.txt	(revision 0)
@@ -0,0 +1,67 @@
+# RUN: llvm-mc --disassemble %s -triple=mips64-unknown-linux

+

8. Why is that "mem64" has DecoderMethod but "mem" doesn't? And why do
the load/store instructions have DecodeMethod? Is tablegen unable to
handle MIOperand?

def mem64 : Operand<i64> {
   let PrintMethod = "printMemOperand";
   let MIOperandInfo = (ops CPU64Regs, simm16_64);
+  let DecoderMethod = "DecodeMem64";
 }


On Fri, Mar 16, 2012 at 8:45 AM, Medic, Vladimir <vmedic at mips.com> wrote:
> Hi everyone,
> here is a patch that adds support for MIPS Disassembler. Comments are
> welcome.
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>