[PATCH] D98537: [M68k] Implement AsmParser

Mon Apr 5 14:53:43 PDT 2021

ricky26 marked an inline comment as done.
ricky26 added a comment.

In D98537#2669604 <https://reviews.llvm.org/D98537#2669604>, @myhsu wrote:

> 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?

Thanks! I'm having fun and it's been nice to dip my toes into LLVM contribution. :)

For now, this only supports Motorola syntax (actually, MIT-style `(offset, %reg)` should parse correctly too). It also only supports _some_ operand formats (immediate, address, direct register, indirect and indirect with displacement).

I agree about the tests. I think the existing tests should be converted to Motorola syntax too (as that seems to be the canonical format), however, I'd argue that we should do those //after// the assembler & disassembler have landed (in my opinion, fleshing out that functionality is more pressing as it helps us write better tests more easily).

The included test covers the broad strokes of the supported syntax so far and should be enough to prevent code rot. As per the older comment it'll still need a fair few (smaller) revisions. In my opinion, it makes the most sense to add more tests as we go through and ensure the AsmParser actually supports everything it should. Perhaps it makes sense to add relevant Bugzilla bugs for the missing functionality as a prerequisite to landing this?

================
Comment at: llvm/lib/Target/M68k/AsmParser/M68kAsmParser.cpp:51
+  // Parser functions.
+  void eatComma();
+
----------------
myhsu wrote:
> Should this be exposed as public method?
Hadn't thought much about this as the class is in an anonymous namespace but yes, makes sense, I'll sort them out properly.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98537