[llvm] [Hexagon] Adding tests, TableGen entries, and code for emitting add opcode.

[colinl]

Summary

Updating .td files, updating some function names and adding functionality for emitting add opcode.

[• rafael]

unittests/MC/Hexagon/HexagonMCCodeEmitterTest.cpp

57

Why can't you test this with llvm-mc? It seems a much better way of testing this then by manually creating instructions in a unit test.

[colinl]

unittests/MC/Hexagon/HexagonMCCodeEmitterTest.cpp

57

The plan was to get the integrated assember updated before putting in the assembler parser which is non-existent at the moment.

We thought it could also help narrowing down issues by testing the code emission in isolation.

What do you think?

[sidneym]

unittests/MC/Hexagon/HexagonMCCodeEmitterTest.cpp

57

That is right. The hexagon assembly language syntax is somewhat C-like and violates some of the assumptions the MC assembler makes.

We want to get the TD files updated with encoding bits and update the rest of MCTargetDesc so that object files can be directly emitted. Using this test framework allows us to add the encoding bits and have a directed test for each instruction.

[• rafael]

That is right. The hexagon assembly language syntax is somewhat C-like and violates some of the assumptions the MC assembler makes.

We want to get the TD files updated with encoding bits and update the rest of MCTargetDesc so that object files can be directly emitted. Using this test framework allows us to add the encoding bits and have a directed test for each instruction.

Can you test the encoding with disassembler tests? Even if is printing
in a sudo-hexagon syntax for now that should be sufficient for
testing. I am afraid that using gtest for instruction encoding might
get even messier then the clang-format tests.

Cheers,
Rafael

[colinl]

In D5624#12, @rafael wrote:

That is right. The hexagon assembly language syntax is somewhat C-like and violates some of the assumptions the MC assembler makes.

We want to get the TD files updated with encoding bits and update the rest of MCTargetDesc so that object files can be directly emitted. Using this test framework allows us to add the encoding bits and have a directed test for each instruction.

Can you test the encoding with disassembler tests? Even if is printing
in a sudo-hexagon syntax for now that should be sufficient for
testing. I am afraid that using gtest for instruction encoding might
get even messier then the clang-format tests.

Cheers,
Rafael

Disassembler tests might be possible, I hadn't considered that before. We had thought the gtests would be sufficient since the inputs and outputs shouldn't vary as time goes on since the instructions and their encoding are tied to existing hardware.

What types of problems with clang-format tests are we looking to avoid? We have a fair number of instructions with running gtests we have pretty much prepped for upstreaming and haven't encountered any problems but we want to avoid something maybe we haven't thought of.

[• rafael]

Disassembler tests might be possible, I hadn't considered that before. We had thought the gtests would be sufficient since the inputs and outputs shouldn't vary as time goes on since the instructions and their encoding are tied to existing hardware.

What types of problems with clang-format tests are we looking to avoid? We have a fair number of instructions with running gtests we have pretty much prepped for upstreaming and haven't encountered any problems but we want to avoid something maybe we haven't thought of.

