[PATCH] D50784: [TableGen] Add Instruction custom byte sequence emission

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: utils/TableGen/CodeBeadsGen.cpp:13-14
+/// defined for an instruction. They are used to pass very target specific
+/// binary information directly from a tablegen file, e.g. instruction
+/// encoding.
+///
----------------
m4yers wrote:
> efriedma wrote:
> > lebedev.ri wrote:
> > > Ignorant question: but surely the problem of storing instruction encoding in tablegen has already arose?
> > > How does this solution differ from the existing practice?
> > Most other targets use CodeEmitterGen to generate a function getBinaryCodeForInstr.  This is nice, but it only works for targets with fixed-length instructions of length 4 or less.
> > 
> > x86 doesn't store instruction encodings in TableGen; instead, it uses a hand-written implementation of X86MCCodeEmitter::encodeInstruction, driven by flags in the instruction description.
> It allows to pass encoding strings of various length. Also it is semantically different from TSFlags and I thought it'd be a poor choice to extend its size and reuse.
(At the very least this should be explained here in the comment.)


================
Comment at: utils/TableGen/CodeBeadsGen.cpp:26
+
+#include <map>
+#include <string>
----------------
map and vector are unused it seems.


================
Comment at: utils/TableGen/CodeBeadsGen.cpp:52-54
+  // Current limit of beads is 192 bits
+  constexpr unsigned BeadsLength = 192;
+  constexpr unsigned BeadsNumber = BeadsLength / BeadSize;
----------------
I wonder if it would be more straight-forward to go the other way around?
You wouldn't need to manually worry to specify multiple-of-BeadSize-bits value.


================
Comment at: utils/TableGen/CodeBeadsGen.cpp:101
+        if (BitInit *B = dyn_cast<BitInit>(Beads->getBit(i))) {
+          Value |= (static_cast<unsigned>(B->getValue()) << Shift);
+        } else {
----------------
I'm surprised there is no abstraction for this in LLVM already.
Maybe i'm looking in the wrong places.


Repository:
  rL LLVM

https://reviews.llvm.org/D50784