[M68k] Add `TRAP`, `TRAPV`, `BKPT`, `ILLEGAL` instructions

[ids1024]

Summary

This makes it possible to use TRAP to make Linux system calls using inline assembly for instance.

This does not validate that the 4 and 3 bit immediates taken by TRAP and BKPT are in range. But it seems the backend isn't validating the range of immediates in general.

[myhsu]

Thank you for the patch!
First, could you add MC tests? If you're not familiar, I believe test/MC/M68k/Bits/Classes/MxBTST_RR.s and test/MC/Disassembler/M68k/bits.txt are good examples.

This does not validate that the 4 and 3 bit immediates taken by TRAP and BKPT are in range. But it seems the backend isn't validating the range of immediates in general.

This can be done by adding a new immediate Operand with a different AsmOperandClass, instead of using MxImm. In this new AsmOperandClass you can use different PredicateMethod / RenderMethod / ParserMethod to validate the range.

[myhsu]

llvm/lib/Target/M68k/M68kInstrControl.td

356

nit: could you add a simple banner here like

//===------------===//
// Trap / Breakpoint
//===------------===//

[ids1024]

This does not validate that the 4 and 3 bit immediates taken by TRAP and BKPT are in range. But it seems the backend isn't validating the range of immediates in general.

This can be done by adding a new immediate Operand with a different AsmOperandClass, instead of using MxImm. In this new AsmOperandClass you can use different PredicateMethod / RenderMethod / ParserMethod to validate the range.

Ah, so this would be best done with another AsmOperandClass? And not somehow using MxImm and MxOp. I'm still trying to understand how these sorts of things are handled in tablegen.

With the current version here, it accepts __asm__ ("bkpt #6666666666666666"); without error. So Mxi8imm doesn't seem to be correctly validating that the immediate fits in 8 bits.

[myhsu]

In D147102#4238899, @ids1024 wrote:

This does not validate that the 4 and 3 bit immediates taken by TRAP and BKPT are in range. But it seems the backend isn't validating the range of immediates in general.

This can be done by adding a new immediate Operand with a different AsmOperandClass, instead of using MxImm. In this new AsmOperandClass you can use different PredicateMethod / RenderMethod / ParserMethod to validate the range.

Ah, so this would be best done with another AsmOperandClass? And not somehow using MxImm and MxOp. I'm still trying to understand how these sorts of things are handled in tablegen.

I don't think it's necessary to have a MxOp alternative because you can do something like:

// New AsmOperandClass for 3-bit vector
def MxVec3Imm : AsmOperandClass { ... }
...
let ParserMatchClass = MxVec3Imm in
def MxVec3imm  : MxOp<i8,  MxSize8,  "i">;

With the current version here, it accepts __asm__ ("bkpt #6666666666666666"); without error. So Mxi8imm doesn't seem to be correctly validating that the immediate fits in 8 bits.

Right, this is something we need to fix in another patch, thanks for pointing it out :-)

[ids1024]

Looks like the Imm union member is currently not set anywhere. It's only used in M68kOperand::print. M68kOperand::createImm sets the Expr union variant.

Edit: But it looks like Expr->evaluateAsAbsolute(imm)) works to get the immediate value to validate it. So not a problem here, but the definition of print looks wrong.

[ids1024]

This now validates that the immediates are in range. This also adds tests, and a banner at the start of this section of the .td file.

[myhsu]

llvm/lib/Target/M68k/AsmParser/M68kAsmParser.cpp

54

is this used anywhere?

181

could you add some simple comments for these two new functions?

359

format: local variables have to start with uppercase (https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly).

364

isInt<4>(imm) might be better, see https://llvm.org/doxygen/MathExtras_8h.html.

llvm/lib/Target/M68k/M68kInstrControl.td

359–371

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";
  }
}

360

super nit: MxTrapImm?

[myhsu]

Another thing I just found out is the absent of surrounding code in the patch ("Context not available."). Please use git show -U999999 ... > patch.diff if you manually generated the patch. See https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[ids1024]

Code style, comment, and unused function declaration.

[myhsu]

LGTM Thank you!
I will commit the patch on your behalf, please tell me your preferred name and email.

[ids1024]

In D147102#4243625, @myhsu wrote:

LGTM Thank you!
I will commit the patch on your behalf, please tell me your preferred name and email.

Ian Douglas Scott <[e-mail]>

llvm/lib/Target/M68k/AsmParser/M68kAsmParser.cpp

