[PATCH] D98537: [M68k] Implement AsmParser

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

myhsu added a comment.

Thanks for this amazing efforts :-)  I only have minor formatting comments inlined in the code.
I do have a more high-level question: It seems that you only support Motorola syntax right now, is that true? I'm totally fine if you don't add the MIT syntax in this patch, just want to make sure.
Now a bigger issue is the testing: Presumably we should rewrite all of our existing tests under `test/CodeGen/M68k/Encoding` into assembly and put it under `test/MC/M68k`. I believe this can give us a better testing coverage on all of our MC components including AsmParser.
On one hand I don't want to put those migrating changes into this patch because it will easily bloat the size. Not to mention those tests contains binary encoding tests, so if we need migrate both kinds of tests at once. On the other hand, I still need to make sure this patch reaches certain test coverage. Fortunately, the test you provided seemed to cover cases in each instruction classes. Can you add a comment in that test file to note our tests migration plan?



================
Comment at: llvm/lib/Target/M68k/AsmParser/M68kAsmParser.cpp:51
+  // Parser functions.
+  void eatComma();
+
----------------
Should this be exposed as public method?


================
Comment at: llvm/lib/Target/M68k/AsmParser/M68kAsmParser.cpp:55
+  OperandMatchResultTy parseImm(OperandVector &Operands);
+  OperandMatchResultTy parseMemOp(OperandVector &Operands);
+
----------------
ditto for the above three. Ideally we should minimize the number of public methods


================
Comment at: llvm/test/MC/M68k/instructions.s:1
+; RUN: llvm-mc -triple m68k -show-encoding -motorola-integers %s | FileCheck -check-prefixes=CHECK %s
+
----------------
`-check-prefixes=CHECK` is redundant


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98537