[llvm-commits] [LLVM] [MC Disassembler] Addding support for dissassembler for MIPS
Akira Hatanaka
ahatanak at gmail.com
Fri Mar 23 13:57:47 PDT 2012
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
>
More information about the llvm-commits
mailing list