#include "M68kGenAsmMatcher.inc"
   bool parseRegisterName(MCRegister &RegNo, SMLoc Loc, StringRef RegisterName);
   OperandMatchResultTy parseRegister(MCRegister &RegNo);
 
   // Parser functions.
   void eatComma();
 
   bool isExpr();
   OperandMatchResultTy parseImm(OperandVector &Operands);
   OperandMatchResultTy parseMemOp(OperandVector &Operands);
   OperandMatchResultTy parseRegOrMoveMask(OperandVector &Operands);
 
 public:
   M68kAsmParser(const MCSubtargetInfo &STI, MCAsmParser &Parser,
                 const MCInstrInfo &MII, const MCTargetOptions &Options)
       : MCTargetAsmParser(Options, STI, MII), STI(STI), Parser(Parser) {
     MCAsmParserExtension::Initialize(Parser);
     MRI = getContext().getRegisterInfo();
public:
 
   // Imm
   bool isImm() const override;
   void addImmOperands(MCInst &Inst, unsigned N) const;
 
   static std::unique_ptr<M68kOperand> createImm(const MCExpr *Expr, SMLoc Start,
                                                 SMLoc End);
 
+  // Imm for TRAP instruction
+  bool isTrapImm() const;
+  // Imm for BKPT instruction
+  bool isBkptImm() const;
+
   // MoveMask
   bool isMoveMask() const;
   void addMoveMaskOperands(MCInst &Inst, unsigned N) const;
 
   // Addr
   bool isAddr() const;
   bool isAddr8() const { return isAddrN<8>(); }
   bool isAddr16() const { return isAddrN<16>(); }

 
 std::unique_ptr<M68kOperand> M68kOperand::createImm(const MCExpr *Expr,
                                                     SMLoc Start, SMLoc End) {
   auto Op = std::make_unique<M68kOperand>(KindTy::Imm, Start, End);
   Op->Expr = Expr;
   return Op;
 }
 
+bool M68kOperand::isTrapImm() const {
+  int64_t Value;
+  if (!isImm() || !Expr->evaluateAsAbsolute(Value))
+    return false;
+
+  return isUInt<4>(Value);
+}
+
+bool M68kOperand::isBkptImm() const {
+  int64_t Value;
+  if (!isImm() || !Expr->evaluateAsAbsolute(Value))
+    return false;
+
+  return isUInt<3>(Value);
+}
+
 // MoveMask
 bool M68kOperand::isMoveMask() const {
   if (!isMemOp())
     return false;
 
   if (MemOp.Op == M68kMemOp::Kind::RegMask)
     return true;
 

llvm/lib/Target/M68k/M68kInstrControl.td

 def : Pat<(sub MxXRD32:$op, (and (i32 (MxSetCC_C MxCONDcs, CCR)), 1)),
           (SUBX32dd MxDRD32:$op, (MOV32ri 0))>;
 
 // (sub OP, SETCC_CARRY) -> (addx OP, (move 0))
 def : Pat<(sub MxDRD8:$op, (i8 (MxSetCC_C MxCONDcs, CCR))),
           (ADDX8dd MxDRD8:$op, (MOV8di 0))>;
 def : Pat<(sub MxXRD32:$op, (i32 (MxSetCC_C MxCONDcs, CCR))),
           (ADDX32dd MxDRD32:$op, (MOV32ri 0))>;
+
+//===------------===//
+// Trap / Breakpoint
+//===------------===//
+
+let RenderMethod = "addImmOperands", ParserMethod = "parseImm" in {
+  def MxTrapImm : AsmOperandClass {
+    let Name = "MxTrapImm";
+    let PredicateMethod = "isTrapImm";
+  }
+
+  def MxBkptImm : AsmOperandClass {
+    let Name = "MxBkptImm";
+    let PredicateMethod = "isBkptImm";
+  }
+}
+
+let ParserMatchClass = MxTrapImm in
+def MxTrapimm : MxOp<i8,  MxSize8,  "i">;
+
+let ParserMatchClass = MxBkptImm in
+def MxBkptimm : MxOp<i8,  MxSize8,  "i">;
+
+def TRAP : MxInst<(outs), (ins MxTrapimm:$vect), "trap\t$vect", []> {
+  let Inst = (descend 0b0100, 0b1110, 0b0100, (operand "$vect", 4));
+}
+
+def TRAPV : MxInst<(outs), (ins), "trapv", []> {
+  let Inst = (descend 0b0100, 0b1110, 0b0111, 0b0110);
+}
+
+def BKPT : MxInst<(outs), (ins MxBkptimm:$vect), "bkpt\t$vect", []> {
+  let Inst = (descend 0b0100, 0b1000, 0b0100, 0b1 , (operand "$vect", 3));
+}
+
+def ILLEGAL : MxInst<(outs), (ins), "illegal", []> {
+  let Inst = (descend 0b0100, 0b1010, 0b1111, 0b1100);
+}

llvm/test/MC/Disassembler/M68k/trap-break.txt

+# RUN: llvm-mc -disassemble -triple m68k %s | FileCheck %s
+
+; CHECK: trap #13
+0x4e 0x4d
+# CHECK: bkpt #7
+0x48 0x4f
+# CHECK: trapv
+0x4e 0x76
+# CHECK: illegal
+0x4a 0xfc
+

llvm/test/MC/M68k/Control/trap-break.s

+; RUN: llvm-mc -triple=m68k -show-encoding %s | FileCheck %s
+
+; CHECK:      trap #13
+; CHECK-SAME: encoding: [0x4e,0x4d]
+trap  #13
+; CHECK:      bkpt #7
+; CHECK-SAME: encoding: [0x48,0x4f]
+bkpt  #7
+; CHECK:      trapv
+; CHECK-SAME: encoding: [0x4e,0x76]
+trapv
+; CHECK:      illegal
+; CHECK-SAME: encoding: [0x4a,0xfc]
+illegal