[M68k] Adopt VarLenCodeEmitter for control instructions

[0x59616e]

Summary

Refactor the instructions in M68kInstrControl.td to use the VarLenCodeEmitter.

This patch is tested by the existing test cases.

[0x59616e]

update diff

[ricky26]

Thanks for this, I'm really enjoying the new syntax, it's so much easier to follow. I'm a little bit unsure about the test change.

llvm/lib/Target/M68k/M68kInstrControl.td

47

nit: Any reason you've split this up by byte? Generally the manuals I've read have it as one string of bits (I wonder if the bit syntax supports underscores).

llvm/test/MC/M68k/Control/call-pc-rel.s

5

It looks like this just negates the whole test?

[0x59616e]

llvm/lib/Target/M68k/M68kInstrControl.td

47

Just for readability.

And...no, the bit syntax does not support underscore.

llvm/test/MC/M68k/Control/call-pc-rel.s

5

This just negates the second test, which needs disassembler support.

The first test is still online.

[ricky26]

llvm/test/MC/M68k/Control/call-pc-rel.s

5

Is it worth adding a TODO: here? I don't want it to get lost and I don't think it would be immediately obvious to me that it was disabled.

[0x59616e]

llvm/test/MC/M68k/Control/call-pc-rel.s

5

You have a point. Thanks ;)

[0x59616e]

address feedback

[ricky26]

LGTM. Might be worth waiting for @myhsu to review, but I don't think there's anything controversial. Thanks! 😄

[myhsu]

Please also XFAIL test/MC/Disassembler/M68k/control.txt

[myhsu]

Thanks for the patch :-)
I only have minor comments (and please remember to XFAIL control.txt per my previous comment)

llvm/lib/Target/M68k/M68kInstrControl.td

169

I'm fine with the syntax here, however in the future if you want to avoid supplying disp_16_32 with empty (ascend) for 16/32 bits variants, you can use the conditional bang operator !cond like this:

class MxBcc<string cc, Operand TARGET, int SIZE>... {
  ...
  let Inst = !cond(
      !eq(SIZE, 8):   /*encoding for Bcc8*/,
      !eq(SIZE, 16):  /*encoding for Bcc16*/,
      !eq(SIZE, 32):  /*encoding for Bcc32*/
  );
}

187–189

nit formatting

191–193

220–224

223–224

[0x59616e]

address feedback

[0x59616e]

llvm/lib/Target/M68k/M68kInstrControl.td

169

I leave a FIXME here ;)

[myhsu]

LGTM
please address my last comment before commit though

llvm/lib/Target/M68k/M68kInstrControl.td

167

"we can we conditional"?

[0x59616e]

address feedback

[0x59616e]

llvm/lib/Target/M68k/M68kInstrControl.td

167

my bad

[0x59616e]

rebase

[0x59616e]

OK I think all the feedbacks are addressed properly.

Thanks @ricky26 @myhsu ;)

llvm/lib/Target/M68k/M68kInstrControl.td

 //===-- M68kInstrControl.td - Control Flow Instructions ----*- tablegen -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
 /// This file describes the M68k jump, return, call, and related instructions.
 /// Here is the current status of the file:
 ///
 ///  Machine:
 ///
-///       BRA   [x]     BSR  [ ]     Bcc [ ]     DBcc [ ]     FBcc [ ]
+///       BRA   [x]     BSR  [ ]     Bcc [~]     DBcc [ ]     FBcc [ ]
 ///       FDBcc [ ]     FNOP [ ]     FPn [ ]     FScc [ ]     FTST [ ]
 ///       JMP   [~]     JSR  [x]     NOP [x]     RTD  [!]     RTR  [ ]
-///       RTS   [x]     Scc  [x]     TST [ ]
+///       RTS   [x]     Scc  [~]     TST [ ]
 ///
 ///  Pseudo:
 ///
 ///          RET [x]
 ///    TCRETURNj [x]   TCRETURNq [x]
 ///     TAILJMPj [x]    TAILJMPq [x]
 ///
 ///  Map:

 ///
 //===----------------------------------------------------------------------===//
 
 //===----------------------------------------------------------------------===//
 // NOP
 //===----------------------------------------------------------------------===//
 
 let hasSideEffects = 0 in {
-  def NOP : MxInst<(outs), (ins), "nop", [], MxEncFixed<0x4E71>>;
+  def NOP : MxInst<(outs), (ins), "nop", []> {
+    let Inst = (descend 0b0100, 0b1110, 0b0111, 0b0001);
+  }
 }
 
 
 //===----------------------------------------------------------------------===//
 // Conditions
 //===----------------------------------------------------------------------===//
 
 /// CC—Carry clear      GE—Greater than or equal
 /// LS—Lower or same    PL—Plus
 /// CS—Carry set        GT—Greater than
 /// LT—Less than        T—Always true*
 /// EQ—Equal            HI—Higher
 /// MI—Minus            VC—Overflow clear
 /// F—Never true*       LE—Less than or equal
 /// NE—Not equal        VS—Overflow set
 ///
 /// *Not applicable to the Bcc instructions.
