[PATCH] D147102: [M68k] Add `TRAP`, `TRAPV`, `BKPT`, `ILLEGAL` instructions

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

myhsu added inline comments.


================
Comment at: llvm/lib/Target/M68k/AsmParser/M68kAsmParser.cpp:54
   OperandMatchResultTy parseImm(OperandVector &Operands);
+  OperandMatchResultTy parseTrapImm(OperandVector &Operands);
   OperandMatchResultTy parseMemOp(OperandVector &Operands);
----------------
is this used anywhere?


================
Comment at: llvm/lib/Target/M68k/AsmParser/M68kAsmParser.cpp:181
 
+  bool isTrapImm() const;
+  bool isBkptImm() const;
----------------
could you add some simple comments for these two new functions?


================
Comment at: llvm/lib/Target/M68k/AsmParser/M68kAsmParser.cpp:359
+bool M68kOperand::isTrapImm() const {
+  int64_t imm;
+  if (!isImm() || !Expr->evaluateAsAbsolute(imm))
----------------
format: local variables have to start with uppercase (https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).


================
Comment at: llvm/lib/Target/M68k/AsmParser/M68kAsmParser.cpp:364
+  // 4 bit unsigned
+  return imm >= 0 && imm < 16;
+}
----------------
`isInt<4>(imm)` might be better, see https://llvm.org/doxygen/MathExtras_8h.html.


================
Comment at: llvm/lib/Target/M68k/M68kInstrControl.td:359-371
+def MxTrapImm : AsmOperandClass {
+  let Name = "MxtrapImm";
+  let PredicateMethod = "isTrapImm";
+  let RenderMethod = "addImmOperands";
+  let ParserMethod = "parseImm";
+}
+
----------------
`RenderMethod` and `ParserMethod` can be factored out:
```
let RenderMethod = "...", ParserMethod = "..." in {
  def MxTrapImm : AsmOperandClass {
    let Name = "MxtrapImm";
    let PredicateMethod = "isTrapImm";
  }

  def MxBkptImm : AsmOperandClass {
    let Name = "MxBkptImm";
    let PredicateMethod = "isBkptImm";
  }
}
```


================
Comment at: llvm/lib/Target/M68k/M68kInstrControl.td:360
+def MxTrapImm : AsmOperandClass {
+  let Name = "MxtrapImm";
+  let PredicateMethod = "isTrapImm";
----------------
super nit: MxTrapImm?


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

https://reviews.llvm.org/D147102