The issues with X86 prefixes: step 2

[avt77]

Summary

This patch includes everything we have in D36788 plus it covers PR19251. I was forced to refactor and to re-design the current implementation because it was not adopted for various use cases of prefixes. Now it should fix all known issues.

[craig.topper]

include/llvm/MC/MCInst.h

165

sence->sense

[RKSimon]

lib/Target/X86/Disassembler/X86Disassembler.cpp

243

Flags |= 1 ?

251

These hard coded values for the flag bits are going to be tricky to maintain. Perhaps an enum ?

enum OpPrefixFlag {
  OPF_OpSize = 1,
  OPF_AdSize = 2,
  OPF_REP = 4,
  OPF_REP = 8,
};

void clearFlags() { Flags = 0; }
void setFlag(OpPrefixFlag F) { Flags |= (unsigned)F; }
bool isFlagSet(OpPrefixFlag F) const { return !!(Flags & (unsigned)F); }

[avt77]

I fixed issues raised by Craig and Simon.

[avt77]

Craig,
You reviewed https://reviews.llvm.org/D36788 which was the first step of fixing issues with prefixes: could you review this new patch as well? In fact it's simply an extended version of D36788 and covers issues raised by echristo.

[craig.topper]

I'm not sure I can approve growing the size of MCInst. Though I can't see anyway around it. @rafael what do you think?

[craig.topper]

The new test cases don't test anything because they aren't running FileCheck.

test/MC/Disassembler/X86/prefixes-i386.s

1 ↗

