[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/HexagonAsmPrinter.cpp

       BundleMIs.push_back(MInst);
       ++MII;
     }
-    unsigned Size = BundleMIs.size();
-    assert((Size+IgnoreCount) == MI->getBundleSize() && "Corrupt Bundle!");
-    for (unsigned Index = 0; Index < Size; Index++) {
-      HexagonMCInst MCI;
-      MCI.setPacketStart(Index == 0);
-      MCI.setPacketEnd(Index == (Size-1));
-
-      HexagonLowerToMC(BundleMIs[Index], MCI, *this);
+    unsigned Size = BundleMIs.size();
+    assert((Size+IgnoreCount) == MI->getBundleSize() && "Corrupt Bundle!");
+    for (unsigned Index = 0; Index < Size; Index++) {
+      HexagonMCInst MCI (BundleMIs[Index]->getOpcode());
+      MCI.setPacketBegin(Index == 0);
+      MCI.setPacketEnd(Index == (Size-1));
+
+      HexagonLowerToMC(BundleMIs[Index], MCI, *this);
       EmitToStreamer(OutStreamer, MCI);
-    }
-  }
-  else {
-    HexagonMCInst MCI;
-    if (MI->getOpcode() == Hexagon::ENDLOOP0) {
-      MCI.setPacketStart(true);
-      MCI.setPacketEnd(true);
-    }
-    HexagonLowerToMC(MI, MCI, *this);
+    }
+  }
+  else {
+    HexagonMCInst MCI(MI->getOpcode());
+    if (MI->getOpcode() == Hexagon::ENDLOOP0) {
+      MCI.setPacketBegin(true);
+      MCI.setPacketEnd(true);
+    }
+    HexagonLowerToMC(MI, MCI, *this);
     EmitToStreamer(OutStreamer, MCI);
   }
 

lib/Target/Hexagon/HexagonInstrInfo.td

 def HexagonWrapperCombineII :
   SDNode<"HexagonISD::WrapperCombineII", SDTHexagonI64I32I32>;
 
-def HexagonWrapperCombineRR :
-  SDNode<"HexagonISD::WrapperCombineRR", SDTHexagonI64I32I32>;
-
-multiclass ALU32_Pbase<string mnemonic, RegisterClass RC, bit isNot,
-                       bit isPredNew> {
-  let isPredicatedNew = isPredNew in
+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>;
+}
+
+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
   def NAME : ALU32_rr<(outs RC:$dst),
             (ins PredRegs:$src1, IntRegs:$src2, IntRegs: $src3),
             !if(isNot, "if (!$src1", "if ($src1")#!if(isPredNew,".new) $dst = ",

lib/Target/Hexagon/HexagonMCInstLower.cpp

 #include "llvm/IR/Mangler.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