-def MxCCt  : MxBead4Bits<0b0000>;
-def MxCCf  : MxBead4Bits<0b0001>;
-def MxCChi : MxBead4Bits<0b0010>;
-def MxCCls : MxBead4Bits<0b0011>;
-def MxCCcc : MxBead4Bits<0b0100>;
-def MxCCcs : MxBead4Bits<0b0101>;
-def MxCCne : MxBead4Bits<0b0110>;
-def MxCCeq : MxBead4Bits<0b0111>;
-def MxCCvc : MxBead4Bits<0b1000>;
-def MxCCvs : MxBead4Bits<0b1001>;
-def MxCCpl : MxBead4Bits<0b1010>;
-def MxCCmi : MxBead4Bits<0b1011>;
-def MxCCge : MxBead4Bits<0b1100>;
-def MxCClt : MxBead4Bits<0b1101>;
-def MxCCgt : MxBead4Bits<0b1110>;
-def MxCCle : MxBead4Bits<0b1111>;
+class MxEncCondOp<bits<4> cond> {
+  dag Value = (descend cond);
+}
+
+def MxCCt  : MxEncCondOp<0b0000>;
+def MxCCf  : MxEncCondOp<0b0001>;
+def MxCChi : MxEncCondOp<0b0010>;
+def MxCCls : MxEncCondOp<0b0011>;
+def MxCCcc : MxEncCondOp<0b0100>;
+def MxCCcs : MxEncCondOp<0b0101>;
+def MxCCne : MxEncCondOp<0b0110>;
+def MxCCeq : MxEncCondOp<0b0111>;
+def MxCCvc : MxEncCondOp<0b1000>;
+def MxCCvs : MxEncCondOp<0b1001>;
+def MxCCpl : MxEncCondOp<0b1010>;
+def MxCCmi : MxEncCondOp<0b1011>;
+def MxCCge : MxEncCondOp<0b1100>;
+def MxCClt : MxEncCondOp<0b1101>;
+def MxCCgt : MxEncCondOp<0b1110>;
+def MxCCle : MxEncCondOp<0b1111>;
+
+
 
 /// --------------------------------+---------+---------
 ///  F  E  D  C | B  A  9  8 | 7  6 | 5  4  3 | 2  1  0
 /// --------------------------------+---------+---------
 ///  0  1  0  1 | CONDITION  | 1  1 |   MODE  |   REG
 /// ----------------------------------------------------