(On Diff #115822)

This needs to be named .txt not .s

You also need to pass the output to FileCheck

test/MC/Disassembler/X86/prefixes-x86_64.s

1 ↗

(On Diff #115822)

This file needs be named .txt not .s

You also need to pass the output to FileCheck

[avt77]

I fixed tests mentioned by Craig. And about extension of MCInst: this opaque data (not simple flag) could be really useful for other components (not only for disassembler).

[avt77]

There was a email thread about the issues in this patch. To keep track of those emails I'm putting them here:

It was added when I’ve start poking around with prefixes, to implement the proper recognition of xaquire/xrelease (rL311309).
I can suggest some additional views to the matter at hand:
Enhancing prefix digestion by the parser is highly recommended – aforementioned FIXME note describes those issues I’ve found on a brief exploring, surly there’s more.
Currently it is emitted as a standalone instruction, which I don’t see much sense in.
IMHO, we should aggregate prefixes till we have consumed an ‘actual’ instruction, and then make queries about whether they form a legal combination, and I deem it to be the right course for the disassembler as well, unless we don’t care about tolerating nonsense in disassembly (how does others disassemblers handle such phenomena?)
Personally, I see prefixes as a part of a particular instruction, so at least on the concept level I’m in favor of ‘Flags’.
More generally, whether producing multiple MCInsts, using flag or whatever other approach – it’s technicalities.
Agreement should be first reached on what would be considered as a proper handling for both ends (parser, disassembler).
p.s. can you guyz please use Phab for further discussion? Hard (for me) to keep track on mail correspondence.

From: Andrew Tischenko [mailto:[e-mail]]
BTW, JFYI, I found the following comment in the source:

// FIXME:
// Enhace prefixes integrity robustness. for example, following forms
// are currently tolerated:
// repz repnz <insn>    ; GAS errors for the use of two similar prefixes
// lock addq %rax, %rbx ; Destination operand must be of memory type
// xacquire <insn>      ; xacquire must be accompanied by 'lock'

The approach with Flag will allow to implement it.
Andrew

On 27.09.2017 12:18, Andrew Tischenko wrote:

OK, I'll try to change the assembler properly but there are some questions:

    Should I do it in the same patch?
    Currently if we have:

repz repnz repe cmpsb

    then we produce with 'llvm-mc -triple x86_64-unknown-unknown -x86-asm-syntax=intel -show-encoding intel-syntax.s':

        rep                             # encoding: [0xf3]
        repne                           # encoding: [0xf2]
        rep                             # encoding: [0xf3]
        cmpsb   %es:(%rdi), (%rsi)      # encoding: [0xa6]

    but after the change we'll get only the following:

        rep                             # encoding: [0xf3]
        cmpsb   %es:(%rdi), (%rsi)      # encoding: [0xa6]

    Is it OK? (IMHO, Yes.)

    If YES, do we need any warnings here? (IMHO, No.)

Andrew

On 26.09.2017 22:31, Rafael Avila de Espindola wrote:

    The assembler and disassembler should use the same path.
    I would be OK with always producing 1 or N instructions, as long as both
    the assembler and disassembler do the same. That is, it is OK to have
    Flags, as long as the assembler uses that instead of creating a separate
    instruction for prefixes.
    It seems that allowing the disassembler to create multiple instructions
    would have the advantage of not needing Flags, but that is secondary
    IMHO.
    Cheers,
    Rafael

    Craig Topper <[e-mail]> writes:

        Here's my understanding of what I think happens today.
        -For a very select few instructions if the AsmParser sees a repne/repe
        prefix it creates a special version of the instruction that has the REP
        bits set in TSFlags. For any other instruction it emits the repne/rep/repe
        as a separate MCInst.
        -For the disassembler if it sees a repne/repe byte at the start that it
        doesn't think goes with an instruction it will emit a MCInst containing
        just the REP.
        -If the disassembler encounters a repne/repe byte not at the start of the
        instruction that doesn't go with the instruction we drop it and don't print
        anything. The disassembler interface only allows us to return one
        instruction. So we can't return a separate repne/repe instruction and a
        real instruction from the same byte sequence. I don't believe the assembler
        can ever produce a byte sequence that hits this case, but that doesn't mean
        some binary couldn't contain that string of bytes created by hand. So this
        patch is trying to preserve the extra prefix information in the one MCInst
        we're allowed to emit. Maybe another option would be to allow creating
        multiple MCInsts from the disassembler?
        ~Craig

        On Tue, Sep 26, 2017 at 10:37 AM, Rafael Avila de Espindola <
        [e-mail]> wrote:
            The question is why it is different for disassembler than for the
            assembler?
            How does the assembler handle trepne?
          Cheers,
           Rafael

            Andrew Tischenko <[e-mail]> writes:
                It is not a simple flag, it's some data. And this data could be useful
                for any other component because it's some opaque info which could be
                send via MCInst from one low level target component to another one.
                Without this (additional) data MCInst loosing (potentially very useful)
                info about the given instruction.

                Andrew

                On 25.09.2017 22:05, Rafael Avila de Espindola wrote:

                    Having a flag field that is used only on disassembly seems wrong.
                    Don't we support parsing our own output? I don't see trepne in any .s
                    test for example.

                    Cheers,
                    Rafael

[avt77]

Rafael, Craig,

After of some investigation of AsmParser I realised that assembler modification to support Flags for proper prefixes elaboration will require signature change of at least 2 virtual functions: ParseInstruction and MatchAndEmitInstruction. The reason of such change is very simple: ParseInstruction does real parsing (and as result could track prefixes) but does not deal with MCInst while MatchAndEmitInstruction create MCInst. This signature modifications will force massive changes in the sources that's why it will be difficult to review.

The question is: could we accept the current patch as it is (only disassembler is being supported) and follow up with a new patch which will include the corresponding assembler changes?
Or the big patch is OK?

[avt77]

Alternatively, I could add a new X86Operand type (e.g. Prefix): as result I will not change any signature but it will be X86-specific only. What's better from your point of view?

[avt77]

I re-implemented assembler in case of working with X86-prefixes. Now both X86-assembler and X86-disassembler use the new Flags field from MCInst. As result now it's possible to track several prefixes for one instr and now one prefix is not a separate instruction but only is the parameter of the one. I tried to keep the current tests unchanged where it's possible. And I did not extend/change any diagnostic related to prefixes: it should be done in the follow up patches.

[avt77]

I added a test to cover PR32809.
Guys, could you speed up the review? In fact you already reviewed everything except X86-ASM changes: I did it to have one path for both assembler and disassembler as we agreed in our previuos discussions.

[craig.topper]

lib/Target/X86/AsmParser/X86AsmParser.cpp

2887

Use isPrefix()?

2888

I'm not sure this isn't a use after free. pop_back_val is going to return a std::unique_ptr which you dereferenced, but I'm not convinced anything is keeping the std::unique_ptr alive.

lib/Target/X86/AsmParser/X86Operand.h

395

isPrefix*

[avt77]

lib/Target/X86/AsmParser/X86AsmParser.cpp

2888

It seems that everything is OK here because

C++  Utilities library  Dynamic memory management std::unique_ptr

typename std::add_lvalue_reference<T>::type operator*() const; (1) (since C++11)
pointer operator->() const noexcept; (2) (since C++11)

operator* and operator-> provide access to the object owned by *this.
The behavior is undefined if get() == nullptr

Parameters
(none)

Return value

1. Returns the object owned by *this, equivalent to *get().
2. Returns a pointer to the object owned by *this, i.e. get().

Am I right?

[avt77]

I added usage of isPrefix() accordingly to Craig requirement.

[avt77]

I added tests covering PR21640. Now this patch covers PR7709, PR17697, PR19251, PR32809 and PR21640.

[craig.topper]

lib/Target/X86/AsmParser/X86AsmParser.cpp

2888

The problem isn't with the operator*. It's about the ordering of when the destructor for the std::unqiue_ptr returned by pop_back_val is executed. I believe it will happen as soon as that line ends since it's an unnamed temporary. So the destructor will run and delete the object the unique_ptr is pointing at. But you're still holding a reference to that object. So it's a use after free.

I think you should assign to Prefixes while the object is still in the vector using .back(), and then once you're done with that just call pop_back() on the vector.

[avt77]

lib/Target/X86/AsmParser/X86AsmParser.cpp

2888

I'm not sure you're right because they say about "end of scope" to invoke destructor and here the "scope" should be "if" statement. But to be absolutely safe I'll do it. Tnx.

[avt77]

Safe implementation for std::unique_ptr usage was done (raised by Craig).

[craig.topper]

Thanks for fixing that. Scope only applies to named objects, the unique_ptr here has no name. https://stackoverflow.com/questions/2298781/why-do-un-named-c-objects-destruct-before-the-scope-block-ends.

LGTM other than the one comment.

[avt77]

The patch was committed as rL315899.

include/llvm/MC/MCInst.h

 template <> struct isPodLike<MCOperand> { static const bool value = true; };
 
 /// \brief Instances of this class represent a single low-level machine
 /// instruction.
 class MCInst {
   unsigned Opcode = 0;
   SMLoc Loc;
   SmallVector<MCOperand, 8> Operands;
+  // These flags could be used to pass some info from one target subcomponent
+  // to another, for example, from disassembler to asm printer. The values of
+  // the flags have any sense on target level only (e.g. prefixes on x86).
+  unsigned Flags = 0;
 
 public:
   MCInst() = default;
 
   void setOpcode(unsigned Op) { Opcode = Op; }
   unsigned getOpcode() const { return Opcode; }
 
+  void setFlags(unsigned F) { Flags = F; }
+  unsigned getFlags() const { return Flags; }
+
   void setLoc(SMLoc loc) { Loc = loc; }
   SMLoc getLoc() const { return Loc; }
 
   const MCOperand &getOperand(unsigned i) const { return Operands[i]; }
   MCOperand &getOperand(unsigned i) { return Operands[i]; }
   unsigned getNumOperands() const { return Operands.size(); }
 
   void addOperand(const MCOperand &Op) { Operands.push_back(Op); }

lib/Target/X86/AsmParser/X86AsmParser.cpp

if (ComparisonCode != ~0U) {
       const MCExpr *ImmOp = MCConstantExpr::create(ComparisonCode,
                                                    getParser().getContext());
       Operands.push_back(X86Operand::CreateImm(ImmOp, NameLoc, NameLoc));
 
       PatchedName = PatchedName.substr(PatchedName.size() - CCIdx);
     }
   }
 
-  Operands.push_back(X86Operand::CreateToken(PatchedName, NameLoc));
 
   // Determine whether this is an instruction prefix.
   // FIXME:
   // Enhance prefixes integrity robustness. for example, following forms
   // are currently tolerated:
   // repz repnz <insn>    ; GAS errors for the use of two similar prefixes
   // lock addq %rax, %rbx ; Destination operand must be of memory type
   // xacquire <insn>      ; xacquire must be accompanied by 'lock'
   bool isPrefix = StringSwitch<bool>(Name)
-    .Cases("lock",
-           "rep",       "repe",
-           "repz",      "repne",
-           "repnz",     "rex64",
-           "data32",    "data16",   true)
-    .Cases("xacquire",  "xrelease", true)
-    .Cases("acquire",   "release",  isParsingIntelSyntax())
+                      .Cases("rex64", "data32", "data16", true)
+                      .Cases("xacquire", "xrelease", true)
+                      .Cases("acquire", "release", isParsingIntelSyntax())
                       .Default(false);
 
+  auto isLockRepeatPrefix = [](StringRef N) {
+    return StringSwitch<bool>(N)
+        .Cases("lock", "rep", "repe", "repz", "repne", "repnz", true)
+        .Default(false);
+  };
+
   bool CurlyAsEndOfStatement = false;
+
+  unsigned Flags = X86::IP_NO_PREFIX;
+  while (isLockRepeatPrefix(Name.lower())) {
+    unsigned Prefix =
+        StringSwitch<unsigned>(Name)
+            .Cases("lock", "lock", X86::IP_HAS_LOCK)
+            .Cases("rep", "repe", "repz", X86::IP_HAS_REPEAT)
+            .Cases("repne", "repnz", X86::IP_HAS_REPEAT_NE)
+            .Default(X86::IP_NO_PREFIX); // Invalid prefix (impossible)
+    Flags |= Prefix;
+    Name = Parser.getTok().getString();
+    Parser.Lex(); // eat the prefix
+    // Hack: we could have something like
+    //    "lock; cmpxchg16b $1" or "lock\0A\09incl" or "lock/incl"
+    while (Name.startswith(";") || Name.startswith("\n") ||
+           Name.startswith("\t") or Name.startswith("/")) {
+      Name = Parser.getTok().getString();
+      Parser.Lex(); // go to next prefix or instr
+    }
+  }
+
+  if (Flags)
+    PatchedName = Name;
+  Operands.push_back(X86Operand::CreateToken(PatchedName, NameLoc));
+
   // This does the actual operand parsing.  Don't parse any more if we have a
   // prefix juxtaposed with an operation like "lock incl 4(%rax)", because we
   // just want to parse the "lock" as the first instruction and the "incl" as
   // the next one.
   if (getLexer().isNot(AsmToken::EndOfStatement) && !isPrefix) {
-
     // Parse '*' modifier.
     if (getLexer().is(AsmToken::Star))
       Operands.push_back(X86Operand::CreateToken("*", consumeToken()));
 
     // Read the operands.
     while(1) {
       if (std::unique_ptr<X86Operand> Op = ParseOperand()) {
         Operands.push_back(std::move(Op));
if ((Name == "xlat" || Name == "xlatb") && Operands.size() == 2) {
     if (Op1.isMem8()) {
       Warning(Op1.getStartLoc(), "memory operand is only for determining the "
                                  "size, (R|E)BX will be used for the location");
       Operands.pop_back();
       static_cast<X86Operand &>(*Operands[0]).setTokenValue("xlatb");
     }
   }
 
+  if (Flags)
+    Operands.push_back(X86Operand::CreatePrefix(Flags, NameLoc, NameLoc));
   return false;
 }
 
 bool X86AsmParser::processInstruction(MCInst &Inst, const OperandVector &Ops) {
   return false;
 }
 
 static const char *getSubtargetFeatureName(uint64_t Val);
bool X86AsmParser::ErrorMissingFeature(SMLoc IDLoc, uint64_t ErrorInfo,
   for (unsigned i = 0; i < (sizeof(ErrorInfo)*8-1); ++i) {
     if (ErrorInfo & Mask)
       OS << ' ' << getSubtargetFeatureName(ErrorInfo & Mask);
     Mask <<= 1;
   }
   return Error(IDLoc, OS.str(), SMRange(), MatchingInlineAsm);
 }
 
+static unsigned getPrefixes(OperandVector &Operands) {
+  unsigned Result = 0;
+  X86Operand &Prefix = static_cast<X86Operand &>(*Operands.back());
+  if (Prefix.isPrefix()) {
+    Result = Prefix.getPrefix();
+    Operands.pop_back();
+  }
+  return Result;
+}
+
 bool X86AsmParser::MatchAndEmitATTInstruction(SMLoc IDLoc, unsigned &Opcode,
                                               OperandVector &Operands,
                                               MCStreamer &Out,
                                               uint64_t &ErrorInfo,
                                               bool MatchingInlineAsm) {
   assert(!Operands.empty() && "Unexpect empty operand list!");
   X86Operand &Op = static_cast<X86Operand &>(*Operands[0]);
   assert(Op.isToken() && "Leading operand should always be a mnemonic!");
   SMRange EmptyRange = None;
 
   // First, handle aliases that expand to multiple instructions.
   MatchFPUWaitAlias(IDLoc, Op, Operands, Out, MatchingInlineAsm);
 
   bool WasOriginallyInvalidOperand = false;
+  unsigned Prefixes = getPrefixes(Operands);
+
   MCInst Inst;
 
+  if (Prefixes)
+    Inst.setFlags(Prefixes);
+
   // First, try a direct match.
   switch (MatchInstruction(Operands, Inst, ErrorInfo, MatchingInlineAsm,
                            isParsingIntelSyntax())) {
   default: llvm_unreachable("Unexpected match result!");
   case Match_Success:
     // Some instructions need post-processing to, for example, tweak which
     // encoding is selected. Loop on it while changes happen so the
     // individual transformations can chain off each other.
bool X86AsmParser::MatchAndEmitIntelInstruction(SMLoc IDLoc, unsigned &Opcode,
                                                 uint64_t &ErrorInfo,
                                                 bool MatchingInlineAsm) {
   assert(!Operands.empty() && "Unexpect empty operand list!");
   X86Operand &Op = static_cast<X86Operand &>(*Operands[0]);
   assert(Op.isToken() && "Leading operand should always be a mnemonic!");
   StringRef Mnemonic = Op.getToken();
   SMRange EmptyRange = None;
   StringRef Base = Op.getToken();
+  unsigned Prefixes = getPrefixes(Operands);
 
   // First, handle aliases that expand to multiple instructions.
   MatchFPUWaitAlias(IDLoc, Op, Operands, Out, MatchingInlineAsm);
 
   MCInst Inst;
 
+  if (Prefixes)
+    Inst.setFlags(Prefixes);
+
   // Find one unsized memory operand, if present.
   X86Operand *UnsizedMemOp = nullptr;
   for (const auto &Op : Operands) {
     X86Operand *X86Op = static_cast<X86Operand *>(Op.get());
     if (X86Op->isMemUnsized()) {
       UnsizedMemOp = X86Op;
       // Have we found an unqualified memory operand,
       // break. IA allows only one memory operand.

lib/Target/X86/AsmParser/X86Operand.h

 #include <cassert>
 #include <memory>
 
 namespace llvm {
 
 /// X86Operand - Instances of this class represent a parsed X86 machine
 /// instruction.
 struct X86Operand : public MCParsedAsmOperand {
-  enum KindTy {
-    Token,
-    Register,
-    Immediate,
-    Memory
-  } Kind;
+  enum KindTy { Token, Register, Immediate, Memory, Prefix } Kind;
 
   SMLoc StartLoc, EndLoc;
   SMLoc OffsetOfLoc;
   StringRef SymName;
   void *OpDecl;
   bool AddressOf;
 
   struct TokOp {
     const char *Data;
     unsigned Length;
   };
 
   struct RegOp {
     unsigned RegNo;
   };
 
+  struct PrefOp {
+    unsigned Prefixes;
+  };
+
   struct ImmOp {
     const MCExpr *Val;
   };
 
   struct MemOp {
     unsigned SegReg;
     const MCExpr *Disp;
     unsigned BaseReg;
     unsigned IndexReg;
     unsigned Scale;
     unsigned Size;
     unsigned ModeSize;
 
     /// If the memory operand is unsized and there are multiple instruction
     /// matches, prefer the one with this size.
     unsigned FrontendSize;
   };
 
   union {
     struct TokOp Tok;
     struct RegOp Reg;
     struct ImmOp Imm;
     struct MemOp Mem;
+    struct PrefOp Pref;
   };
 
   X86Operand(KindTy K, SMLoc Start, SMLoc End)
     : Kind(K), StartLoc(Start), EndLoc(End) {}
 
   StringRef getSymName() override { return SymName; }
   void *getOpDecl() override { return OpDecl; }
 
void setTokenValue(StringRef Value) {
     Tok.Length = Value.size();
   }
 
   unsigned getReg() const override {
     assert(Kind == Register && "Invalid access!");
     return Reg.RegNo;
   }
 
+  unsigned getPrefix() const {
+    assert(Kind == Prefix && "Invalid access!");
+    return Pref.Prefixes;
+  }
+
   const MCExpr *getImm() const {
     assert(Kind == Immediate && "Invalid access!");
     return Imm.Val;
   }
 
   const MCExpr *getMemDisp() const {
     assert(Kind == Memory && "Invalid access!");
     return Mem.Disp;
struct X86Operand : public MCParsedAsmOperand {
   }
   bool isMemOffs64_32() const {
     return isMemOffs() && Mem.ModeSize == 64 && (!Mem.Size || Mem.Size == 32);
   }
   bool isMemOffs64_64() const {
     return isMemOffs() && Mem.ModeSize == 64 && (!Mem.Size || Mem.Size == 64);
   }
 
+  bool isPrefix() const { return Kind == Prefix; }
   bool isReg() const override { return Kind == Register; }
 
   bool isGR32orGR64() const {
     return Kind == Register &&
       (X86MCRegisterClasses[X86::GR32RegClassID].contains(getReg()) ||
       X86MCRegisterClasses[X86::GR64RegClassID].contains(getReg()));
   }
 
CreateReg(unsigned RegNo, SMLoc StartLoc, SMLoc EndLoc,
     Res->Reg.RegNo = RegNo;
     Res->AddressOf = AddressOf;
     Res->OffsetOfLoc = OffsetOfLoc;
     Res->SymName = SymName;
     Res->OpDecl = OpDecl;
     return Res;
   }
 
+  static std::unique_ptr<X86Operand>
+  CreatePrefix(unsigned Prefixes, SMLoc StartLoc, SMLoc EndLoc) {
+    auto Res = llvm::make_unique<X86Operand>(Prefix, StartLoc, EndLoc);
+    Res->Pref.Prefixes = Prefixes;
+    return Res;
+  }
+
   static std::unique_ptr<X86Operand> CreateImm(const MCExpr *Val,
                                                SMLoc StartLoc, SMLoc EndLoc) {
     auto Res = llvm::make_unique<X86Operand>(Immediate, StartLoc, EndLoc);
     Res->Imm.Val = Val;
     return Res;
   }
 
   /// Create an absolute memory operand.

lib/Target/X86/Disassembler/X86Disassembler.cpp

 //   table emitter and the disassembler.
 // X86DisassemblerDecoder.h contains the public interface of the decoder,
 //   factored out into C for possible use by other projects.
 // X86DisassemblerDecoder.c contains the source code of the decoder, which is
 //   responsible for steps 1-6.
 //
 //===----------------------------------------------------------------------===//
 
+#include "MCTargetDesc/X86BaseInfo.h"
 #include "MCTargetDesc/X86MCTargetDesc.h"
 #include "X86DisassemblerDecoder.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCDisassembler/MCDisassembler.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCSubtargetInfo.h"
int Ret = decodeInstruction(&InternalInstr, regionReader, (const void *)&R,
                               LoggerFn, (void *)&VStream,
                               (const void *)MII.get(), Address, fMode);
 
   if (Ret) {
     Size = InternalInstr.readerCursor - Address;
     return Fail;
   } else {
     Size = InternalInstr.length;
-    return (!translateInstruction(Instr, InternalInstr, this)) ? Success : Fail;
+    bool Ret = translateInstruction(Instr, InternalInstr, this);
+    if (!Ret) {
+      unsigned Flags = X86::IP_NO_PREFIX;
+      if (InternalInstr.hasAdSize)
+        Flags |= X86::IP_HAS_AD_SIZE;
+      if (!InternalInstr.mandatoryPrefix) {
+        if (InternalInstr.hasOpSize)
+          Flags |= X86::IP_HAS_OP_SIZE;
+        if (InternalInstr.repeatPrefix == 0xf2)
+          Flags |= X86::IP_HAS_REPEAT_NE;
+        else if (InternalInstr.repeatPrefix == 0xf3 &&
+                 // It should not be 'pause' f3 90
+                 InternalInstr.opcode != 0x90)
+          Flags |= X86::IP_HAS_REPEAT;
+      }
+      Instr.setFlags(Flags);
+    }
+    return (!Ret) ? Success : Fail;
   }
 }
 
 //
 // Private code that translates from struct InternalInstructions to MCInsts.
 //
 
 /// translateRegister - Translates an internal register to the appropriate LLVM

 /// translateSrcIndex   - Appends a source index operand to an MCInst.
 ///
 /// @param mcInst       - The MCInst to append to.
 /// @param insn         - The internal instruction.
 static bool translateSrcIndex(MCInst &mcInst, InternalInstruction &insn) {
   unsigned baseRegNo;
 
   if (insn.mode == MODE_64BIT)
-    baseRegNo = insn.prefixPresent[0x67] ? X86::ESI : X86::RSI;
+    baseRegNo = insn.hasAdSize ? X86::ESI : X86::RSI;
   else if (insn.mode == MODE_32BIT)
-    baseRegNo = insn.prefixPresent[0x67] ? X86::SI : X86::ESI;
+    baseRegNo = insn.hasAdSize ? X86::SI : X86::ESI;
   else {
     assert(insn.mode == MODE_16BIT);
-    baseRegNo = insn.prefixPresent[0x67] ? X86::ESI : X86::SI;
+    baseRegNo = insn.hasAdSize ? X86::ESI : X86::SI;
   }
   MCOperand baseReg = MCOperand::createReg(baseRegNo);
   mcInst.addOperand(baseReg);
 
   MCOperand segmentReg;
   segmentReg = MCOperand::createReg(segmentRegnums[insn.segmentOverride]);
   mcInst.addOperand(segmentReg);
   return false;
 }
 
 /// translateDstIndex   - Appends a destination index operand to an MCInst.
 ///
 /// @param mcInst       - The MCInst to append to.
 /// @param insn         - The internal instruction.
 
 static bool translateDstIndex(MCInst &mcInst, InternalInstruction &insn) {
   unsigned baseRegNo;
 
   if (insn.mode == MODE_64BIT)
-    baseRegNo = insn.prefixPresent[0x67] ? X86::EDI : X86::RDI;
+    baseRegNo = insn.hasAdSize ? X86::EDI : X86::RDI;
   else if (insn.mode == MODE_32BIT)
-    baseRegNo = insn.prefixPresent[0x67] ? X86::DI : X86::EDI;
+    baseRegNo = insn.hasAdSize ? X86::DI : X86::EDI;
   else {
     assert(insn.mode == MODE_16BIT);
-    baseRegNo = insn.prefixPresent[0x67] ? X86::EDI : X86::DI;
+    baseRegNo = insn.hasAdSize ? X86::EDI : X86::DI;
   }
   MCOperand baseReg = MCOperand::createReg(baseRegNo);
   mcInst.addOperand(baseReg);
   return false;
 }
 
 /// translateImmediate  - Appends an immediate operand to an MCInst.
 ///

lib/Target/X86/Disassembler/X86DisassemblerDecoder.h

struct InternalInstruction {
   DisassemblerMode mode;
   // The start of the instruction, usable with the reader
   uint64_t startLocation;
   // The length of the instruction, in bytes
   size_t length;
 
   // Prefix state
 
-  // 1 if the prefix byte corresponding to the entry is present; 0 if not
-  uint8_t prefixPresent[0x100];
-  // contains the location (for use with the reader) of the prefix byte
-  uint64_t prefixLocations[0x100];
+  // The possible mandatory prefix
+  uint8_t mandatoryPrefix;
   // The value of the vector extension prefix(EVEX/VEX/XOP), if present
   uint8_t vectorExtensionPrefix[4];
   // The type of the vector extension prefix
   VectorExtensionType vectorExtensionType;
   // The value of the REX prefix, if present
   uint8_t rexPrefix;
-  // The location where a mandatory prefix would have to be (i.e., right before
-  // the opcode, or right before the REX prefix if one is present).
-  uint64_t necessaryPrefixLocation;
   // The segment override type
   SegmentOverride segmentOverride;
   // 1 if the prefix byte, 0xf2 or 0xf3 is xacquire or xrelease
   bool xAcquireRelease;
 
+  // Address-size override
+  bool hasAdSize;
+  // Operand-size override
+  bool hasOpSize;
+  // The repeat prefix if any
+  uint8_t repeatPrefix;
+
   // Sizes of various critical pieces of data, in bytes
   uint8_t registerSize;
   uint8_t addressSize;
   uint8_t displacementSize;
   uint8_t immediateSize;
 
   // Offsets from the start of the instruction to the pieces of data, which is
   // needed to find relocation entries for adding symbolic operands.

lib/Target/X86/Disassembler/X86DisassemblerDecoder.cpp

static void dbgprintf(struct InternalInstruction* insn,
 
   va_start(ap, format);
   (void)vsnprintf(buffer, sizeof(buffer), format, ap);
   va_end(ap);
 
   insn->dlog(insn->dlogArg, buffer);
 }
 
+static bool isREX(struct InternalInstruction *insn, uint8_t prefix) {
+  if (insn->mode == MODE_64BIT)
+    return prefix >= 0x40 && prefix <= 0x4f;
+  return false;
+}
+
 /*
- * setPrefixPresent - Marks that a particular prefix is present at a particular
- *   location.
+ * setPrefixPresent - Marks that a particular prefix is present as mandatory
  *
  * @param insn      - The instruction to be marked as having the prefix.
  * @param prefix    - The prefix that is present.
- * @param location  - The location where the prefix is located (in the address
- *                    space of the instruction's reader).
  */
-static void setPrefixPresent(struct InternalInstruction* insn,
-                                    uint8_t prefix,
-                                    uint64_t location)
-{
-  insn->prefixPresent[prefix] = 1;
-  insn->prefixLocations[prefix] = location;
+static void setPrefixPresent(struct InternalInstruction *insn, uint8_t prefix) {
+  uint8_t nextByte;
+  switch (prefix) {
+  case 0xf2:
+  case 0xf3:
+    if (lookAtByte(insn, &nextByte))
+      break;
+    // TODO:
+    //  1. There could be several 0x66
+    //  2. if (nextByte == 0x66) and nextNextByte != 0x0f then
+    //      it's not mandatory prefix
+    //  3. if (nextByte >= 0x40 && nextByte <= 0x4f) it's REX and we need
+    //     0x0f exactly after it to be mandatory prefix
+    if (isREX(insn, nextByte) || nextByte == 0x0f || nextByte == 0x66)
+      // The last of 0xf2 /0xf3 is mandatory prefix
+      insn->mandatoryPrefix = prefix;
+    insn->repeatPrefix = prefix;
+    break;
+  case 0x66:
+    if (lookAtByte(insn, &nextByte))
+      break;
+    // 0x66 can't overwrite existing mandatory prefix and should be ignored
+    if (!insn->mandatoryPrefix && (nextByte == 0x0f || isREX(insn, nextByte)))
+      insn->mandatoryPrefix = prefix;
+    break;
   }
-
-/*
- * isPrefixAtLocation - Queries an instruction to determine whether a prefix is
- *   present at a given location.
- *
- * @param insn      - The instruction to be queried.
- * @param prefix    - The prefix.
- * @param location  - The location to query.
- * @return          - Whether the prefix is at that location.
- */
-static bool isPrefixAtLocation(struct InternalInstruction* insn,
-                               uint8_t prefix,
-                               uint64_t location)
-{
-  return insn->prefixPresent[prefix] == 1 &&
-     insn->prefixLocations[prefix] == location;
 }
 
 /*
  * readPrefixes - Consumes all of an instruction's prefix bytes, and marks the
  *   instruction as having them.  Also sets the instruction's default operand,
  *   address, and other relevant data sizes to report operands correctly.
  *
  * @param insn  - The instruction whose prefixes are to be read.
  * @return      - 0 if the instruction could be read until the end of the prefix
  *                bytes, and no prefixes conflicted; nonzero otherwise.
  */
 static int readPrefixes(struct InternalInstruction* insn) {
   bool isPrefix = true;
-  bool prefixGroups[4] = { false };
-  uint64_t prefixLocation;
   uint8_t byte = 0;
   uint8_t nextByte;
 
-  bool hasAdSize = false;
-  bool hasOpSize = false;
-
   dbgprintf(insn, "readPrefixes()");
 
   while (isPrefix) {
-    prefixLocation = insn->readerCursor;
-
     /* If we fail reading prefixes, just stop here and let the opcode reader deal with it */
     if (consumeByte(insn, &byte))
       break;
 
     /*
      * If the byte is a LOCK/REP/REPNE prefix and not a part of the opcode, then
      * break and let it be disassembled as a normal "instruction".
      */
-    if (insn->readerCursor - 1 == insn->startLocation && byte == 0xf0)
+    if (insn->readerCursor - 1 == insn->startLocation && byte == 0xf0) // LOCK
       break;
 
-    if (insn->readerCursor - 1 == insn->startLocation
-        && (byte == 0xf2 || byte == 0xf3)
-        && !lookAtByte(insn, &nextByte))
-    {
+    if ((byte == 0xf2 || byte == 0xf3) && !lookAtByte(insn, &nextByte)) {
       /*
        * If the byte is 0xf2 or 0xf3, and any of the following conditions are
        * met:
        * - it is followed by a LOCK (0xf0) prefix
        * - it is followed by an xchg instruction
        * then it should be disassembled as a xacquire/xrelease not repne/rep.
        */
-      if ((byte == 0xf2 || byte == 0xf3) &&
-          ((nextByte == 0xf0) ||
-          ((nextByte & 0xfe) == 0x86 || (nextByte & 0xf8) == 0x90)))
+      if (((nextByte == 0xf0) ||
+           ((nextByte & 0xfe) == 0x86 || (nextByte & 0xf8) == 0x90))) {
         insn->xAcquireRelease = true;
+        if (!(byte == 0xf3 && nextByte == 0x90)) // PAUSE instruction support
+          break;
+      }
       /*
        * Also if the byte is 0xf3, and the following condition is met:
        * - it is followed by a "mov mem, reg" (opcode 0x88/0x89) or
        *                       "mov mem, imm" (opcode 0xc6/0xc7) instructions.
        * then it should be disassembled as an xrelease not rep.
        */
-      if (byte == 0xf3 &&
-          (nextByte == 0x88 || nextByte == 0x89 ||
-           nextByte == 0xc6 || nextByte == 0xc7))
+      if (byte == 0xf3 && (nextByte == 0x88 || nextByte == 0x89 ||
+                           nextByte == 0xc6 || nextByte == 0xc7)) {
         insn->xAcquireRelease = true;
-      if (insn->mode == MODE_64BIT && (nextByte & 0xf0) == 0x40) {
-        if (consumeByte(insn, &nextByte))
+        if (nextByte != 0x90) // PAUSE instruction support
+          break;
+      }
+      if (isREX(insn, nextByte)) {
+        uint8_t nnextByte;
+        // Go to REX prefix after the current one
+        if (consumeByte(insn, &nnextByte))
           return -1;
-        if (lookAtByte(insn, &nextByte))
+        // We should be able to read next byte after REX prefix
+        if (lookAtByte(insn, &nnextByte))
           return -1;
         unconsumeByte(insn);
       }
-      if (nextByte != 0x0f && nextByte != 0x90)
-        break;
     }
 
     switch (byte) {
     case 0xf0:  /* LOCK */
     case 0xf2:  /* REPNE/REPNZ */
     case 0xf3:  /* REP or REPE/REPZ */
-      if (prefixGroups[0])
-        dbgprintf(insn, "Redundant Group 1 prefix");
-      prefixGroups[0] = true;
-      setPrefixPresent(insn, byte, prefixLocation);
+      setPrefixPresent(insn, byte);
       break;
     case 0x2e:  /* CS segment override -OR- Branch not taken */
     case 0x36:  /* SS segment override -OR- Branch taken */
     case 0x3e:  /* DS segment override */
     case 0x26:  /* ES segment override */
     case 0x64:  /* FS segment override */
     case 0x65:  /* GS segment override */
       switch (byte) {
case 0x65:  /* GS segment override */
         break;
       case 0x65:
         insn->segmentOverride = SEG_OVERRIDE_GS;
         break;
       default:
         debug("Unhandled override");
         return -1;
       }
-      if (prefixGroups[1])
-        dbgprintf(insn, "Redundant Group 2 prefix");
-      prefixGroups[1] = true;
-      setPrefixPresent(insn, byte, prefixLocation);
+      setPrefixPresent(insn, byte);
       break;
     case 0x66:  /* Operand-size override */
-      if (prefixGroups[2])
-        dbgprintf(insn, "Redundant Group 3 prefix");
-      prefixGroups[2] = true;
-      hasOpSize = true;
-      setPrefixPresent(insn, byte, prefixLocation);
+      insn->hasOpSize = true;
+      setPrefixPresent(insn, byte);
       break;
     case 0x67:  /* Address-size override */
-      if (prefixGroups[3])
-        dbgprintf(insn, "Redundant Group 4 prefix");
-      prefixGroups[3] = true;
-      hasAdSize = true;
-      setPrefixPresent(insn, byte, prefixLocation);
+      insn->hasAdSize = true;
+      setPrefixPresent(insn, byte);
       break;
     default:    /* Not a prefix byte */
       isPrefix = false;
       break;
     }
 
     if (isPrefix)
       dbgprintf(insn, "Found prefix 0x%hhx", byte);
if (byte == 0x62) {
     }
 
     if ((insn->mode == MODE_64BIT || (byte1 & 0xc0) == 0xc0) &&
        ((~byte1 & 0xc) == 0xc) && ((byte2 & 0x4) == 0x4)) {
       insn->vectorExtensionType = TYPE_EVEX;
     } else {
       unconsumeByte(insn); /* unconsume byte1 */
       unconsumeByte(insn); /* unconsume byte  */
-      insn->necessaryPrefixLocation = insn->readerCursor - 2;
     }
 
     if (insn->vectorExtensionType == TYPE_EVEX) {
       insn->vectorExtensionPrefix[0] = byte;
       insn->vectorExtensionPrefix[1] = byte1;
       if (consumeByte(insn, &insn->vectorExtensionPrefix[2])) {
         dbgprintf(insn, "Couldn't read third byte of EVEX prefix");
         return -1;
static int readPrefixes(struct InternalInstruction* insn) {
   } else if (byte == 0xc4) {
     uint8_t byte1;
 
     if (lookAtByte(insn, &byte1)) {
       dbgprintf(insn, "Couldn't read second byte of VEX");
       return -1;
     }
 
-    if (insn->mode == MODE_64BIT || (byte1 & 0xc0) == 0xc0) {
+    if (insn->mode == MODE_64BIT || (byte1 & 0xc0) == 0xc0)
       insn->vectorExtensionType = TYPE_VEX_3B;
-      insn->necessaryPrefixLocation = insn->readerCursor - 1;
-    } else {
+    else
       unconsumeByte(insn);
-      insn->necessaryPrefixLocation = insn->readerCursor - 1;
-    }
 
     if (insn->vectorExtensionType == TYPE_VEX_3B) {
       insn->vectorExtensionPrefix[0] = byte;
       consumeByte(insn, &insn->vectorExtensionPrefix[1]);
       consumeByte(insn, &insn->vectorExtensionPrefix[2]);
 
       /* We simulate the REX prefix for simplicity's sake */
 
-      if (insn->mode == MODE_64BIT) {
+      if (insn->mode == MODE_64BIT)
         insn->rexPrefix = 0x40
                         | (wFromVEX3of3(insn->vectorExtensionPrefix[2]) << 3)
                         | (rFromVEX2of3(insn->vectorExtensionPrefix[1]) << 2)
                         | (xFromVEX2of3(insn->vectorExtensionPrefix[1]) << 1)
                         | (bFromVEX2of3(insn->vectorExtensionPrefix[1]) << 0);
-      }
 
       dbgprintf(insn, "Found VEX prefix 0x%hhx 0x%hhx 0x%hhx",
                 insn->vectorExtensionPrefix[0], insn->vectorExtensionPrefix[1],
                 insn->vectorExtensionPrefix[2]);
     }
   } else if (byte == 0xc5) {
     uint8_t byte1;
 
     if (lookAtByte(insn, &byte1)) {
       dbgprintf(insn, "Couldn't read second byte of VEX");
       return -1;
     }
 
-    if (insn->mode == MODE_64BIT || (byte1 & 0xc0) == 0xc0) {
+    if (insn->mode == MODE_64BIT || (byte1 & 0xc0) == 0xc0)
       insn->vectorExtensionType = TYPE_VEX_2B;
-    } else {
+    else
       unconsumeByte(insn);
-    }
 
     if (insn->vectorExtensionType == TYPE_VEX_2B) {
       insn->vectorExtensionPrefix[0] = byte;
       consumeByte(insn, &insn->vectorExtensionPrefix[1]);
 
-      if (insn->mode == MODE_64BIT) {
+      if (insn->mode == MODE_64BIT)
         insn->rexPrefix = 0x40
                         | (rFromVEX2of2(insn->vectorExtensionPrefix[1]) << 2);
-      }
 
       switch (ppFromVEX2of2(insn->vectorExtensionPrefix[1])) {
       default:
         break;
       case VEX_PREFIX_66:
-        hasOpSize = true;
+        insn->hasOpSize = true;
         break;
       }
 
       dbgprintf(insn, "Found VEX prefix 0x%hhx 0x%hhx",
                 insn->vectorExtensionPrefix[0],
                 insn->vectorExtensionPrefix[1]);
     }
   } else if (byte == 0x8f) {
     uint8_t byte1;
 
     if (lookAtByte(insn, &byte1)) {
       dbgprintf(insn, "Couldn't read second byte of XOP");
       return -1;
     }
 
-    if ((byte1 & 0x38) != 0x0) { /* 0 in these 3 bits is a POP instruction. */
+    if ((byte1 & 0x38) != 0x0) /* 0 in these 3 bits is a POP instruction. */
       insn->vectorExtensionType = TYPE_XOP;
-      insn->necessaryPrefixLocation = insn->readerCursor - 1;
-    } else {
+    else
       unconsumeByte(insn);
-      insn->necessaryPrefixLocation = insn->readerCursor - 1;
-    }
 
     if (insn->vectorExtensionType == TYPE_XOP) {
       insn->vectorExtensionPrefix[0] = byte;
       consumeByte(insn, &insn->vectorExtensionPrefix[1]);
       consumeByte(insn, &insn->vectorExtensionPrefix[2]);
 
       /* We simulate the REX prefix for simplicity's sake */
 
-      if (insn->mode == MODE_64BIT) {
+      if (insn->mode == MODE_64BIT)
         insn->rexPrefix = 0x40
                         | (wFromXOP3of3(insn->vectorExtensionPrefix[2]) << 3)
                         | (rFromXOP2of3(insn->vectorExtensionPrefix[1]) << 2)
                         | (xFromXOP2of3(insn->vectorExtensionPrefix[1]) << 1)
                         | (bFromXOP2of3(insn->vectorExtensionPrefix[1]) << 0);
-      }
 
       switch (ppFromXOP3of3(insn->vectorExtensionPrefix[2])) {
       default:
         break;
       case VEX_PREFIX_66:
-        hasOpSize = true;
+        insn->hasOpSize = true;
         break;
       }
 
       dbgprintf(insn, "Found XOP prefix 0x%hhx 0x%hhx 0x%hhx",
                 insn->vectorExtensionPrefix[0], insn->vectorExtensionPrefix[1],
                 insn->vectorExtensionPrefix[2]);
     }
-  } else {
-    if (insn->mode == MODE_64BIT) {
-      if ((byte & 0xf0) == 0x40) {
-        uint8_t opcodeByte;
-
-        if (lookAtByte(insn, &opcodeByte) || ((opcodeByte & 0xf0) == 0x40)) {
-          dbgprintf(insn, "Redundant REX prefix");
-          return -1;
-        }
-
-        insn->rexPrefix = byte;
-        insn->necessaryPrefixLocation = insn->readerCursor - 2;
-
-        dbgprintf(insn, "Found REX prefix 0x%hhx", byte);
-      } else {
-        unconsumeByte(insn);
-        insn->necessaryPrefixLocation = insn->readerCursor - 1;
-      }
-    } else {
-      unconsumeByte(insn);
-      insn->necessaryPrefixLocation = insn->readerCursor - 1;
-    }
-  }
+  } else if (isREX(insn, byte)) {
+    if (lookAtByte(insn, &nextByte))
+  } else
 
   if (insn->mode == MODE_16BIT) {
-    insn->registerSize       = (hasOpSize ? 4 : 2);
-    insn->addressSize        = (hasAdSize ? 4 : 2);
-    insn->displacementSize   = (hasAdSize ? 4 : 2);
-    insn->immediateSize      = (hasOpSize ? 4 : 2);
+    insn->registerSize = (insn->hasOpSize ? 4 : 2);
+    insn->addressSize = (insn->hasAdSize ? 4 : 2);
+    insn->displacementSize = (insn->hasAdSize ? 4 : 2);
+    insn->immediateSize = (insn->hasOpSize ? 4 : 2);
   } else if (insn->mode == MODE_32BIT) {
-    insn->registerSize       = (hasOpSize ? 2 : 4);
-    insn->addressSize        = (hasAdSize ? 2 : 4);
-    insn->displacementSize   = (hasAdSize ? 2 : 4);
-    insn->immediateSize      = (hasOpSize ? 2 : 4);
+    insn->registerSize = (insn->hasOpSize ? 2 : 4);
+    insn->addressSize = (insn->hasAdSize ? 2 : 4);
+    insn->displacementSize = (insn->hasAdSize ? 2 : 4);
+    insn->immediateSize = (insn->hasOpSize ? 2 : 4);
   } else if (insn->mode == MODE_64BIT) {
     if (insn->rexPrefix && wFromREX(insn->rexPrefix)) {
       insn->registerSize       = 8;
-      insn->addressSize        = (hasAdSize ? 4 : 8);
+      insn->addressSize = (insn->hasAdSize ? 4 : 8);
       insn->displacementSize   = 4;
       insn->immediateSize      = 4;
     } else {
-      insn->registerSize       = (hasOpSize ? 2 : 4);
-      insn->addressSize        = (hasAdSize ? 4 : 8);
-      insn->displacementSize   = (hasOpSize ? 2 : 4);
-      insn->immediateSize      = (hasOpSize ? 2 : 4);
+      insn->registerSize = (insn->hasOpSize ? 2 : 4);
+      insn->addressSize = (insn->hasAdSize ? 4 : 8);
+      insn->displacementSize = (insn->hasOpSize ? 2 : 4);
+      insn->immediateSize = (insn->hasOpSize ? 2 : 4);
     }
   }
 
   return 0;
 }
 
 /*
  * readOpcode - Reads the opcode (excepting the ModR/M byte in the case of
if (current == 0x38) {
         return -1;
 
       insn->opcodeType = THREEBYTE_3A;
     } else {
       dbgprintf(insn, "Didn't find a three-byte escape prefix");
 
       insn->opcodeType = TWOBYTE;
     }
-  }
+  } else if (insn->mandatoryPrefix)
+    // The opcode with mandatory prefix must start with opcode escape.
+    // If not it's legacy repeat prefix
+    insn->mandatoryPrefix = 0;
 
   /*
    * At this point we have consumed the full opcode.
    * Anything we consume from here on must be unconsumed.
    */
 
   insn->opcode = current;
 
if (insn->vectorExtensionType == TYPE_EVEX) {
         break;
       }
 
       if (lFromXOP3of3(insn->vectorExtensionPrefix[2]))
         attrMask |= ATTR_VEXL;
     } else {
       return -1;
     }
-  } else {
-    if (insn->mode != MODE_16BIT && isPrefixAtLocation(insn, 0x66, insn->necessaryPrefixLocation))
+  } else if (!insn->mandatoryPrefix) {
+    // If we don't have mandatory prefix we should use legacy prefixes here
+    if (insn->hasOpSize && (insn->mode != MODE_16BIT))
       attrMask |= ATTR_OPSIZE;
-    else if (isPrefixAtLocation(insn, 0x67, insn->necessaryPrefixLocation))
+    if (insn->hasAdSize)
       attrMask |= ATTR_ADSIZE;
-    else if (isPrefixAtLocation(insn, 0xf3, insn->necessaryPrefixLocation))
+    if (insn->opcodeType == ONEBYTE) {
+      if (insn->repeatPrefix == 0xf3 && (insn->opcode == 0x90))
+        // Special support for PAUSE
         attrMask |= ATTR_XS;
-    else if (isPrefixAtLocation(insn, 0xf2, insn->necessaryPrefixLocation))
+    } else {
+      if (insn->repeatPrefix == 0xf2)
+        attrMask |= ATTR_XD;
+      else if (insn->repeatPrefix == 0xf3)
+        attrMask |= ATTR_XS;
+    }
+  } else {
+    switch (insn->mandatoryPrefix) {
+    case 0xf2:
       attrMask |= ATTR_XD;
+      break;
+    case 0xf3:
+      attrMask |= ATTR_XS;
+      break;
+    case 0x66:
+      if (insn->mode != MODE_16BIT)
+        attrMask |= ATTR_OPSIZE;
+      break;
+    case 0x67:
+      attrMask |= ATTR_ADSIZE;
+      break;
+    }
   }
 
   if (insn->rexPrefix & 0x08)
     attrMask |= ATTR_REXW;
 
   /*
    * JCXZ/JECXZ need special handling for 16-bit mode because the meaning
    * of the AdSize prefix is inverted w.r.t. 32-bit mode.
    */
   if (insn->mode == MODE_16BIT && insn->opcodeType == ONEBYTE &&
       insn->opcode == 0xE3)
     attrMask ^= ATTR_ADSIZE;
 
   /*
    * In 64-bit mode all f64 superscripted opcodes ignore opcode size prefix
    * CALL/JMP/JCC instructions need to ignore 0x66 and consume 4 bytes
    */
 
-  if (insn->mode == MODE_64BIT &&
-      isPrefixAtLocation(insn, 0x66, insn->necessaryPrefixLocation)) {
+  if ((insn->mode == MODE_64BIT) && insn->hasOpSize) {
     switch (insn->opcode) {
     case 0xE8:
     case 0xE9:
       // Take care of psubsb and other mmx instructions.
       if (insn->opcodeType == ONEBYTE) {
         attrMask ^= ATTR_OPSIZE;
         insn->immediateSize = 4;
         insn->displacementSize = 4;
static int getID(struct InternalInstruction* insn, const void *miiArg) {
    * Absolute moves need special handling.
    * -For 16-bit mode because the meaning of the AdSize and OpSize prefixes are
    *  inverted w.r.t.
    * -For 32-bit mode we need to ensure the ADSIZE prefix is observed in
    *  any position.
    */
   if (insn->opcodeType == ONEBYTE && ((insn->opcode & 0xFC) == 0xA0)) {
     /* Make sure we observed the prefixes in any position. */
-    if (insn->prefixPresent[0x67])
+    if (insn->hasAdSize)
       attrMask |= ATTR_ADSIZE;
-    if (insn->prefixPresent[0x66])
+    if (insn->hasOpSize)
       attrMask |= ATTR_OPSIZE;
 
     /* In 16-bit, invert the attributes. */
     if (insn->mode == MODE_16BIT)
       attrMask ^= ATTR_ADSIZE | ATTR_OPSIZE;
 
     if (getIDWithAttrMask(&instructionID, insn, attrMask))
       return -1;
 
     insn->instructionID = instructionID;
     insn->spec = specifierForUID(instructionID);
     return 0;
   }
 
-  if ((insn->mode == MODE_16BIT || insn->prefixPresent[0x66]) &&
+  if ((insn->mode == MODE_16BIT || insn->hasOpSize) &&
       !(attrMask & ATTR_OPSIZE)) {
     /*
      * The instruction tables make no distinction between instructions that
      * allow OpSize anywhere (i.e., 16-bit operations) and that need it in a
      * particular spot (i.e., many MMX operations).  In general we're
      * conservative, but in the specific case where OpSize is present but not
      * in the right place we check if there's a 16-bit operation.
      */
if (getIDWithAttrMask(&instructionIDWithOpsize,
       insn->spec = spec;
       return 0;
     }
 
     specName = GetInstrName(instructionID, miiArg);
     specWithOpSizeName = GetInstrName(instructionIDWithOpsize, miiArg);
 
     if (is16BitEquivalent(specName.data(), specWithOpSizeName.data()) &&
-        (insn->mode == MODE_16BIT) ^ insn->prefixPresent[0x66]) {
+        (insn->mode == MODE_16BIT) ^ insn->hasOpSize) {
       insn->instructionID = instructionIDWithOpsize;
       insn->spec = specifierForUID(instructionIDWithOpsize);
     } else {
       insn->instructionID = instructionID;
       insn->spec = spec;
     }
     return 0;
   }

lib/Target/X86/InstPrinter/X86ATTInstPrinter.cpp

void X86ATTInstPrinter::printInst(const MCInst *MI, raw_ostream &OS,
   const MCInstrDesc &Desc = MII.get(MI->getOpcode());
   uint64_t TSFlags = Desc.TSFlags;
 
   // If verbose assembly is enabled, we can print some informative comments.
   if (CommentStream)
     HasCustomInstComment =
         EmitAnyX86InstComments(MI, *CommentStream, getRegisterName);
 
+  unsigned Flags = MI->getFlags();
   if (TSFlags & X86II::LOCK)
     OS << "\tlock\t";
+  if (!(TSFlags & X86II::LOCK) && Flags & X86::IP_HAS_LOCK)
+    OS << "\tlock\n";
+
+  if (Flags & X86::IP_HAS_REPEAT_NE)
+    OS << "\trepne\n";
+  else if (Flags & X86::IP_HAS_REPEAT)
+    OS << "\trep\n";
 
   // Output CALLpcrel32 as "callq" in 64-bit mode.
   // In Intel annotation it's always emitted as "call".
   //
   // TODO: Probably this hack should be redesigned via InstAlias in
   // InstrInfo.td as soon as Requires clause is supported properly
   // for InstAlias.
   if (MI->getOpcode() == X86::CALLpcrel32 &&

lib/Target/X86/InstPrinter/X86IntelInstPrinter.cpp

void X86IntelInstPrinter::printInst(const MCInst *MI, raw_ostream &OS,
                                     StringRef Annot,
                                     const MCSubtargetInfo &STI) {
   const MCInstrDesc &Desc = MII.get(MI->getOpcode());
   uint64_t TSFlags = Desc.TSFlags;
 
   if (TSFlags & X86II::LOCK)
     OS << "\tlock\n";
 
+  unsigned Flags = MI->getFlags();
+  if (Flags & X86::IP_HAS_REPEAT_NE)
+    OS << "\trepne\n";
+  else if (Flags & X86::IP_HAS_REPEAT)
+    OS << "\trep\n";
+
   printInstruction(MI, OS);
 
   // Next always print the annotation.
   printAnnotation(OS, Annot);
 
   // If verbose assembly is enabled, we can print some informative comments.
   if (CommentStream)
     EmitAnyX86InstComments(MI, *CommentStream, getRegisterName);

lib/Target/X86/MCTargetDesc/X86BaseInfo.h

namespace X86 {
   /// avx512fintrin.h.
   enum STATIC_ROUNDING {
     TO_NEAREST_INT = 0,
     TO_NEG_INF = 1,
     TO_POS_INF = 2,
     TO_ZERO = 3,
     CUR_DIRECTION = 4
   };
+
+  /// The constants to describe instr prefixes if there are
+  enum IPREFIXES {
+    IP_NO_PREFIX = 0,
+    IP_HAS_OP_SIZE = 1,
+    IP_HAS_AD_SIZE = 2,
+    IP_HAS_REPEAT_NE = 4,
+    IP_HAS_REPEAT = 8,
+    IP_HAS_LOCK = 16
+  };
 } // end namespace X86;
 
 /// X86II - This namespace holds all of the target specific flags that
 /// instruction info tracks.
 ///
 namespace X86II {
   /// Target Operand Flag enum.
   enum TOF {

lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

bool X86MCCodeEmitter::emitOpcodePrefix(uint64_t TSFlags, unsigned &CurByte,
                                         raw_ostream &OS) const {
   bool Ret = false;
   // Emit the operand size opcode prefix as needed.
   if ((TSFlags & X86II::OpSizeMask) == (is16BitMode(STI) ? X86II::OpSize32
                                                          : X86II::OpSize16))
     EmitByte(0x66, CurByte, OS);
 
   // Emit the LOCK opcode prefix.
-  if (TSFlags & X86II::LOCK)
+  if (TSFlags & X86II::LOCK || MI.getFlags() & X86::IP_HAS_LOCK)
     EmitByte(0xF0, CurByte, OS);
 
   switch (TSFlags & X86II::OpPrefixMask) {
   case X86II::PD:   // 66
     EmitByte(0x66, CurByte, OS);
     break;
   case X86II::XS:   // F3
     EmitByte(0xF3, CurByte, OS);

 
 void X86MCCodeEmitter::
 encodeInstruction(const MCInst &MI, raw_ostream &OS,
                   SmallVectorImpl<MCFixup> &Fixups,
                   const MCSubtargetInfo &STI) const {
   unsigned Opcode = MI.getOpcode();
   const MCInstrDesc &Desc = MCII.get(Opcode);
   uint64_t TSFlags = Desc.TSFlags;
+  unsigned Flags = MI.getFlags();
 
   // Pseudo instructions don't get encoded.
   if ((TSFlags & X86II::FormMask) == X86II::Pseudo)
     return;
 
   unsigned NumOps = Desc.getNumOperands();
   unsigned CurOp = X86II::getOperandBias(Desc);
 
encodeInstruction(const MCInst &MI, raw_ostream &OS,
   if (MemoryOperand != -1) MemoryOperand += CurOp;
 
   // Emit segment override opcode prefix as needed.
   if (MemoryOperand >= 0)
     EmitSegmentOverridePrefix(CurByte, MemoryOperand+X86::AddrSegmentReg,
                               MI, OS);
 
   // Emit the repeat opcode prefix as needed.
-  if (TSFlags & X86II::REP)
+  if (TSFlags & X86II::REP || Flags & X86::IP_HAS_REPEAT)
     EmitByte(0xF3, CurByte, OS);
+  if (Flags & X86::IP_HAS_REPEAT_NE)
+    EmitByte(0xF2, CurByte, OS);
 
   // Emit the address size opcode prefix as needed.
   bool need_address_override;
   uint64_t AdSize = TSFlags & X86II::AdSizeMask;
   if ((is16BitMode(STI) && AdSize == X86II::AdSize32) ||
       (is32BitMode(STI) && AdSize == X86II::AdSize16) ||
       (is64BitMode(STI) && AdSize == X86II::AdSize32)) {
     need_address_override = true;

test/MC/Disassembler/X86/prefixes.txt

 # RUN: llvm-mc --disassemble %s -triple=x86_64 | FileCheck %s
 
+# CHECK: rep
+# CHECK-NEXT: insb    %dx, %es:(%rdi)
+0xf3 0x6c #rep ins
+# CHECK: rep
+# CHECK-NEXT: insl    %dx, %es:(%rdi)
+0xf3 0x6d #rep ins
+# CHECK: rep
+# CHECK-NEXT: movsb   (%rsi), %es:(%rdi)
+0xf3 0xa4 #rep movs
+# CHECK: rep
+# CHECK-NEXT: movsl   (%rsi), %es:(%rdi)
+0xf3 0xa5 #rep movs
+# CHECK: rep
+# CHECK-NEXT: outsb   (%rsi), %dx
+0xf3 0x6e #rep outs
+# CHECK: rep
+# CHECK-NEXT: outsl   (%rsi), %dx
+0xf3 0x6f #rep outs
+# CHECK: rep
+# CHECK-NEXT: lodsb   (%rsi), %al
+0xf3 0xac #rep lods
+# CHECK: rep
+# CHECK-NEXT: lodsl   (%rsi), %eax
+0xf3 0xad #rep lods
+# CHECK: rep
+# CHECK-NEXT: stosb   %al, %es:(%rdi)
+0xf3 0xaa #rep stos
+# CHECK: rep
+# CHECK-NEXT: stosl   %eax, %es:(%rdi)
+0xf3 0xab #rep stos
+# CHECK: rep
+# CHECK-NEXT: cmpsb   %es:(%rdi), (%rsi)
+0xf3 0xa6 #rep cmps
+# CHECK: rep
+# CHECK-NEXT: cmpsl   %es:(%rdi), (%rsi)
+0xf3 0xa7 #repe cmps
+# CHECK: rep
+# CHECK-NEXT: scasb   %es:(%rdi), %al
+0xf3 0xae #repe scas
+# CHECK: rep
+# CHECK-NEXT: scasl   %es:(%rdi), %eax
+0xf3 0xaf #repe scas
+# CHECK: repne
+# CHECK-NEXT: cmpsb   %es:(%rdi), (%rsi)
+0xf2 0xa6 #repne cmps
+# CHECK: repne
+# CHECK-NEXT: cmpsl   %es:(%rdi), (%rsi)
+0xf2 0xa7 #repne cmps
+# CHECK: repne
+# CHECK-NEXT: scasb   %es:(%rdi), %al
+0xf2 0xae #repne scas
+# CHECK: repne
+# CHECK-NEXT: scasl   %es:(%rdi), %eax
+0xf2 0xaf #repne scas
+
 # CHECK: lock
 # CHECK-NEXT: orl $16, %fs:776
 0xf0 0x64 0x83 0x0c 0x25 0x08 0x03 0x00 0x00 0x10
 
 # CHECK: movq %fs:768, %rdi
 0x64 0x48 0x8b 0x3c 0x25 0x00 0x03 0x00 0x00
 
 # CHECK: rep

 0xf0 0x90
 
 # Test that immediate is printed correctly within opsize prefix
 # CHECK: addw    $-12, %ax
 0x66,0x83,0xc0,0xf4
 
 # Test that multiple redundant prefixes work (redundant, but valid x86).
 # CHECK: rep
-# CHECK-NEXT: rep
 # CHECK-NEXT: stosq
 0xf3 0xf3 0x48 0xab
 
 
 # Test that we can disassembler control registers above CR8
 # CHECK: movq %cr15, %rax
 0x44 0x0f 0x20 0xf8
 # CHECK: movq %dr15, %rax

test/MC/Disassembler/X86/x86-32.txt

 # CHECK: lwpval $2309737967, (%esp), %edx
 0x8f 0xea 0x68 0x12 0x0c 0x24 0xef 0xcd 0xab 0x89
 
 # CHECK: nopl %eax
 0x0f 0x1f 0xc0
 
 # CHECK: nopw %ax
 0x66 0x0f 0x1f 0xc0
+
+# CHECK: movw    %bx, %cs:(%esi,%ebp)
+0x2e 0x66 0x89 0x1c 0x2e
+# CHECK: movl    %ebx, %cs:(%si)
+0x2e 0x67 0x89 0x1c
+# CHECK: movl    %ebx, %cs:(%esi,%ebp)
+0x2e 0x89 0x1c 0x2e
+# CHECK: movw    %bx, %cs:(%si)
+0x2e 0x67 0x66 0x89 0x1c
+# CHECK: movw    %bx, %cs:(%si)
+0x2e 0x66 0x67 0x89 0x1c

test/MC/Disassembler/X86/x86-64.txt

 # CHECK: lwpval $2309737967, (%esp), %edx
 0x67 0x8f 0xea 0x68 0x12 0x0c 0x24 0xef 0xcd 0xab 0x89
 
 # CHECK: nopq -559038737(%rbx,%rcx,8)
 0x48 0x0f 0x1f 0x84 0xcb 0xef 0xbe 0xad 0xde
 
 # CHECK: nopq %rax
 0x48 0x0f 0x1f 0xC0
+
+# CHECK:  xchgw   %di, %ax
+0x66 0x3e 0x97
+
+# CHECK: movw    %bx, %cs:(%rsi,%rbp)
+0x2e 0x66 0x89 0x1c 0x2e
+# CHECK: movl    %ebx, %cs:(%esi,%ebp)
+0x2e 0x67 0x89 0x1c 0x2e
+# CHECK: movl    %ebx, %cs:(%rsi,%rbp)
+0x2e 0x89 0x1c 0x2e
+# CHECK: movw    %bx, %cs:(%esi,%ebp)
+0x2e 0x67 0x66 0x89 0x1c 0x2e
+# CHECK: movw    %bx, %cs:(%esi,%ebp)
+0x2e 0x66 0x67 0x89 0x1c 0x2e
+

test/MC/X86/intel-syntax-encoding.s

// CHECK: encoding: [0x66,0x83,0xf8,0xf4]
   cmp ax, -12
 // CHECK: encoding: [0x83,0xf8,0xf4]
   cmp eax, -12
 // CHECK: encoding: [0x48,0x83,0xf8,0xf4]
   cmp rax, -12
 
   acquire lock add [rax], rax
 // CHECK: encoding: [0xf2]
-// CHECK: encoding: [0xf0]
-// CHECK: encoding: [0x48,0x01,0x00]
+// CHECK: encoding: [0xf0,0x48,0x01,0x00]
   release lock add [rax], rax
 // CHECK: encoding: [0xf3]
-// CHECK: encoding: [0xf0]
-// CHECK: encoding: [0x48,0x01,0x00]
+// CHECK: encoding: [0xf0,0x48,0x01,0x00]
 
 // CHECK: encoding: [0x9c]
 // CHECK: encoding: [0x9d]
 pushf
 popf
 
 LBB0_3:
 // CHECK: encoding: [0xeb,A]

test/MC/X86/x86-64.s

 // rdar://8741045
 lock/incl 1(%rsp)
 // CHECK: lock
 // CHECK: incl 1(%rsp)
 
 
 lock addq %rsi, (%rdi)
 // CHECK: lock
-// CHECK: encoding: [0xf0]
 // CHECK: addq %rsi, (%rdi)
-// CHECK: encoding: [0x48,0x01,0x37]
+// CHECK: encoding: [0xf0,0x48,0x01,0x37]
 
 lock subq %rsi, (%rdi)
 // CHECK: lock
-// CHECK: encoding: [0xf0]
 // CHECK: subq %rsi, (%rdi)
-// CHECK: encoding: [0x48,0x29,0x37]
+// CHECK: encoding: [0xf0,0x48,0x29,0x37]
 
 lock andq %rsi, (%rdi)
 // CHECK: lock
-// CHECK: encoding: [0xf0]
 // CHECK: andq %rsi, (%rdi)
-// CHECK: encoding: [0x48,0x21,0x37]
+// CHECK: encoding: [0xf0,0x48,0x21,0x37]
 
 lock orq %rsi, (%rdi)
 // CHECK: lock
-// CHECK: encoding: [0xf0]
 // CHECK: orq %rsi, (%rdi)
-// CHECK: encoding: [0x48,0x09,0x37]
+// CHECK: encoding: [0xf0,0x48,0x09,0x37]
 
 lock xorq %rsi, (%rdi)
 // CHECK: lock
-// CHECK: encoding: [0xf0]
 // CHECK: xorq %rsi, (%rdi)
-// CHECK: encoding: [0x48,0x31,0x37]
+// CHECK: encoding: [0xf0,0x48,0x31,0x37]
 
 xacquire lock addq %rax, (%rax)
 // CHECK: xacquire
 // CHECK: encoding: [0xf2]
 // CHECK: lock
-// CHECK: encoding: [0xf0]
 // CHECK: addq %rax, (%rax)
-// CHECK: encoding: [0x48,0x01,0x00]
+// CHECK: encoding: [0xf0,0x48,0x01,0x00]
 
 xrelease lock addq %rax, (%rax)
 // CHECK: xrelease
 // CHECK: encoding: [0xf3]
 // CHECK: lock
-// CHECK: encoding: [0xf0]
 // CHECK: addq %rax, (%rax)
-// CHECK: encoding: [0x48,0x01,0x00]
+// CHECK: encoding: [0xf0,0x48,0x01,0x00]
 
 // rdar://8033482
 rep movsl
 // CHECK: rep
-// CHECK: encoding: [0xf3]
 // CHECK: movsl
-// CHECK: encoding: [0xa5]
+// CHECK: encoding: [0xf3,0xa5]
 
 
 // rdar://8403974
 iret
 // CHECK: iretl
 // CHECK: encoding: [0xcf]
 iretw
 // CHECK: iretw