-
-using namespace llvm;
-
-static MCOperand GetSymbolRef(const MachineOperand& MO, const MCSymbol* Symbol,
-                              HexagonAsmPrinter& Printer) {
-  MCContext &MC = Printer.OutContext;
-  const MCExpr *ME;
-
+
+using namespace llvm;
+
+static MCOperand GetSymbolRef(const MachineOperand &MO, const MCSymbol *Symbol,
+                              HexagonAsmPrinter &Printer) {
+  MCContext &MC = Printer.OutContext;
+  const MCExpr *ME;
+
   ME = MCSymbolRefExpr::Create(Symbol, MCSymbolRefExpr::VK_None, MC);
 
   if (!MO.isJTI() && MO.getOffset())

                                  MC);
 
   return (MCOperand::CreateExpr(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());
-
-  for (unsigned i = 0, e = MI->getNumOperands(); i < e; i++) {
-    const MachineOperand &MO = MI->getOperand(i);
+}
+
+// Create an MCInst from a MachineInstr
+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);
     MCOperand MCO;
 
     switch (MO.getType()) {
     default:
       MI->dump();
-      llvm_unreachable("unknown operand type");
-    case MachineOperand::MO_Register:
-      // Ignore all implicit register operands.
-      if (MO.isImplicit()) continue;
-      MCO = MCOperand::CreateReg(MO.getReg());
-      break;
-    case MachineOperand::MO_FPImmediate: {
+      llvm_unreachable("unknown operand type");
+    case MachineOperand::MO_Register:
+      // Ignore all implicit register operands.
+      if (MO.isImplicit())
+        continue;
+      MCO = MCOperand::CreateReg(MO.getReg());
+      break;
+    case MachineOperand::MO_FPImmediate: {
       APFloat Val = MO.getFPImm()->getValueAPF();
       // FP immediates are used only when setting GPRs, so they may be dealt
       // with like regular immediates from this point on.

       break;
     }
     case MachineOperand::MO_Immediate:
-      MCO = MCOperand::CreateImm(MO.getImm());
+      MCO = MCOperand::CreateImm(MO.getImm());
+      break;
+    case MachineOperand::MO_MachineBasicBlock:
+      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);
+      break;
+    case MachineOperand::MO_JumpTableIndex:
+      MCO = GetSymbolRef(MO, AP.GetJTISymbol(MO.getIndex()), AP);
       break;
-    case MachineOperand::MO_MachineBasicBlock:
-      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);
-      break;
-    case MachineOperand::MO_JumpTableIndex:
-      MCO = GetSymbolRef(MO, AP.GetJTISymbol(MO.getIndex()), AP);
-      break;
     case MachineOperand::MO_ConstantPoolIndex:
-      MCO = GetSymbolRef(MO, AP.GetCPISymbol(MO.getIndex()), AP);
-      break;
-    case MachineOperand::MO_BlockAddress:
-      MCO = GetSymbolRef(MO, AP.GetBlockAddressSymbol(MO.getBlockAddress()),AP);
-      break;
-    }
-
+      MCO = GetSymbolRef(MO, AP.GetCPISymbol(MO.getIndex()), AP);
+      break;
+    case MachineOperand::MO_BlockAddress:
+      MCO =
+          GetSymbolRef(MO, AP.GetBlockAddressSymbol(MO.getBlockAddress()), AP);
+      break;
+    }
+
     MCI.addOperand(MCO);
   }
 }

lib/Target/Hexagon/InstPrinter/HexagonInstPrinter.cpp

              endPacket = '}';
   // TODO: add outer HW loop when it's supported too.
   if (MI->getOpcode() == Hexagon::ENDLOOP0) {
-    // Ending a harware loop is different from ending an regular packet.
-    assert(MI->isPacketEnd() && "Loop-end must also end the packet");
-
-    if (MI->isPacketStart()) {
-      // There must be a packet to end a loop.
-      // FIXME: when shuffling is always run, this shouldn't be needed.
-      HexagonMCInst Nop;
-      StringRef NoAnnot;
-
-      Nop.setOpcode (Hexagon::NOP);
-      Nop.setPacketStart (MI->isPacketStart());
-      printInst (&Nop, O, NoAnnot);
-    }
-
+    // Ending a harware loop is different from ending an regular packet.
+    assert(MI->isPacketEnd() && "Loop-end must also end the packet");
+
+    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 (Hexagon::NOP);
+      StringRef NoAnnot;
+
+      Nop.setPacketBegin (MI->isPacketBegin());
+      printInst (&Nop, O, NoAnnot);
+    }
+
     // Close the packet.
     if (MI->isPacketEnd())
       O << PacketPadding << endPacket;
 
     printInstruction(MI, O);