-class MxSccEncoding<MxEncEA EA, MxEncExt EXT, MxBead4Bits CC>
-    : MxEncoding<EA.Reg, EA.DA, EA.Mode, MxBead2Bits<0b11>, CC, MxBead4Bits<0b0101>,
-                 EXT.Imm, EXT.B8, EXT.Scale, EXT.WL, EXT.DAReg>;
 
 let Uses = [CCR] in {
 class MxSccR<string CC>
     : MxInst<(outs MxDRD8:$dst), (ins), "s"#CC#"\t$dst",
-             [(set i8:$dst, (MxSetCC !cast<PatLeaf>("MxCOND"#CC), CCR))],
-             MxSccEncoding<MxEncEAd_0, MxExtEmpty,
-                           !cast<MxBead4Bits>("MxCC"#CC)>>;
+             [(set i8:$dst, (MxSetCC !cast<PatLeaf>("MxCOND"#CC), CCR))]> {
+  let Inst = (descend 0b0101, !cast<MxEncCondOp>("MxCC"#CC).Value, 0b11,
+              /*MODE without last bit*/0b00,
+              /*REGISTER prefixed with D/A bit*/(operand "$dst", 4));
+}
 
-class MxSccM<string CC, MxOperand MEMOpd, ComplexPattern MEMPat,
-             MxEncEA EA, MxEncExt EXT>
+class MxSccM<string CC, MxOperand MEMOpd, ComplexPattern MEMPat, MxEncMemOp DST_ENC>
     : MxInst<(outs), (ins MEMOpd:$dst), "s"#CC#"\t$dst",
-             [(store (MxSetCC !cast<PatLeaf>("MxCOND"#CC), CCR), MEMPat:$dst)],
-             MxSccEncoding<EA, EXT, !cast<MxBead4Bits>("MxCC"#CC)>>;
+             [(store (MxSetCC !cast<PatLeaf>("MxCOND"#CC), CCR), MEMPat:$dst)]> {
+  let Inst =
+    (ascend
+      (descend 0b0101, !cast<MxEncCondOp>("MxCC"#CC).Value, 0b11, DST_ENC.EA),
+      DST_ENC.Supplement
+    );
+}
 }
 
 foreach cc = [ "cc", "ls", "lt", "eq", "mi", "f", "ne", "ge",
                "cs", "pl", "gt", "t", "hi", "vc", "le", "vs"] in {
 def SET#"d8"#cc : MxSccR<cc>;
-def SET#"j8"#cc : MxSccM<cc, MxType8.JOp, MxType8.JPat, MxEncEAj_0, MxExtEmpty>;
-def SET#"p8"#cc : MxSccM<cc, MxType8.POp, MxType8.PPat, MxEncEAp_0, MxExtI16_0>;
+def SET#"j8"#cc : MxSccM<cc, MxType8.JOp, MxType8.JPat, MxEncAddrMode_j<"dst">>;
+def SET#"p8"#cc : MxSccM<cc, MxType8.POp, MxType8.PPat, MxEncAddrMode_p<"dst">>;
 }
 
 //===----------------------------------------------------------------------===//
 // Jumps
 //===----------------------------------------------------------------------===//
 
 ///------------------------------+---------+---------
 /// F  E  D  C  B  A  9  8  7  6 | 5  4  3 | 2  1  0
 ///------------------------------+---------+---------
 /// 0  1  0  0  1  1  1  0  1  1 |  MODE   |   REG
 ///------------------------------+---------+---------
 let isBranch = 1, isTerminator = 1, isBarrier = 1, isIndirectBranch = 1 in
-class MxJMP<MxOperand LOCOp, MxEncEA EA, MxEncExt EXT>
-    : MxInst<(outs), (ins LOCOp:$dst), "jmp\t$dst", [(brind iPTR:$dst)],
-             MxEncoding<EA.Reg, EA.DA, EA.Mode, MxBead2Bits<0b11>,
-                        MxBead4Bits<0b1110>, MxBead4Bits<0b0100>,
-                        EXT.Imm, EXT.B8, EXT.Scale, EXT.WL, EXT.DAReg>>;
+class MxJMP<MxOperand LOCOp, MxEncMemOp DST_ENC>
+    : MxInst<(outs), (ins LOCOp:$dst), "jmp\t$dst", [(brind iPTR:$dst)]> {
+  let Inst =
+    (ascend
+      (descend 0b0100, 0b1110, 0b11, DST_ENC.EA),
+      DST_ENC.Supplement
+    );
+}
 
-def JMP32j : MxJMP<MxARI32, MxEncEAj_0, MxExtEmpty>;
+def JMP32j : MxJMP<MxARI32, MxEncAddrMode_j<"dst">>;
 
 
 // FIXME Support 16 bit indirect jump.
 // Currently M68k does not allow 16 bit indirect jumps use sext operands
 // def JMP16r     : MxInst<(outs), (ins M68k_ARI16:$dst),
 //                             "jmp\t$dst",
 //                             [(brind AR16:$dst)]>;
 
 //===----------------------------------------------------------------------===//
 // Branches
 //===----------------------------------------------------------------------===//
 
 /// --------------------------------------------------
 ///  F  E  D  C | B  A  9  8 | 7  6  5  4  3  2  1  0
 /// --------------------------------------------------
 ///  0  1  1  0 | CONDITION |   8-BIT DISPLACEMENT
 /// --------------------------------------------------
 ///  16-BIT DISPLACEMENT IF 8-BIT DISPLACEMENT = $00
 /// --------------------------------------------------
 ///  32-BIT DISPLACEMENT IF 8-BIT DISPLACEMENT = $FF
 /// --------------------------------------------------
 let isBranch = 1, isTerminator = 1, Uses = [CCR] in
-class MxBcc<string cc, Operand TARGET, MxEncoding ENC = MxEncEmpty>
-    : MxInst<(outs), (ins TARGET:$dst), "b"#cc#"\t$dst", [], ENC>;
+class MxBcc<string cc, Operand TARGET, dag disp_8, dag disp_16_32>
+    : MxInst<(outs), (ins TARGET:$dst), "b"#cc#"\t$dst", []> {
+  // FIXME: If we want to avoid supplying disp_16_32 with empty
+  //        (ascend) for 16/32 bits variants, we can use conditional
+  //        bang operator like this:
+  //        ```
+  //        class MxBcc<string cc, Operand TARGET, int SIZE>
+  //        ...
+  //        let Inst = !cond(
+  //            !eq(SIZE, 8):   /* encoding for Bcc8  */
+  //            !eq(SIZE, 16):  /* encoding for Bcc16 */
+  //            !eq(SIZE, 32):  /* encoding for Bcc32 */
+  //        );
+  let Inst =
+      (ascend
+        (descend 0b0110, !cast<MxEncCondOp>("MxCC"#cc).Value, disp_8),
+        disp_16_32
+      );
+}
 
 foreach cc = [ "cc", "ls", "lt", "eq", "mi", "ne", "ge",
                "cs", "pl", "gt", "hi", "vc", "le", "vs"] in {
   def B#cc#"8"
     : MxBcc<cc, MxBrTarget8,
-            MxEncoding<MxBead8Disp<0>,
-                       !cast<MxBead4Bits>("MxCC"#cc), MxBead4Bits<0x6>>>;
+            (operand "$dst", 8, (encoder "encodePCRelImm<8>")), (ascend)>;
+
   def B#cc#"16"
-    : MxBcc<cc, MxBrTarget16,
-            MxEncoding<MxBead4Bits<0x0>,
-                       MxBead4Bits<0x0>, !cast<MxBead4Bits>("MxCC"#cc),
-                       MxBead4Bits<0x6>, MxBead16Imm<0>>>;
+    : MxBcc<cc, MxBrTarget16, (descend 0b0000, 0b0000),
+            (operand "$dst", 16, (encoder "encodePCRelImm<16>"))>;
 }
 
 foreach cc = [ "cc", "ls", "lt", "eq", "mi", "ne", "ge",
                "cs", "pl", "gt", "hi", "vc", "le", "vs"] in {
 def : Pat<(MxBrCond bb:$target, !cast<PatLeaf>("MxCOND"#cc), CCR),
           (!cast<Instruction>("B"#cc#"8") MxBrTarget8:$target)>;
 }
 
 /// -------------------------------------------------
 ///  F  E  D  C  B  A  9  8 | 7  6  5  4  3  2  1  0
 /// -------------------------------------------------
 ///  0  1  1  0  0  0  0  0 |   8-BIT DISPLACEMENT
 /// -------------------------------------------------
 ///  16-BIT DISPLACEMENT IF 8-BIT DISPLACEMENT = $00
 /// -------------------------------------------------
 ///  32-BIT DISPLACEMENT IF 8-BIT DISPLACEMENT = $FF
 /// -------------------------------------------------
-let isBranch = 1, isTerminator = 1, isBarrier=1 in
-class MxBra<Operand TARGET, MxEncoding ENC = MxEncEmpty>
-    : MxInst<(outs), (ins TARGET:$dst), "bra\t$dst", [], ENC>;
+let isBranch = 1, isTerminator = 1, isBarrier = 1 in
+class MxBra<Operand TARGET, dag disp_8, dag disp_16_32>
+    : MxInst<(outs), (ins TARGET:$dst), "bra\t$dst", []> {
+  let Inst =
+    (ascend
+      (descend 0b0110, 0b0000, disp_8),
+      disp_16_32
+    );
+}
 
 def BRA8  : MxBra<MxBrTarget8,
-                  MxEncoding<MxBead8Disp<0>, MxBead4Bits<0x0>,
-                             MxBead4Bits<0x6>>>;
-def BRA16 : MxBra<MxBrTarget16,
-                  MxEncoding<MxBead4Bits<0x0>, MxBead4Bits<0x0>,
-                             MxBead4Bits<0x0>, MxBead4Bits<0x6>,
-                             MxBead16Imm<0>>>;
+                  (operand "$dst", 8, (encoder "encodePCRelImm<8>")), (ascend)>;
+
+def BRA16 : MxBra<MxBrTarget16, (descend 0b0000, 0b0000),
+                  (operand "$dst", 16, (encoder "encodePCRelImm<16>"))>;
 
 def : Pat<(br bb:$target), (BRA8 MxBrTarget8:$target)>;
 
 
 //===----------------------------------------------------------------------===//
 // Call
 //===----------------------------------------------------------------------===//
 
 // All calls clobber the non-callee saved registers. %SP is marked as
 // a use to prevent stack-pointer assignments that appear immediately
 // before calls from potentially appearing dead. Uses for argument
 // registers are added manually.
 let Uses = [SP] in
 let isCall = 1 in
 ///------------------------------+---------+---------
 /// F  E  D  C  B  A  9  8  7  6 | 5  4  3 | 2  1  0
 ///------------------------------+---------+---------
 /// 0  1  0  0  1  1  1  0  1  0 |  MODE   |   REG
 ///------------------------------+---------+---------
-class MxCall<MxOperand LOCOp, MxEncEA EA, MxEncExt EXT>
-    : MxInst<(outs), (ins LOCOp:$dst), "jsr\t$dst", [],
-             MxEncoding<EA.Reg, EA.DA, EA.Mode, MxBead2Bits<0b10>,
-                        MxBead4Bits<0b1110>, MxBead4Bits<0b0100>,
-                        EXT.Imm, EXT.B8, EXT.Scale, EXT.WL, EXT.DAReg>>;
-
-def CALLk : MxCall<MxPCI32, MxEncEAk,   MxExtBrief_0>;
-def CALLq : MxCall<MxPCD32, MxEncEAq,   MxExtI16_0>;
-def CALLb : MxCall<MxAL32,  MxEncEAb,   MxExtI32_0>;
-def CALLj : MxCall<MxARI32, MxEncEAj_0, MxExtEmpty>;
+class MxCall<MxOperand LOCOp, MxEncMemOp DST_ENC>
+    : MxInst<(outs), (ins LOCOp:$dst), "jsr\t$dst", []> {
+  let Inst =
+    (ascend
+      (descend 0b0100, 0b1110, 0b10, DST_ENC.EA),
+      DST_ENC.Supplement
+    );
+}
+
+def CALLk : MxCall<MxPCI32, MxEncAddrMode_k<"dst">>;
+def CALLq : MxCall<MxPCD32, MxEncAddrMode_q<"dst">>;
+def CALLb : MxCall<MxAL32,  MxEncAddrMode_abs<"dst", true>>;
+def CALLj : MxCall<MxARI32, MxEncAddrMode_j<"dst">>;
 
 multiclass CallPat<MxCall callOp, Predicate pred> {
   let Predicates = [pred] in {
     def : Pat<(MxCall (i32 tglobaladdr:$dst)),  (callOp tglobaladdr:$dst)>;
     def : Pat<(MxCall (i32 texternalsym:$dst)), (callOp texternalsym:$dst)>;
     def : Pat<(MxCall (i32 imm:$dst)),          (callOp imm:$dst)>;
   }
 }

 //===----------------------------------------------------------------------===//
 // Return
 //===----------------------------------------------------------------------===//
 
 // TODO Implement LINK/UNLK
 
 let isTerminator = 1, isReturn = 1, isBarrier = 1, hasCtrlDep = 1 in {
 
-def RTS : MxInst<(outs), (ins), "rts", [], MxEncFixed<0x4E75>>;
+def RTS : MxInst<(outs), (ins), "rts", []> {
+  let Inst = (descend 0b0100, 0b1110, 0b0111, 0b0101);
+}
 
 let isCodeGenOnly = 1 in
 def RET : MxPseudo<(outs), (ins i32imm:$adj, variable_ops),
                    [(MxRet timm:$adj)]>;
 } // isTerminator = 1, isReturn = 1, isBarrier = 1, hasCtrlDep = 1
 
 //===----------------------------------------------------------------------===//
 // SETCC_C Patterns

llvm/test/MC/Disassembler/M68k/control.txt

 # RUN: llvm-mc -disassemble -triple m68k %s | FileCheck %s
+# XFAIL: *
 
 # CHECK: bra $0
 0x60 0x00 0x00 0x00
 # CHECK: jsr $0
 0x4e 0xb9 0x00 0x00 0x00 0x00
 # CHECK: rts
 0x4e 0x75
 # CHECK: seq %d0
 0x57 0xc0
 # CHECK: sgt %d0
 0x5e 0xc0
 # CHECK: nop
 0x4e 0x71

llvm/test/MC/M68k/Control/call-pc-rel.s

 ; RUN: llvm-mc -triple=m68k -show-encoding %s | FileCheck %s
+; TODO: We negates the second test since it needs disassembler support
+;       Revert it back when we have that.
 ; RUN: llvm-mc -triple=m68k -filetype=obj < %s | \
-; RUN:     llvm-objdump -d - | FileCheck --check-prefix=CHECK-OBJ %s
+; RUN:     llvm-objdump -d - | not FileCheck --check-prefix=CHECK-OBJ %s
 
 ; CHECK-LABEL: BACKWARD:
 BACKWARD:
   ; CHECK:      nop
   ; CHECK-SAME: encoding: [0x4e,0x71]
   nop
   ; CHECK:      rts
   ; CHECK-SAME: encoding: [0x4e,0x75]