[PATCH] D126349: [TableGen] Remove code beads

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

myhsu added a comment.

Please implement `M68kInstrInfo::isPCRelRegisterOperandLegal` as suggested by the inline comment. Otherwise rest of the patch looks good :-)



================
Comment at: llvm/lib/Target/M68k/M68kInstrInfo.cpp:603
     const MachineOperand &MO) const {
-  assert(MO.isReg());
-  const auto *MI = MO.getParent();
-  const uint8_t *Beads = M68k::getMCInstrBeads(MI->getOpcode());
-  assert(*Beads);
-
-  // Only addressing mode k has (non-pc) register with PCRel
-  // So we're looking for EA Beads equal to
-  // `3Bits<011>_1Bit<1>_2Bits<11>`
-  // FIXME: There is an important caveat and two assumptions
-  // here: The caveat is that EA encoding always sit on the LSB.
-  // Where the assumptions are that if there are more than one
-  // operands, the EA encoding for the source operand always sit
-  // on the LSB. At the same time, k addressing mode can not be used
-  // on destination operand.
-  // The last assumption is kinda dirty so we need to find a way around
-  // it
-  const uint8_t EncEAk[3] = {0b011, 0b1, 0b11};
-  for (const uint8_t Pat : EncEAk) {
-    uint8_t Bead = *(Beads++);
-    if (!Bead)
-      return false;
-
-    switch (Bead & 0xF) {
-    default:
-      return false;
-    case M68kBeads::Bits1:
-    case M68kBeads::Bits2:
-    case M68kBeads::Bits3: {
-      uint8_t Val = (Bead & 0xF0) >> 4;
-      if (Val != Pat)
-        return false;
-    }
-    }
-  }
-  return true;
+  llvm_unreachable("unimplemented");
 }
----------------
We can't mark this function as unimplemented. I added this `isPCRelRegisterOperandLegal` hook when we were upstreaming the m68k backend. The story behind this was that, as the comments suggested, a logical operand that has addressing mode 'k' is consisting of 3 MachineOperand, for example:
```
(87, %pc, %a1)
```
And we mark all three MachineOperand as pc-rel due to the nature of addressing mode 'k'. But MachineVerifier was not happy about this b/c it was not expecting a register operand (i.e. `%a1` in the snippet above) to be marked as pc-rel. So this hook was added for MachineVerifier to check this very scenario. (The reason we didn't see this problem after the transition is because the new MC tests don't even get translated into MachineInstr).

One way to implement this function will be (manually) pattern matching against the opcode and the MachineOperand.

Here is a MachineInstr to test:
```
name: test_move32jk
body: |
  bb.0:
    MOV32jk $a1,  0, $a2, implicit-def $ccr
```


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kMCCodeEmitter.cpp:235
+  LLVM_DEBUG(dbgs() << "Instruction " << MCII.getName(Opcode) << "(" << Opcode
+                    << ") is using the new code emitter\n");
+  ArrayRef<uint64_t> Data(EncodedInst.getRawData(), EncodedInst.getNumWords());
----------------
we can remove this debug print


================
Comment at: llvm/utils/gn/secondary/llvm/utils/TableGen/BUILD.gn:15
     "CallingConvEmitter.cpp",
-    "CodeBeadsGen.cpp",
     "CodeEmitterGen.cpp",
----------------
Thanks for taking care GN as well!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126349