With gtest you end up with a massive .cpp files which are hard to read
and hard to check against a native tool. IMHO it is also way too easy
to simply write gtest tests that just check that copy and paste is
working :-(

If you checkin a .o and test disassembly, things are much better IMHO:

• You can check the .o agains a native tool or even the CPU.
• You get an end to end test.
• FileCheck error makes it very easy to see what instruction failed to

disassemble.

Cheers,
Rafael

[colinl]

In D5624#14, @rafael wrote:

Disassembler tests might be possible, I hadn't considered that before. We had thought the gtests would be sufficient since the inputs and outputs shouldn't vary as time goes on since the instructions and their encoding are tied to existing hardware.

What types of problems with clang-format tests are we looking to avoid? We have a fair number of instructions with running gtests we have pretty much prepped for upstreaming and haven't encountered any problems but we want to avoid something maybe we haven't thought of.

With gtest you end up with a massive .cpp files which are hard to read
and hard to check against a native tool. IMHO it is also way too easy
to simply write gtest tests that just check that copy and paste is
working :-(

If you checkin a .o and test disassembly, things are much better IMHO:

• You can check the .o agains a native tool or even the CPU.
• You get an end to end test.
• FileCheck error makes it very easy to see what instruction failed to

disassemble.

Cheers,
Rafael

Test quality is a good point. What we've been doing internally to keep the test quality high is using them as our source-of-truth for correct encoding. We thought unit-tests would be more of a library-centric approach rather than a toolchain-centric with files or calls out to other tools in the system.

Do you think we can push up a few more of these unit tests in the next week or so and see how it shapes up or is this more of a blocking issue? I think in the end you'll be happy with the quality.

[• rafael]

Test quality is a good point. What we've been doing internally to keep the test quality high is using them as our source-of-truth for correct encoding. We thought unit-tests would be more of a library-centric approach rather than a toolchain-centric with files or calls out to other tools in the system.

Do you think we can push up a few more of these unit tests in the next week or so and see how it shapes up or is this more of a blocking issue? I think in the end you'll be happy with the quality.

I really don't want to have gtests for encoding in the long run. I
could see using them as a *short* term compromise, but It looks like
disassembler tests should do the job, why not use them?

Cheers,
Rafael

[colinl]

In D5624#16, @rafael wrote:

Test quality is a good point. What we've been doing internally to keep the test quality high is using them as our source-of-truth for correct encoding. We thought unit-tests would be more of a library-centric approach rather than a toolchain-centric with files or calls out to other tools in the system.

Do you think we can push up a few more of these unit tests in the next week or so and see how it shapes up or is this more of a blocking issue? I think in the end you'll be happy with the quality.

I really don't want to have gtests for encoding in the long run. I
could see using them as a *short* term compromise, but It looks like
disassembler tests should do the job, why not use them?

Cheers,
Rafael

With uploading .o files and disassembling them as a test we thought it would be difficult to document a process on how the public can regenerate the .o files if needed or add more in the future if we encounter a bug. We had thought adding unit tests would be more in line with the goals of being a library rather than a file-mover as with our implementation in a different toolchain.

One thing we could do to address the concern about a huge cpp file would be to break the files up in to logical units based on the instruction category in the documentation. We would have to make some changes internally but if that helps with organization we could make that change. Would you want us to do that when submitting follow up patches with tests?

[• rafael]

With uploading .o files and disassembling them as a test we thought it would be difficult to document a process on how the public can regenerate the .o files if needed or add more in the future if we encounter a bug. We had thought adding unit tests would be more in line with the goals of being a library rather than a file-mover as with our implementation in a different toolchain.

One thing we could do to address the concern about a huge cpp file would be to break the files up in to logical units based on the instruction category in the documentation. We would have to make some changes internally but if that helps with organization we could make that change. Would you want us to do that when submitting follow up patches with tests?

Even if regenerating the .o files is not trivial before llvm-mc
getting support of the Hexagon assembly, it is probably still way more
maintainable to have

CHECK: some instruction in pseudo hexagon assembly syntax

then

59 {
60 llvm::raw_string_ostream stream(str);
61 llvm::HexagonMCInst inst(llvm::Hexagon::A2_add);
62 inst.addOperand(llvm::MCOperand::CreateReg(llvm::Hexagon::R17));
63 inst.addOperand(llvm::MCOperand::CreateReg(llvm::Hexagon::R21));
64 inst.addOperand(llvm::MCOperand::CreateReg(llvm::Hexagon::R31));
65 Emitter.Emitter->EncodeInstruction(inst, stream, EmptyFixups,
66 *Emitter.Subtarget);
67 }
68 ASSERT_EQ(4, str.size());
69 EXPECT_EQ(0x11, static_cast<uint8_t>(str[0]));
70 EXPECT_EQ(0xdf, static_cast<uint8_t>(str[1]));
71 EXPECT_EQ(0x15, static_cast<uint8_t>(str[2]));
72 EXPECT_EQ(0xf3, static_cast<uint8_t>(str[3]))

Using gtests is reasonable for things that are not otherwise exposed
in a tool. No tool in LLVM directly exposes a map, so a gtest is
perfect for testing DenseMap. In the case of encoding, it is exposed
via the assembler/disassembler.

Think of it as a special case of code refactoring. Instead of writing
a c++ test for every instruction, you can use a single program
(llvm-mc) and multiple inputs (instructions in a .o/.s).

Cheers,
Rafael

[colinl]

I think I understand the reason for the request now. It sounds like you're saying the translation between assembly and machine code is most efficiently and compactly represented by assembling or disassembling and since we have a tool for that, it would be nice to use it. That logic makes sense.

I factored the unit test and it seems like they can be represented in a compact form which might satisfy the terseness request.

If this change is still rejected Sid is investigating disassembler patches though it make take a few to get all the required parts in place.

[colinl]

Adding tests using llvm-mc and removing unit tests.

lib/Target/Hexagon/Disassembler/HexagonDisassembler.cpp

 };
 }
 
