[PATCH] D98540: [M68k] Implement Disassembler

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu added a comment.

First of all, good job :-)
Though I only scanned over the patch, I'm a little concerned about the adoption of code beads here because //ideally//, we are going to remove code beads from LLVM in the future. As stated in one of my comments, can we get any help from the disassembler table generated by TableGen?

Also can you split any changes regarding assembly syntax into a separate patch? Because MCDisassembler only generates MCInst, it is AsmPrinter's (and InstPrinter's) job to decide how to present it.



================
Comment at: llvm/lib/Target/M68k/CMakeLists.txt:14
 tablegen(LLVM M68kGenAsmMatcher.inc       -gen-asm-matcher)
+tablegen(LLVM M68kGenDisassemblerTables.inc -gen-disassembler)
 
----------------
Do we use this table anywhere? If not, is there any reason we can't use it?


================
Comment at: llvm/lib/Target/M68k/Disassembler/CMakeLists.txt:4
+
+  LINK_COMPONENTS
+  MCDisassembler
----------------
Since you use `getTheM68kTarget` here, please explicitly link M68kInfo. I bumped into a related [[ https://github.com/llvm/llvm-project/commit/91b6ef64db55084b33295e640258c926acedcb1a | problem ]] in M68kAsmParser where the linker is unable to find `getTheM68kTarget` symbol if you build LLVM libraries as shared objects.


================
Comment at: llvm/lib/Target/M68k/Disassembler/M68kDisassembler.cpp:309
+      case M68kBeads::Ctrl:
+        switch (Ext) {
+        case M68kBeads::Ignore:
----------------
Is there any specific reason to use a single-case switch rather than if-statement here?


================
Comment at: llvm/lib/Target/M68k/Disassembler/M68kDisassembler.cpp:380
+                                              unsigned Bead) const {
+  auto Ext = Bead >> 4;
+
----------------
Can we use explicit type here


================
Comment at: llvm/lib/Target/M68k/Disassembler/M68kDisassembler.cpp:401
+                                              unsigned Bead) const {
+  auto Ext = Bead >> 4;
+
----------------
ditto


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kInstPrinter.cpp:196
+
+  if (MO.isExpr()) {
+    MO.getExpr()->print(O, &MAI);
----------------
Can we put these changes into a separate patch (possibility combined with your changes on tests below)?
If the changes here affect the disassembler test, then maybe you can put it (the separate patch) as the dependency for the current disassembler patch.


================
Comment at: llvm/test/MC/M68k/instructions.s:11
 
-; CHECK: move.l %a1, %a0
+; CHECK: move.l %a1, %a0    ; encoding: [0x20,0x49]
 move.l %a1, %a0
----------------
Test directives related to instruction encoding is irrelevant to disassembler. Please remove them from this patch.


================
Comment at: llvm/test/MC/M68k/instructions.s:36
+; CHECK: lsl.l #1, %a1      ; encoding: [0xe3,0x89]
+lsl.l #1, %a1
+; CHECK: lsr.l #3, %a1      ; encoding: [0xe6,0x89]
----------------
Can we put changes on tests into a separate patch?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98540/new/

https://reviews.llvm.org/D98540