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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

I'd prefer to see the style changes separated out, there are a few too many for me to be bundled with functional changes, and there are places where it takes a moment to check whether something's a functional change or not.



================
Comment at: llvm/lib/Target/M68k/AsmParser/M68kAsmParser.cpp:86
 struct M68kMemOp {
   enum class Kind {
     Addr,
----------------
Needs rebasing to use KindTy


================
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;
----------------
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.


================
Comment at: llvm/lib/Target/M68k/AsmParser/M68kAsmParser.cpp:325
 void M68kOperand::addExpr(MCInst &Inst, const MCExpr *Expr) {
+  if (Expr == nullptr) {
+    // The given displacement wasn't specified but is needed for this particular
----------------
Ditto


================
Comment at: llvm/test/MC/M68k/instructions.s:1
-; RUN: llvm-mc -triple m68k -show-encoding -motorola-integers %s | FileCheck %s
+; RUN: not llvm-mc -triple m68k -show-encoding -motorola-integers %s --show-inst-operands 2>&1 | FileCheck %s
 
----------------
Don't combine error and non-error cases in the same test, otherwise there could be errors triggered by the believed-non-error cases that you fail to catch


================
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)
----------------
This is... really weird. I don't think it makes any sense to add parsing before it can actually be used for anything.


================
Comment at: llvm/test/MC/M68k/instructions.s:64
+
+; TODO: the size & scale are not preserved here because they are not supported
+; at codegen time (yet).
----------------
That probably means you shouldn't be supporting parsing them yet. Also CodeGen its above the MC layer, not below it, so what CodeGen does is irrelevant. The issue is the lack of MC support, not a lack of CodeGen patterns.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108760