+static const uint16_t IntRegDecoderTable[] = {
+    Hexagon::R0,  Hexagon::R1,  Hexagon::R2,  Hexagon::R3,  Hexagon::R4,
+    Hexagon::R5,  Hexagon::R6,  Hexagon::R7,  Hexagon::R8,  Hexagon::R9,
+    Hexagon::R10, Hexagon::R11, Hexagon::R12, Hexagon::R13, Hexagon::R14,
+    Hexagon::R15, Hexagon::R16, Hexagon::R17, Hexagon::R18, Hexagon::R19,
+    Hexagon::R20, Hexagon::R21, Hexagon::R22, Hexagon::R23, Hexagon::R24,
+    Hexagon::R25, Hexagon::R26, Hexagon::R27, Hexagon::R28, Hexagon::R29,
+    Hexagon::R30, Hexagon::R31};
+
+static const uint16_t PredRegDecoderTable[] = {Hexagon::P0, Hexagon::P1,
+                                               Hexagon::P2, Hexagon::P3};
+
+static DecodeStatus DecodeIntRegsRegisterClass(MCInst &Inst, unsigned RegNo,
+                                               uint64_t /*Address*/,
+                                               void const *Decoder) {
+  if (RegNo > 31)
+    return MCDisassembler::Fail;
+
+  unsigned Register = IntRegDecoderTable[RegNo];
+  Inst.addOperand(MCOperand::CreateReg(Register));
+  return MCDisassembler::Success;
+}
+
+static DecodeStatus DecodePredRegsRegisterClass(MCInst &Inst, unsigned RegNo,
+                                                uint64_t /*Address*/,
+                                                void const *Decoder) {
+  if (RegNo > 3)
+    return MCDisassembler::Fail;
+
+  unsigned Register = PredRegDecoderTable[RegNo];
+  Inst.addOperand(MCOperand::CreateReg(Register));
+  return MCDisassembler::Success;
+}
+
 #include "HexagonGenDisassemblerTables.inc"
 
 static MCDisassembler *createHexagonDisassembler(Target const &T,

lib/Target/Hexagon/HexagonAsmPrinter.cpp

     unsigned Size = BundleMIs.size();
     assert((Size+IgnoreCount) == MI->getBundleSize() && "Corrupt Bundle!");
     for (unsigned Index = 0; Index < Size; Index++) {
-      HexagonMCInst MCI;
-      MCI.setPacketStart(Index == 0);
+      HexagonMCInst MCI (BundleMIs[Index]->getOpcode());
+      MCI.setPacketBegin(Index == 0);
       MCI.setPacketEnd(Index == (Size-1));
 
       HexagonLowerToMC(BundleMIs[Index], MCI, *this);

     }
   }
   else {
-    HexagonMCInst MCI;
+    HexagonMCInst MCI(MI->getOpcode());
     if (MI->getOpcode() == Hexagon::ENDLOOP0) {
-      MCI.setPacketStart(true);
+      MCI.setPacketBegin(true);
       MCI.setPacketEnd(true);
     }
     HexagonLowerToMC(MI, MCI, *this);

lib/Target/Hexagon/HexagonInstrInfo.td

 def HexagonWrapperCombineRR :
   SDNode<"HexagonISD::WrapperCombineRR", SDTHexagonI64I32I32>;
 
+let hasSideEffects = 0, hasNewValue = 1, InputType = "reg" in
+class T_ALU32_3op<string mnemonic, bits<3> MajOp, bits<3> MinOp, bit OpsRev,
+                  bit IsComm>
+  : ALU32_rr<(outs IntRegs:$Rd), (ins IntRegs:$Rs, IntRegs:$Rt),
+             "$Rd = "#mnemonic#"($Rs, $Rt)",
+             [], "", ALU32_3op_tc_1_SLOT0123>, ImmRegRel, PredRel {
+  let isCommutable = IsComm;
+  let BaseOpcode = mnemonic#_rr;
+  let CextOpcode = mnemonic;
+
+  bits<5> Rs;
+  bits<5> Rt;
+  bits<5> Rd;
+
+  let IClass = 0b1111;
+  let Inst{27} = 0b0;
+  let Inst{26-24} = MajOp;
+  let Inst{23-21} = MinOp;
+  let Inst{20-16} = !if(OpsRev,Rt,Rs);
+  let Inst{12-8} = !if(OpsRev,Rs,Rt);
+  let Inst{4-0} = Rd;
+}
+
+let hasSideEffects = 0, hasNewValue = 1 in
+class T_ALU32_3op_pred<string mnemonic, bits<3> MajOp, bits<3> MinOp,
+                       bit OpsRev, bit PredNot, bit PredNew>
+  : ALU32_rr<(outs IntRegs:$Rd), (ins PredRegs:$Pu, IntRegs:$Rs, IntRegs:$Rt),
+             "if ("#!if(PredNot,"!","")#"$Pu"#!if(PredNew,".new","")#") "#
+             "$Rd = "#mnemonic#"($Rs, $Rt)",
+             [], "", ALU32_3op_tc_1_SLOT0123>, ImmRegRel, PredNewRel {
+  let isPredicated = 1;
+  let isPredicatedFalse = PredNot;
+  let isPredicatedNew = PredNew;
+  let BaseOpcode = mnemonic#_rr;
+  let CextOpcode = mnemonic;
+
+  bits<2> Pu;
+  bits<5> Rs;
+  bits<5> Rt;
+  bits<5> Rd;
+
+  let IClass = 0b1111;
+  let Inst{27} = 0b1;
+  let Inst{26-24} = MajOp;
+  let Inst{23-21} = MinOp;
+  let Inst{20-16} = !if(OpsRev,Rt,Rs);
+  let Inst{13} = PredNew;
+  let Inst{12-8} = !if(OpsRev,Rs,Rt);
+  let Inst{7} = PredNot;
+  let Inst{6-5} = Pu;
+  let Inst{4-0} = Rd;
+}
+
+multiclass T_ALU32_3op_p<string mnemonic, bits<3> MajOp, bits<3> MinOp,
+                         bit OpsRev> {
+  def t    : T_ALU32_3op_pred<mnemonic, MajOp, MinOp, OpsRev, 0, 0>;
+  def f    : T_ALU32_3op_pred<mnemonic, MajOp, MinOp, OpsRev, 1, 0>;
+  def tnew : T_ALU32_3op_pred<mnemonic, MajOp, MinOp, OpsRev, 0, 1>;
+  def fnew : T_ALU32_3op_pred<mnemonic, MajOp, MinOp, OpsRev, 1, 1>;
+}
+
+multiclass T_ALU32_3op_A2<string mnemonic, bits<3> MajOp, bits<3> MinOp,
+                          bit OpsRev, bit IsComm> {
+  let isPredicable = 1 in
+  def  A2_#NAME  : T_ALU32_3op  <mnemonic, MajOp, MinOp, OpsRev, IsComm>;
+  defm A2_p#NAME : T_ALU32_3op_p<mnemonic, MajOp, MinOp, OpsRev>;
+}
+
+let isCodeGenOnly = 0 in
+defm add : T_ALU32_3op_A2<"add", 0b011, 0b000, 0, 1>;
+
 multiclass ALU32_Pbase<string mnemonic, RegisterClass RC, bit isNot,
                        bit isPredNew> {
   let isPredicatedNew = isPredNew in

lib/Target/Hexagon/HexagonMCInstLower.cpp

 
 using namespace llvm;
 
-static MCOperand GetSymbolRef(const MachineOperand& MO, const MCSymbol* Symbol,
-                              HexagonAsmPrinter& Printer) {
+static MCOperand GetSymbolRef(const MachineOperand &MO, const MCSymbol *Symbol,
+                              HexagonAsmPrinter &Printer) {
   MCContext &MC = Printer.OutContext;
   const MCExpr *ME;
 

 }
 
 // Create an MCInst from a MachineInstr
-void llvm::HexagonLowerToMC(const MachineInstr* MI, HexagonMCInst& MCI,
-                            HexagonAsmPrinter& AP) {
-  MCI.setOpcode(MI->getOpcode());
-  MCI.setDesc(MI->getDesc());
+void llvm::HexagonLowerToMC(const MachineInstr *MI, HexagonMCInst &MCI,
+                            HexagonAsmPrinter &AP) {
+  assert(MCI.getOpcode() == static_cast<unsigned>(MI->getOpcode()) &&
+         "MCI opcode should have been set on construction");
 
   for (unsigned i = 0, e = MI->getNumOperands(); i < e; i++) {
     const MachineOperand &MO = MI->getOperand(i);

       llvm_unreachable("unknown operand type");
     case MachineOperand::MO_Register:
       // Ignore all implicit register operands.
-      if (MO.isImplicit()) continue;
+      if (MO.isImplicit())
+        continue;
       MCO = MCOperand::CreateReg(MO.getReg());
       break;
     case MachineOperand::MO_FPImmediate: {

       MCO = MCOperand::CreateImm(MO.getImm());
       break;
     case MachineOperand::MO_MachineBasicBlock:
-      MCO = MCOperand::CreateExpr
-              (MCSymbolRefExpr::Create(MO.getMBB()->getSymbol(),
-               AP.OutContext));
+      MCO = MCOperand::CreateExpr(
+          MCSymbolRefExpr::Create(MO.getMBB()->getSymbol(), AP.OutContext));
       break;
     case MachineOperand::MO_GlobalAddress:
       MCO = GetSymbolRef(MO, AP.getSymbol(MO.getGlobal()), AP);
       break;
     case MachineOperand::MO_ExternalSymbol:
-      MCO = GetSymbolRef(MO, AP.GetExternalSymbolSymbol(MO.getSymbolName()),
-                         AP);
+      MCO =
+          GetSymbolRef(MO, AP.GetExternalSymbolSymbol(MO.getSymbolName()), AP);
       break;
     case MachineOperand::MO_JumpTableIndex:
       MCO = GetSymbolRef(MO, AP.GetJTISymbol(MO.getIndex()), AP);

       MCO = GetSymbolRef(MO, AP.GetCPISymbol(MO.getIndex()), AP);
       break;
     case MachineOperand::MO_BlockAddress:
-      MCO = GetSymbolRef(MO, AP.GetBlockAddressSymbol(MO.getBlockAddress()),AP);
+      MCO =
+          GetSymbolRef(MO, AP.GetBlockAddressSymbol(MO.getBlockAddress()), AP);
       break;
     }
 

lib/Target/Hexagon/InstPrinter/HexagonInstPrinter.cpp

     // Ending a harware loop is different from ending an regular packet.
     assert(MI->isPacketEnd() && "Loop-end must also end the packet");
 
-    if (MI->isPacketStart()) {
+    if (MI->isPacketBegin()) {
       // There must be a packet to end a loop.
       // FIXME: when shuffling is always run, this shouldn't be needed.
-      HexagonMCInst Nop;
+      HexagonMCInst Nop (Hexagon::NOP);
       StringRef NoAnnot;
 
-      Nop.setOpcode (Hexagon::NOP);
-      Nop.setPacketStart (MI->isPacketStart());
+      Nop.setPacketBegin (MI->isPacketBegin());
       printInst (&Nop, O, NoAnnot);
     }
 

   }
   else {
     // Prefix the insn opening the packet.
-    if (MI->isPacketStart())
+    if (MI->isPacketBegin())
       O << PacketPadding << startPacket << '\n';
 
     printInstruction(MI, O);

lib/Target/Hexagon/MCTargetDesc/HexagonMCCodeEmitter.cpp

 uint32_t getPacketBits(HexagonMCInst const &HMI) {
   unsigned const ParseFieldOffset = 14;
   ParseField Field = HMI.isPacketEnd() ? ParseField::end : ParseField::last0;
-  return static_cast <uint32_t> (Field) << ParseFieldOffset;
+  return static_cast<uint32_t>(Field) << ParseFieldOffset;
 }
 void emitLittleEndian(uint64_t Binary, raw_ostream &OS) {
   OS << static_cast<uint8_t>((Binary >> 0x00) & 0xff);

                                              SmallVectorImpl<MCFixup> &Fixups,
                                              MCSubtargetInfo const &STI) const {
   HexagonMCInst const &HMB = static_cast<HexagonMCInst const &>(MI);
-  uint64_t Binary = getBinaryCodeForInstr(HMB, Fixups, STI) | getPacketBits(HMB);
-  assert(HMB.getDesc().getSize() == 4 && "All instructions should be 32bit");
+  uint64_t Binary =
+      getBinaryCodeForInstr(HMB, Fixups, STI) | getPacketBits(HMB);
+  Binary |= getPacketBits(HMB);
   emitLittleEndian(Binary, OS);
   ++MCNumEmitted;
 }

lib/Target/Hexagon/MCTargetDesc/HexagonMCInst.h

 #ifndef LLVM_LIB_TARGET_HEXAGON_MCTARGETDESC_HEXAGONMCINST_H
 #define LLVM_LIB_TARGET_HEXAGON_MCTARGETDESC_HEXAGONMCINST_H
 
-#include "HexagonTargetMachine.h"
 #include "llvm/MC/MCInst.h"
 
 namespace llvm {
-  class MCOperand;
+class MCInstrDesc;
+class MCOperand;
+class HexagonTargetMachine;
 
-  class HexagonMCInst: public MCInst {
-    // MCID is set during instruction lowering.
-    // It is needed in order to access TSFlags for
-    // use in checking MC instruction properties.
-    const MCInstrDesc *MCID;
+class HexagonMCInst : public MCInst {
+public:
+  explicit HexagonMCInst(unsigned op);
 
-    // Packet start and end markers
-    unsigned packetStart: 1, packetEnd: 1;
+  /// 10.6 Instruction Packets
+  bool isPacketBegin() const;
+  /// \brief Is this marked as last in packet.
+  bool isPacketEnd() const;
+  void setPacketBegin(bool Y);
+  /// \brief Mark this as last in packet.
+  void setPacketEnd(bool Y);
+  /// \brief Return the slots used.
+  unsigned getUnits(HexagonTargetMachine const &TM) const;
+  bool isConstExtended() const;
+  /// \brief Return constant extended operand number.
+  unsigned short getCExtOpNum(void) const;
+  /// \brief Return whether this is a new-value consumer.
+  bool isNewValue() const;
+  /// \brief Return whether this is a legal new-value producer.
+  bool hasNewValue() const;
+  /// \brief Return the operand that consumes or produces a new value.
+  MCOperand const &getNewValue() const;
+  /// \brief Return number of bits in the constant extended operand.
+  unsigned getBitCount(void) const;
 
-  public:
-    explicit HexagonMCInst():
-      MCInst(), MCID(nullptr), packetStart(0), packetEnd(0) {};
-    HexagonMCInst(const MCInstrDesc& mcid):
-      MCInst(), MCID(&mcid), packetStart(0), packetEnd(0) {};
-
-    bool isPacketStart() const { return (packetStart); };
-    bool isPacketEnd() const { return (packetEnd); };
-    void setPacketStart(bool Y) { packetStart = Y; };
-    void setPacketEnd(bool Y) { packetEnd = Y; };
-    void resetPacket() { setPacketStart(false); setPacketEnd(false); };
-
-    // Return the slots used by the insn.
-    unsigned getUnits(const HexagonTargetMachine* TM) const;
-
-    // Return the Hexagon ISA class for the insn.
-    unsigned getType() const;
-
-    void setDesc(const MCInstrDesc& mcid) { MCID = &mcid; };
-    const MCInstrDesc& getDesc(void) const { return *MCID; };
-
-    // Return whether the insn is an actual insn.
-    bool isCanon() const;
-
-    // Return whether the insn is a prefix.
-    bool isPrefix() const;
-
-    // Return whether the insn is solo, i.e., cannot be in a packet.
-    bool isSolo() const;
-
-    // Return whether the instruction needs to be constant extended.
-    bool isConstExtended() const;
-
-    // Return constant extended operand number.
-    unsigned short getCExtOpNum(void) const;
-
-    // Return whether the insn is a new-value consumer.
-    bool isNewValue() const;
-
-    // Return whether the instruction is a legal new-value producer.
-    bool hasNewValue() const;
-
-    // Return the operand that consumes or produces a new value.
-    const MCOperand& getNewValue() const;
-
-    // Return number of bits in the constant extended operand.
-    unsigned getBitCount(void) const;
-
-  private:
-    // Return whether the instruction must be always extended.
-    bool isExtended() const;
-
-    // Return true if the insn may be extended based on the operand value.
-    bool isExtendable() const;
-
-    // Return true if the operand can be constant extended.
-    bool isOperandExtended(const unsigned short OperandNum) const;
-
-    // Return the min value that a constant extendable operand can have
-    // without being extended.
-    int getMinValue() const;
-
-    // Return the max value that a constant extendable operand can have
-    // without being extended.
-    int getMaxValue() const;
-  };
+private:
+  /// \brief Return whether this must be always extended.
+  bool isExtended() const;
+  /// \brief Return true if this may be extended based on the operand value.
+  bool isExtendable() const;
+  ///  \brief Return if the operand can be constant extended.
+  bool isOperandExtended(unsigned short const OperandNum) const;
+  /// \brief Return the min value that a constant extendable operand can have
+  /// without being extended.
+  int getMinValue() const;
+  /// \brief Return the max value that a constant extendable operand can have
+  /// without being extended.
+  int getMaxValue() const;
+  bool packetBegin;
+  bool packetEnd;
+  MCInstrDesc const &MCID;
+};
 }
 
 #endif

lib/Target/Hexagon/MCTargetDesc/HexagonMCInst.cpp

 //===----------------------------------------------------------------------===//
 
 #include "HexagonInstrInfo.h"
+#include "HexagonTargetMachine.h"
 #include "MCTargetDesc/HexagonBaseInfo.h"
 #include "MCTargetDesc/HexagonMCInst.h"
 #include "MCTargetDesc/HexagonMCTargetDesc.h"
+#include "llvm/Support/TargetRegistry.h"
 
 using namespace llvm;
 
-// Return the slots used by the insn.
-unsigned HexagonMCInst::getUnits(const HexagonTargetMachine* TM) const {
-  const HexagonInstrInfo *QII = TM->getSubtargetImpl()->getInstrInfo();
-  const InstrItineraryData *II =
-      TM->getSubtargetImpl()->getInstrItineraryData();
-  const InstrStage*
-    IS = II->beginStage(QII->get(this->getOpcode()).getSchedClass());
-
-  return (IS->getUnits());
+HexagonMCInst::HexagonMCInst(unsigned op)
+    : packetBegin(false), packetEnd(false),
+      MCID(llvm::TheHexagonTarget.createMCInstrInfo()->get(op)) {
+  assert(MCID.getSize() == 4 && "All instructions should be 32bit");
+  setOpcode(op);
 }
 
-// Return the Hexagon ISA class for the insn.
-unsigned HexagonMCInst::getType() const {
-  const uint64_t F = MCID->TSFlags;
+bool HexagonMCInst::isPacketBegin() const { return packetBegin; }
+bool HexagonMCInst::isPacketEnd() const { return packetEnd; }
+void HexagonMCInst::setPacketEnd(bool Y) { packetEnd = Y; }
+void HexagonMCInst::setPacketBegin(bool Y) { packetBegin = Y; }
 
-  return ((F >> HexagonII::TypePos) & HexagonII::TypeMask);
-}
+unsigned HexagonMCInst::getUnits(HexagonTargetMachine const &TM) const {
+  const HexagonInstrInfo *QII = TM.getSubtargetImpl()->getInstrInfo();
+  const InstrItineraryData *II = TM.getSubtargetImpl()->getInstrItineraryData();
+  const InstrStage *IS =
+      II->beginStage(QII->get(this->getOpcode()).getSchedClass());
 
-// Return whether the insn is an actual insn.
-bool HexagonMCInst::isCanon() const {
-  return (!MCID->isPseudo() &&
-          !isPrefix() &&
-          getType() != HexagonII::TypeENDLOOP);
+  return (IS->getUnits());
 }
 
-// Return whether the insn is a prefix.
-bool HexagonMCInst::isPrefix() const {
-  return (getType() == HexagonII::TypePREFIX);
-}
-
-// Return whether the insn is solo, i.e., cannot be in a packet.
-bool HexagonMCInst::isSolo() const {
-  const uint64_t F = MCID->TSFlags;
-  return ((F >> HexagonII::SoloPos) & HexagonII::SoloMask);
-}
-
-// Return whether the insn is a new-value consumer.
 bool HexagonMCInst::isNewValue() const {
-  const uint64_t F = MCID->TSFlags;
+  const uint64_t F = MCID.TSFlags;
   return ((F >> HexagonII::NewValuePos) & HexagonII::NewValueMask);
 }
 
-// Return whether the instruction is a legal new-value producer.
 bool HexagonMCInst::hasNewValue() const {
-  const uint64_t F = MCID->TSFlags;
+  const uint64_t F = MCID.TSFlags;
   return ((F >> HexagonII::hasNewValuePos) & HexagonII::hasNewValueMask);
 }
 
-// Return the operand that consumes or produces a new value.
-const MCOperand& HexagonMCInst::getNewValue() const {
-  const uint64_t F = MCID->TSFlags;
-  const unsigned O = (F >> HexagonII::NewValueOpPos) &
-                     HexagonII::NewValueOpMask;
-  const MCOperand& MCO = getOperand(O);
+MCOperand const &HexagonMCInst::getNewValue() const {
+  const uint64_t F = MCID.TSFlags;
+  const unsigned O =
+      (F >> HexagonII::NewValueOpPos) & HexagonII::NewValueOpMask;
+  const MCOperand &MCO = getOperand(O);
 
-  assert ((isNewValue() || hasNewValue()) && MCO.isReg());
+  assert((isNewValue() || hasNewValue()) && MCO.isReg());
   return (MCO);
 }
 

 // 2) For immediate extended operands, return true only if the value is
 //    out-of-range.
 // 3) For global address, always return true.
-
 bool HexagonMCInst::isConstExtended(void) const {
   if (isExtended())
     return true;

     return false;
 
   short ExtOpNum = getCExtOpNum();
-  int MinValue   = getMinValue();
-  int MaxValue   = getMaxValue();
-  const MCOperand& MO = getOperand(ExtOpNum);
+  int MinValue = getMinValue();
+  int MaxValue = getMaxValue();
+  const MCOperand &MO = getOperand(ExtOpNum);
 
   // We could be using an instruction with an extendable immediate and shoehorn
   // a global address into it. If it is a global address it will be constant

   return (ImmValue < MinValue || ImmValue > MaxValue);
 }
 
-// Return whether the instruction must be always extended.
 bool HexagonMCInst::isExtended(void) const {
-  const uint64_t F = MCID->TSFlags;
+  const uint64_t F = MCID.TSFlags;
   return (F >> HexagonII::ExtendedPos) & HexagonII::ExtendedMask;
 }
 
-// Return true if the instruction may be extended based on the operand value.
 bool HexagonMCInst::isExtendable(void) const {
-  const uint64_t F = MCID->TSFlags;
+  const uint64_t F = MCID.TSFlags;
   return (F >> HexagonII::ExtendablePos) & HexagonII::ExtendableMask;
 }
 
-// Return number of bits in the constant extended operand.
 unsigned HexagonMCInst::getBitCount(void) const {
-  const uint64_t F = MCID->TSFlags;
+  const uint64_t F = MCID.TSFlags;
   return ((F >> HexagonII::ExtentBitsPos) & HexagonII::ExtentBitsMask);
 }
 
-// Return constant extended operand number.
 unsigned short HexagonMCInst::getCExtOpNum(void) const {
-  const uint64_t F = MCID->TSFlags;
+  const uint64_t F = MCID.TSFlags;
   return ((F >> HexagonII::ExtendableOpPos) & HexagonII::ExtendableOpMask);
 }
 
-// Return whether the operand can be constant extended.
 bool HexagonMCInst::isOperandExtended(const unsigned short OperandNum) const {
-  const uint64_t F = MCID->TSFlags;
-  return ((F >> HexagonII::ExtendableOpPos) & HexagonII::ExtendableOpMask)
-          == OperandNum;
+  const uint64_t F = MCID.TSFlags;
+  return ((F >> HexagonII::ExtendableOpPos) & HexagonII::ExtendableOpMask) ==
+         OperandNum;
 }
 
-// Return the min value that a constant extendable operand can have
-// without being extended.
 int HexagonMCInst::getMinValue(void) const {
-  const uint64_t F = MCID->TSFlags;
-  unsigned isSigned = (F >> HexagonII::ExtentSignedPos)
-                    & HexagonII::ExtentSignedMask;
-  unsigned bits =  (F >> HexagonII::ExtentBitsPos)
-                    & HexagonII::ExtentBitsMask;
+  const uint64_t F = MCID.TSFlags;
+  unsigned isSigned =
+      (F >> HexagonII::ExtentSignedPos) & HexagonII::ExtentSignedMask;
+  unsigned bits = (F >> HexagonII::ExtentBitsPos) & HexagonII::ExtentBitsMask;
 
-  if (isSigned) // if value is signed
+  if (isSigned)
     return -1U << (bits - 1);
   else
     return 0;
 }
 
-// Return the max value that a constant extendable operand can have
-// without being extended.
 int HexagonMCInst::getMaxValue(void) const {
-  const uint64_t F = MCID->TSFlags;
-  unsigned isSigned = (F >> HexagonII::ExtentSignedPos)
-                    & HexagonII::ExtentSignedMask;
-  unsigned bits =  (F >> HexagonII::ExtentBitsPos)
-                    & HexagonII::ExtentBitsMask;
+  const uint64_t F = MCID.TSFlags;
+  unsigned isSigned =
+      (F >> HexagonII::ExtentSignedPos) & HexagonII::ExtentSignedMask;
+  unsigned bits = (F >> HexagonII::ExtentBitsPos) & HexagonII::ExtentBitsMask;
 
-  if (isSigned) // if value is signed
+  if (isSigned)
     return ~(-1U << (bits - 1));
   else
     return ~(-1U << bits);

lib/Target/Hexagon/MCTargetDesc/HexagonMCTargetDesc.h

 #define LLVM_LIB_TARGET_HEXAGON_MCTARGETDESC_HEXAGONMCTARGETDESC_H
 
 namespace llvm {
+struct InstrItinerary;
+struct InstrStage;
 class MCCodeEmitter;
 class MCContext;
 class MCInstrInfo;

lib/Target/Hexagon/MCTargetDesc/HexagonMCTargetDesc.cpp

 #define GET_REGINFO_MC_DESC
 #include "HexagonGenRegisterInfo.inc"
 
-static MCInstrInfo *createHexagonMCInstrInfo() {
+static llvm::MCInstrInfo *createHexagonMCInstrInfo() {
   MCInstrInfo *X = new MCInstrInfo();
   InitHexagonMCInstrInfo(X);
   return X;

test/MC/Disassembler/Hexagon/alu32.txt

+# RUN: llvm-mc --triple hexagon -disassemble < %s | FileCheck %s
+
+0x11 0xdf 0x15 0xf3
+# CHECK: r17 = add(r21, r31)

test/MC/Disassembler/Hexagon/lit.local.cfg

+if 'Hexagon' not in config.root.targets:
+    config.unsupported = True
+

unittests/MC/CMakeLists.txt

   StringTableBuilderTest.cpp
   YAMLTest.cpp
   )
-  
-foreach(t ${LLVM_TARGETS_TO_BUILD})
-  if (IS_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}/${t}")
-    add_subdirectory(${t})
-  endif (IS_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}/${t}")
-endforeach()

unittests/MC/Hexagon/HexagonMCCodeEmitterTest.cpp

-#include "gtest/gtest.h"
-
-#include <memory>
-
-#include "llvm/MC/MCCodeEmitter.h"
-#include "llvm/MC/MCContext.h"
-#include "llvm/Support/raw_ostream.h"
-#include "llvm/Support/TargetRegistry.h"
-#include "llvm/Support/TargetSelect.h"
-
-#include "MCTargetDesc/HexagonMCInst.h"
-#include "MCTargetDesc/HexagonMCTargetDesc.h"
-
-namespace {
-class TestEmitter {
-public:
-  TestEmitter() : Triple("hexagon-unknown-elf") {
-    LLVMInitializeHexagonTargetInfo();
-    LLVMInitializeHexagonTarget();
-    LLVMInitializeHexagonTargetMC();
-    std::string error;
-    Target = llvm::TargetRegistry::lookupTarget("hexagon", error);
-    assert(Target != nullptr && "Expected to find target");
-    assert(error.empty() && "Error should be empty if we have a target");
-    RegisterInfo = Target->createMCRegInfo(Triple);
-    assert(RegisterInfo != nullptr && "Expecting to find register info");
-    AsmInfo = Target->createMCAsmInfo(*RegisterInfo, Triple);
-    assert(AsmInfo != nullptr && "Expecting to find asm info");
-    Context = new llvm::MCContext(AsmInfo, RegisterInfo, nullptr);
-    assert(Context != nullptr && "Expecting to create a context");
-    Subtarget = Target->createMCSubtargetInfo(Triple, "hexagonv4", "");
-    assert(Subtarget != nullptr && "Expecting to find a subtarget");
-    InstrInfo = Target->createMCInstrInfo();
-    assert(InstrInfo != nullptr && "Expecting to find instr info");
-    Emitter = Target->createMCCodeEmitter(*InstrInfo, *RegisterInfo, *Subtarget,
-                                          *Context);
-    assert(Emitter != nullptr);
-  }
-  std::string Triple;
-  llvm::Target const *Target;
-  llvm::MCRegisterInfo *RegisterInfo;
-  llvm::MCAsmInfo *AsmInfo;
-  llvm::MCContext *Context;
-  llvm::MCSubtargetInfo *Subtarget;
-  llvm::MCInstrInfo *InstrInfo;
-  llvm::MCCodeEmitter *Emitter;
-};
-TestEmitter Emitter;
-}
-
-TEST(HexagonMCCodeEmitter, emitter_creation) {
-  ASSERT_NE(nullptr, Emitter.Emitter);
-}