[PATCH] D16723: [AMDGPU] Disassembler: Added basic disassembler for AMDGPU target.

[Matt Arsenault via llvm-commits]

arsenm added inline comments.

================
Comment at: lib/Target/AMDGPU/AMDGPU.td:306
@@ -305,2 +305,3 @@
   let guessInstructionProperties = 1;
+  let decodePositionallyEncodedOperands = 1;
   let noNamedPositionallyEncodedOperands = 1;
----------------
Do we need this? Ideally we would only use the names

================
Comment at: lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:22
@@ +21,3 @@
+#include "AMDGPU.h"
+#include "AMDGPUDisassembler.h"
+#include "AMDGPURegisterInfo.h"
----------------
This should be the first included file

================
Comment at: lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:40-74
@@ +39,37 @@
+
+static DecodeStatus DecodeVGPR_32RegisterClass(MCInst &Inst, unsigned Imm,
+                                               uint64_t Addr, const void *Decoder);
+
+static DecodeStatus DecodeVS_32RegisterClass(llvm::MCInst &Inst, unsigned Imm,
+                                             uint64_t Addr, const void *Decoder);
+
+static DecodeStatus DecodeVS_64RegisterClass(MCInst &Inst, unsigned Imm,
+                                             uint64_t Addr, const void *Decoder);
+
+static DecodeStatus DecodeVReg_64RegisterClass(MCInst &Inst, unsigned Imm,
+                                               uint64_t Addr, const void *Decoder);
+
+static DecodeStatus DecodeVReg_96RegisterClass(MCInst &Inst, unsigned Imm,
+                                               uint64_t Addr, const void *Decoder);
+
+static DecodeStatus DecodeVReg_128RegisterClass(MCInst &Inst, unsigned Imm,
+                                                uint64_t Addr, const void *Decoder);
+
+static DecodeStatus DecodeSGPR_32RegisterClass(MCInst &Inst, unsigned Imm,
+                                               uint64_t Addr, const void *Decoder);
+
+static DecodeStatus DecodeSReg_32RegisterClass(MCInst &Inst, unsigned Imm,
+                                               uint64_t Addr, const void *Decoder);
+
+static DecodeStatus DecodeSReg_64RegisterClass(MCInst &Inst, unsigned Imm,
+                                               uint64_t Addr, const void *Decoder);
+
+static DecodeStatus DecodeSReg_128RegisterClass(MCInst &Inst, unsigned Imm,
+                                                uint64_t Addr, const void *Decoder);
+
+static DecodeStatus DecodeSReg_256RegisterClass(MCInst &Inst, unsigned Imm,
+                                                uint64_t Addr, const void *Decoder);
+
+static DecodeStatus DecodeSReg_512RegisterClass(MCInst &Inst, unsigned Imm,
+                                                uint64_t Addr, const void *Decoder);
+
----------------
Can you move the definitions of these functions up to avoid needing declaring them separately?

Can they also be moved down to be below the includes for the generated files?

================
Comment at: lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp:113
@@ +112,3 @@
+
+DecodeStatus AMDGPUDisassembler::DecodeLitFloat(unsigned Imm, uint32_t& F) const {
+  // ToDo: case 248: 1/(2*PI) - is allowed only on VI
----------------
It looks like this doesn't handle using one of the literal constants for an 64-bit/f64 operand, which does work.

================
Comment at: lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h:1
@@ +1,2 @@
+//===-- AMDGPUDisassembler.hpp - Disassembler for AMDGPU ISA --------------===//
+//
----------------
This should have a -*- C++ -*_ added at the end of the first line

================
Comment at: lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h:15
@@ +14,3 @@
+//===----------------------------------------------------------------------===//
+//
+
----------------
Remove this extra comment line

================
Comment at: lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h:24-26
@@ +23,5 @@
+
+  class MCSubtargetInfo;
+  class MCContext;
+  class MCInst;
+
----------------
These should be alphabetized


http://reviews.llvm.org/D16723