-  }
-  else {
-    // Prefix the insn opening the packet.
-    if (MI->isPacketStart())
-      O << PacketPadding << startPacket << '\n';
+  }
+  else {
+    // Prefix the insn opening the packet.
+    if (MI->isPacketBegin())
+      O << PacketPadding << startPacket << '\n';
+
+    printInstruction(MI, O);
 
-    printInstruction(MI, O);
-
     // Suffix the insn closing the packet.
     if (MI->isPacketEnd())
       // Suffix the packet in a new line always, since the GNU assembler has

lib/Target/Hexagon/MCTargetDesc/HexagonMCCodeEmitter.cpp

 /// Possible values for instruction packet parse field.
 enum class ParseField { duplex = 0x0, last0 = 0x1, last1 = 0x2, end = 0x3 };
 /// \brief Returns the packet bits based on instruction position.
-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;
-}
-void emitLittleEndian(uint64_t Binary, raw_ostream &OS) {
-  OS << static_cast<uint8_t>((Binary >> 0x00) & 0xff);
+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;
+}
+void emitLittleEndian(uint64_t Binary, raw_ostream &OS) {
+  OS << static_cast<uint8_t>((Binary >> 0x00) & 0xff);
   OS << static_cast<uint8_t>((Binary >> 0x08) & 0xff);
   OS << static_cast<uint8_t>((Binary >> 0x10) & 0xff);
   OS << static_cast<uint8_t>((Binary >> 0x18) & 0xff);

     : MST(aMST), MCT(aMCT) {}
 
 void HexagonMCCodeEmitter::EncodeInstruction(MCInst const &MI, raw_ostream &OS,
-                                             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");
-  emitLittleEndian(Binary, OS);
-  ++MCNumEmitted;
-}
+                                             SmallVectorImpl<MCFixup> &Fixups,
+                                             MCSubtargetInfo const &STI) const {
+  HexagonMCInst const &HMB = static_cast<HexagonMCInst const &>(MI);
+  uint64_t Binary =
+      getBinaryCodeForInstr(HMB, Fixups, STI) | getPacketBits(HMB);
+  Binary |= getPacketBits(HMB);
+  emitLittleEndian(Binary, OS);
+  ++MCNumEmitted;
+}
 
 unsigned
 HexagonMCCodeEmitter::getMachineOpValue(MCInst const &MI, MCOperand const &MO,

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 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;
-
-    // Packet start and end markers
-    unsigned packetStart: 1, packetEnd: 1;
-
-  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;
-  };
-}
-
-#endif
+#ifndef LLVM_LIB_TARGET_HEXAGON_MCTARGETDESC_HEXAGONMCINST_H
+#define LLVM_LIB_TARGET_HEXAGON_MCTARGETDESC_HEXAGONMCINST_H
+
+#include "llvm/MC/MCInst.h"
+
+namespace llvm {
+class MCInstrDesc;
+class MCOperand;
+class HexagonTargetMachine;
+
+class HexagonMCInst : public MCInst {
+public:
+  explicit HexagonMCInst(unsigned op);
+
+  /// 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;
+
+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

 //
 // This class extends MCInst to allow some Hexagon VLIW annotations.
 //
-//===----------------------------------------------------------------------===//
-
-#include "HexagonInstrInfo.h"
-#include "MCTargetDesc/HexagonBaseInfo.h"
-#include "MCTargetDesc/HexagonMCInst.h"
-#include "MCTargetDesc/HexagonMCTargetDesc.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());
-}
-
-// Return the Hexagon ISA class for the insn.
-unsigned HexagonMCInst::getType() const {
-  const uint64_t F = MCID->TSFlags;
-
-  return ((F >> HexagonII::TypePos) & HexagonII::TypeMask);
-}
-
-// Return whether the insn is an actual insn.
-bool HexagonMCInst::isCanon() const {
-  return (!MCID->isPseudo() &&
-          !isPrefix() &&
-          getType() != HexagonII::TypeENDLOOP);
-}
-
-// 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;
-  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;
-  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);
-
-  assert ((isNewValue() || hasNewValue()) && MCO.isReg());
-  return (MCO);
-}
-
+//===----------------------------------------------------------------------===//
+
+#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;
+
+HexagonMCInst::HexagonMCInst(unsigned op)
+    : packetBegin(true), packetEnd(true),
+      MCID(llvm::TheHexagonTarget.createMCInstrInfo()->get(op)) {
+  assert(MCID.getSize() == 4 && "All instructions should be 32bit");
+  setOpcode(op);
+}
+
+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; }
+
+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 (IS->getUnits());
+}
+
+bool HexagonMCInst::isNewValue() const {
+  const uint64_t F = MCID.TSFlags;
+  return ((F >> HexagonII::NewValuePos) & HexagonII::NewValueMask);
+}
+
+bool HexagonMCInst::hasNewValue() const {
+  const uint64_t F = MCID.TSFlags;
+  return ((F >> HexagonII::hasNewValuePos) & HexagonII::hasNewValueMask);
+}
+
+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());
+  return (MCO);
+}
+
 // Return whether the instruction needs to be constant extended.
 // 1) Always return true if the instruction has 'isExtended' flag set.
 //
 // isExtendable:
