[PATCH] D108760: [M68k] Add parsing support for more operand types

[Ricky Taylor via Phabricator via llvm-commits]

ricky26 added inline comments.


================
Comment at: llvm/lib/Target/M68k/AsmParser/M68kAsmParser.cpp:86
 struct M68kMemOp {
   enum class Kind {
     Addr,
----------------
jrtc27 wrote:
> Needs rebasing to use KindTy
Actually, that's a different enum. `KindTy` is in M68kOperand. Perhaps I should rename this one too?


================
Comment at: llvm/lib/Target/M68k/AsmParser/M68kAsmParser.cpp:268
+static inline void printExpr(raw_ostream &OS, const MCExpr *Expr) {
+  if (Expr != nullptr) {
+    OS << *Expr;
----------------
jrtc27 wrote:
> This kind of thing seems like a bad idea, just asking for expressions to go missing and get silently converted to 0. The places where this can happen should really be creating an MCConstantExpr of 0.
I agree but at the moment this is working as a mechanism to preserve the existing functionality.

Ideally we should pick ARII/ARID instructions based on whether the displacement is 0, rather than null. However, that causes a lot of changes to tests and particularly sometimes causes the assembler to pick a 'worse' instruction (using an ARID instruction when an ARII one exists).

I put a couple of comments elsewhere in the file which I hoped covered this sentiment.


================
Comment at: llvm/test/MC/M68k/instructions.s:14
+; don't (yet) support this operand format.
+; CHECK: 'move.l', %6, ([50,%6],%7.4*2,20)
+move.l %a0, ([50, %a0], %a1.L * 2, 20)
----------------
jrtc27 wrote:
> This is... really weird. I don't think it makes any sense to add parsing before it can actually be used for anything.
That's fair - I implemented them as I was going through the checklist for all the addressing modes in the CPU manual (the additional addressing modes were fallout from the initial AsmParser work). I figured that completeness for this one section (even without MC support) would be more beneficial than the smaller patch with just the ones we can currently support. I expected we were going to need the MC part for the backend to graduate from experimental anyway.

I did consider adding the relevant move instruction formats in a separate patch first but I'm lament to add to that area whilst (at least in my mind) there's still a bit of a sword of Damocles hanging over the instruction representation in the tablegen files.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108760