-// 2) For immediate extended operands, return true only if the value is
-//    out-of-range.
-// 3) For global address, always return true.
+// 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;
 
-bool HexagonMCInst::isConstExtended(void) const {
-  if (isExtended())
-    return true;
-
   if (!isExtendable())
-    return false;
-
-  short ExtOpNum = getCExtOpNum();
-  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 false;
+
+  short ExtOpNum = getCExtOpNum();
+  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
   // extended. We do this for COMBINE.
   // We currently only handle isGlobal() because it is the only kind of
   // object we are going to end up with here for now.

   assert(MO.isImm() && "Extendable operand must be Immediate type");
 
   int ImmValue = MO.getImm();
-  return (ImmValue < MinValue || ImmValue > MaxValue);
+  return (ImmValue < MinValue || ImmValue > MaxValue);
+}
+
+bool HexagonMCInst::isExtended(void) const {
+  const uint64_t F = MCID.TSFlags;
+  return (F >> HexagonII::ExtendedPos) & HexagonII::ExtendedMask;
+}
+
+bool HexagonMCInst::isExtendable(void) const {
+  const uint64_t F = MCID.TSFlags;
+  return (F >> HexagonII::ExtendablePos) & HexagonII::ExtendableMask;
+}
+
+unsigned HexagonMCInst::getBitCount(void) const {
+  const uint64_t F = MCID.TSFlags;
+  return ((F >> HexagonII::ExtentBitsPos) & HexagonII::ExtentBitsMask);
+}
+
+unsigned short HexagonMCInst::getCExtOpNum(void) const {
+  const uint64_t F = MCID.TSFlags;
+  return ((F >> HexagonII::ExtendableOpPos) & HexagonII::ExtendableOpMask);
+}
+
+bool HexagonMCInst::isOperandExtended(const unsigned short OperandNum) const {
+  const uint64_t F = MCID.TSFlags;
+  return ((F >> HexagonII::ExtendableOpPos) & HexagonII::ExtendableOpMask) ==
+         OperandNum;
+}
+
+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;
+
+  if (isSigned)
+    return -1U << (bits - 1);
+  else
+    return 0;
+}
+
+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;
+
+  if (isSigned)
+    return ~(-1U << (bits - 1));
+  else
+    return ~(-1U << bits);
 }
-
-// Return whether the instruction must be always extended.
-bool HexagonMCInst::isExtended(void) const {
-  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;
-  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;
-  return ((F >> HexagonII::ExtentBitsPos) & HexagonII::ExtentBitsMask);
-}
-
-// Return constant extended operand number.
-unsigned short HexagonMCInst::getCExtOpNum(void) const {
-  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;
-}
-
-// 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;
-
-  if (isSigned) // if value is signed
-    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;
-
-  if (isSigned) // if value is signed
-    return ~(-1U << (bits - 1));
-  else
-    return ~(-1U << bits);
-}

lib/Target/Hexagon/MCTargetDesc/HexagonMCTargetDesc.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_TARGET_HEXAGON_MCTARGETDESC_HEXAGONMCTARGETDESC_H
-#define LLVM_LIB_TARGET_HEXAGON_MCTARGETDESC_HEXAGONMCTARGETDESC_H
-
-namespace llvm {
-class MCCodeEmitter;
-class MCContext;
-class MCInstrInfo;
+#define LLVM_LIB_TARGET_HEXAGON_MCTARGETDESC_HEXAGONMCTARGETDESC_H
+
+namespace llvm {
+struct InstrItinerary;
+struct InstrStage;
+class MCCodeEmitter;
+class MCContext;
+class MCInstrInfo;
 class MCRegisterInfo;
 class MCSubtargetInfo;
 class Target;

lib/Target/Hexagon/MCTargetDesc/HexagonMCTargetDesc.cpp

 #define GET_SUBTARGETINFO_MC_DESC
 #include "HexagonGenSubtargetInfo.inc"
 
-#define GET_REGINFO_MC_DESC
-#include "HexagonGenRegisterInfo.inc"
-
-static MCInstrInfo *createHexagonMCInstrInfo() {
-  MCInstrInfo *X = new MCInstrInfo();
-  InitHexagonMCInstrInfo(X);
-  return X;
+#define GET_REGINFO_MC_DESC
+#include "HexagonGenRegisterInfo.inc"
+
+static llvm::MCInstrInfo *createHexagonMCInstrInfo() {
+  MCInstrInfo *X = new MCInstrInfo();
+  InitHexagonMCInstrInfo(X);
+  return X;
 }
 
 static MCRegisterInfo *createHexagonMCRegisterInfo(StringRef TT) {

unittests/MC/CMakeLists.txt

   )
 
 add_llvm_unittest(MCTests
-  StringTableBuilderTest.cpp
-  YAMLTest.cpp
-  )
-  
-foreach(t ${LLVM_TARGETS_TO_BUILD})
-  if (IS_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}/${t}")
-    add_subdirectory(${t})
+  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

   llvm::MCContext *Context;
   llvm::MCSubtargetInfo *Subtarget;
   llvm::MCInstrInfo *InstrInfo;
-  llvm::MCCodeEmitter *Emitter;
-};
-TestEmitter Emitter;
-}
-
-TEST(HexagonMCCodeEmitter, emitter_creation) {
-  ASSERT_NE(nullptr, Emitter.Emitter);
-}
+  llvm::MCCodeEmitter *Emitter;
+};
+TestEmitter Emitter;
+llvm::SmallVector<llvm::MCFixup, 0> EmptyFixups;
+uint32_t Encode(unsigned opcode, std::vector<llvm::MCOperand> const &operands,
+                llvm::SmallVectorImpl<llvm::MCFixup> const & = EmptyFixups) {
+  llvm::HexagonMCInst inst(opcode);
+  for (auto &i : operands) {
+    inst.addOperand(i);
+  }
+  std::string str;
+  {
+    llvm::raw_string_ostream stream(str);
+    Emitter.Emitter->EncodeInstruction(inst, stream, EmptyFixups,
+                                       *Emitter.Subtarget);
+  }
+  assert(str.size() == 4 && "All instructions are 32 bits");
+  uint32_t result = (static_cast<uint8_t>(str[3]) << 0x18) |
+                    (static_cast<uint8_t>(str[2]) << 0x10) |
+                    (static_cast<uint8_t>(str[1]) << 0x08) |
+                    (static_cast<uint8_t>(str[0]) << 0x00);
+  return result;
+}
+llvm::MCOperand Reg(unsigned reg) { return llvm::MCOperand::CreateReg(reg); }
+llvm::MCOperand Imm(int64_t imm) { return llvm::MCOperand::CreateImm(imm); }
+}
+
+using namespace llvm;
+using namespace Hexagon;
+
+TEST(HexagonMCCodeEmitter, emitter_creation) {
+  ASSERT_NE(nullptr, Emitter.Emitter);
+}
+
+/// \brief Instruction encoding with different register numbers
+TEST(HexagonMCCodeEmitter, add_Rd_Rs_Rt) {
+  EXPECT_EQ(0xf315df11, Encode(A2_add, {Reg(R17), Reg(R21), Reg(R31)}));
+}