[X86] Preserve redundant Address-Size override prefix

[Amir]

Summary

Print and emit redundant Address-Size override prefix if it's set on the
instruction.

[skan]

I think this patch is totally wrong. We need address size override prefix only when the instruction has a memory operand.The address-size override prefix (67H) allows programs to switch between 16- and 32-bit addressing. Either size can be the default; the prefix selects the non-default size. Using this prefix and/or other undefined opcodes when operands for the instruction do not reside in memory is reserved; such use may cause unpredictable behavior. Whether a instruction need ASZ should be included in the definition of the instruction in tablgen.

[craig.topper]

In D120592#3348216, @skan wrote:

I think this patch is totally wrong. We need address size override prefix only when the instruction has a memory operand.The address-size override prefix (67H) allows programs to switch between 16- and 32-bit addressing. Either size can be the default; the prefix selects the non-default size. Using this prefix and/or other undefined opcodes when operands for the instruction do not reside in memory is reserved; such use may cause unpredictable behavior. Whether a instruction need ASZ should be included in the definition of the instruction in tablgen.

I believe example test case is supported by gas and callq does use address size. What syntax should be used instead?

[skan]

In D120592#3348246, @craig.topper wrote:

In D120592#3348216, @skan wrote:

I think this patch is totally wrong. We need address size override prefix only when the instruction has a memory operand.The address-size override prefix (67H) allows programs to switch between 16- and 32-bit addressing. Either size can be the default; the prefix selects the non-default size. Using this prefix and/or other undefined opcodes when operands for the instruction do not reside in memory is reserved; such use may cause unpredictable behavior. Whether a instruction need ASZ should be included in the definition of the instruction in tablgen.

I believe example test case is supported by gas and callq does use address size. What syntax should be used instead?

This test case is misleading. LLVM-MC can pass the test case w/o this patch b/c an expicit prefix is added. However, this callq does not need a ASZ at all.

[craig.topper]

In D120592#3348249, @skan wrote:

In D120592#3348246, @craig.topper wrote:

In D120592#3348216, @skan wrote:

I think this patch is totally wrong. We need address size override prefix only when the instruction has a memory operand.The address-size override prefix (67H) allows programs to switch between 16- and 32-bit addressing. Either size can be the default; the prefix selects the non-default size. Using this prefix and/or other undefined opcodes when operands for the instruction do not reside in memory is reserved; such use may cause unpredictable behavior. Whether a instruction need ASZ should be included in the definition of the instruction in tablgen.

I believe example test case is supported by gas and callq does use address size. What syntax should be used instead?

This test case is misleading. LLVM-MC can pass the test case w/o this patch b/c an expicit prefix is added. However, this callq does not need a ASZ at all.

There is a legimate use for a redundant addr32 on a call instruction. See this code in lld

// Convert call/jmp instructions.
if (modRm == 0x15) {
  // ABI says we can convert "call *foo@GOTPCREL(%rip)" to "nop; call foo".
  // Instead we convert to "addr32 call foo" where addr32 is an instruction
  // prefix. That makes result expression to be a single instruction.
  loc[-2] = 0x67; // addr32 prefix
  loc[-1] = 0xe8; // call
  write32le(loc, val);
  return;
}

[skan]

In D120592#3348274, @craig.topper wrote:

In D120592#3348249, @skan wrote:

In D120592#3348246, @craig.topper wrote:

In D120592#3348216, @skan wrote:

I think this patch is totally wrong. We need address size override prefix only when the instruction has a memory operand.The address-size override prefix (67H) allows programs to switch between 16- and 32-bit addressing. Either size can be the default; the prefix selects the non-default size. Using this prefix and/or other undefined opcodes when operands for the instruction do not reside in memory is reserved; such use may cause unpredictable behavior. Whether a instruction need ASZ should be included in the definition of the instruction in tablgen.

I believe example test case is supported by gas and callq does use address size. What syntax should be used instead?

This test case is misleading. LLVM-MC can pass the test case w/o this patch b/c an expicit prefix is added. However, this callq does not need a ASZ at all.

There is a legimate use for a redundant addr32 on a call instruction. See this code in lld

// Convert call/jmp instructions.
if (modRm == 0x15) {
  // ABI says we can convert "call *foo@GOTPCREL(%rip)" to "nop; call foo".
  // Instead we convert to "addr32 call foo" where addr32 is an instruction
  // prefix. That makes result expression to be a single instruction.
  loc[-2] = 0x67; // addr32 prefix
  loc[-1] = 0xe8; // call
  write32le(loc, val);
  return;
}

Thanks craig, I agree that we can add a redundant addr32 prefix on callq.

In addition, and GNU objdump also prints the prefix even if it is illegal, which can help developer find their bug.

cat 1.s

addr32 ret

as 1.s -o 1.o && objdump -d 1.o

0000000000000000 <.text>:
   0:   67 c3                   addr32 retq

This change looks good to me. But the test case should be moved into MC and covers ATT/Intel assemble syntax.

[skan]

bolt/test/X86/addr32.s

3–21 ↗

(On Diff #411531)

[skan]

bolt/test/X86/addr32.s

1 ↗

(On Diff #411531)

This test should be moved to llvm/test/MC/Disassembler/X86/

[Amir]

@skan, @craig.topper
Thank you for the review! I didn't communicate the intent clearly. There's a BOLT bug report (https://github.com/llvm/llvm-project/issues/54066#issuecomment-1050327548) in which BOLT removes addr32 prefix from call imm. I've root caused it to instruction printer and emitter ignoring the explicit (redundant) prefix. The code is produced by g++ (GCC) 9.2.1 20200225, with -O3 -g -std=gnu++17 -Wl,-Bsymbolic-functions -Wl,--dynamic-list-cpp-new. The linker wasn't mentioned.

[skan]

In D120592#3349375, @Amir wrote:

@skan, @craig.topper
Thank you for the review! I didn't communicate the intent clearly. There's a BOLT bug report (https://github.com/llvm/llvm-project/issues/54066#issuecomment-1050327548) in which BOLT removes addr32 prefix from call imm. I've root caused it to instruction printer and emitter ignoring the explicit (redundant) prefix. The code is produced by g++ (GCC) 9.2.1 20200225, with -O3 -g -std=gnu++17 -Wl,-Bsymbolic-functions -Wl,--dynamic-list-cpp-new. The linker wasn't mentioned.

I didn't apply the patch and rebuild the compiler. But I think the bug can be reproduced w/o BOLT, right?

[Amir]

Fix tests

[Amir]

In D120592#3350106, @skan wrote:

In D120592#3349375, @Amir wrote:

@skan, @craig.topper
Thank you for the review! I didn't communicate the intent clearly. There's a BOLT bug report (https://github.com/llvm/llvm-project/issues/54066#issuecomment-1050327548) in which BOLT removes addr32 prefix from call imm. I've root caused it to instruction printer and emitter ignoring the explicit (redundant) prefix. The code is produced by g++ (GCC) 9.2.1 20200225, with -O3 -g -std=gnu++17 -Wl,-Bsymbolic-functions -Wl,--dynamic-list-cpp-new. The linker wasn't mentioned.

I didn't apply the patch and rebuild the compiler. But I think the bug can be reproduced w/o BOLT, right?

Yes, it reproduces with llvm-mc disasm-asm roundtrip (the prefix is lost).

[skan]

llvm/test/MC/X86/align-branch-hardcode.s

20 ↗

(On Diff #411935)

Why do we got addr32 here w/ hardcode 0x66?

[Amir]

llvm/test/MC/X86/align-branch-hardcode.s

20 ↗

(On Diff #411935)

Looking into it

[Amir]

Rebase

[Amir]

llvm/test/MC/X86/align-branch-hardcode.s

20 ↗

(On Diff #411935)

@skan
The call has 0x67 prefix regardless of .byte 0x66, perhaps due to using ecx register as a memop.
With this diff we start printing the addr32 prefix that wasn't printed before.

[skan]

llvm/test/MC/X86/code16-32-64.s

18–20 ↗

(On Diff #411938)

addr32 should not be printed here b/c 0x67 is not a redundant prefix here. You can check the behaviour of GNU objdump.

[Amir]

Only print address size override prefix in disassembly when it's redundant

[Amir]

@skan
I moved NeedAddressOverride check out of X86MCCodeEmitter.cpp (along with isXXBitMemOperand) into X86MCTargetDesc in order to use it in printInstFlags to test if addr32 is a redundant prefix.

[skan]

In D120592#3353538, @Amir wrote:

@skan
I moved NeedAddressOverride check out of X86MCCodeEmitter.cpp (along with isXXBitMemOperand) into X86MCTargetDesc in order to use it in printInstFlags to test if addr32 is a redundant prefix.

I belive it's the correct direction. However there is still a bug. (see my inline comments)

llvm/test/MC/Disassembler/X86/prefixes.txt

54 ↗

(On Diff #412323)

0x67 is not redundant here. It changes (%rdi) to (%edi). You can not encode
rep addr32 stosq %rax, %es:(%edi) w/ llvm-mc --show-encoding

llvm/test/MC/X86/addr16-32.s

20–23 ↗

(On Diff #412323)

0x67 is not redundant here.

[Amir]

Moved RawFrmSrc, RawFrmDstSrc, RawFrmDst handling from X86MCCodeEmitter::emitPrefixImpl
to X86_MC::needsAddressSizeOverride. This consolidates all checks controlling
the emission of AddressSize override prefix in needsAddressSizeOverride.

The flipside is a prefix order change but it should be benign.

[Amir]

Code cleanup

[craig.topper]

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp

75

Missing namespace? Or missing static?

llvm/test/MC/X86/index-operations.s

74

Was this a bug before? REX prefix is supposed to be the byte before the opcode.

[Amir]

llvm/test/MC/X86/index-operations.s

74

You're right. It was a bug before, but it's still there:
REX prefix emission happened before emitSegmentOverridePrefix and 0x67 prefix, this diff moved 0x67 prefix emission above.
See the line
HasREX = emitOpcodePrefix(MemoryOperand, MI, STI, OS);

[Amir]

static isMemOperand

[skan]

LGTM.

[Amir]

llvm/test/MC/X86/index-operations.s

74

Addressed this issue in D120871 (follow-up to this diff)

llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp

 
 void X86ATTInstPrinter::printInst(const MCInst *MI, uint64_t Address,
                                   StringRef Annot, const MCSubtargetInfo &STI,
                                   raw_ostream &OS) {
   // If verbose assembly is enabled, we can print some informative comments.
   if (CommentStream)
     HasCustomInstComment = EmitAnyX86InstComments(MI, *CommentStream, MII);
 
-  printInstFlags(MI, OS);
+  printInstFlags(MI, OS, STI);
 
   // 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 &&

llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.h

public:
   void printVPCOMMnemonic(const MCInst *MI, raw_ostream &OS);
   void printVPCMPMnemonic(const MCInst *MI, raw_ostream &OS);
   void printCMPMnemonic(const MCInst *MI, bool IsVCmp, raw_ostream &OS);
   void printRoundingControl(const MCInst *MI, unsigned Op, raw_ostream &O);
   void printPCRelImm(const MCInst *MI, uint64_t Address, unsigned OpNo,
                      raw_ostream &O);
 
 protected:
-  void printInstFlags(const MCInst *MI, raw_ostream &O);
+  void printInstFlags(const MCInst *MI, raw_ostream &O,
+                      const MCSubtargetInfo &STI);
   void printOptionalSegReg(const MCInst *MI, unsigned OpNo, raw_ostream &O);
   void printVKPair(const MCInst *MI, unsigned OpNo, raw_ostream &OS);
 };
 
 } // end namespace llvm
 
 #endif // LLVM_LIB_TARGET_X86_MCTARGETDESC_X86INSTPRINTERCOMMON_H

llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp

 
 #include "X86InstPrinterCommon.h"
 #include "X86BaseInfo.h"
 #include "llvm/MC/MCAsmInfo.h"
 #include "llvm/MC/MCExpr.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstrDesc.h"
 #include "llvm/MC/MCInstrInfo.h"
-#include "llvm/Support/raw_ostream.h"
+#include "llvm/MC/MCSubtargetInfo.h"
 #include "llvm/Support/Casting.h"
-#include <cstdint>
+#include "llvm/Support/raw_ostream.h"
 #include <cassert>
+#include <cstdint>
 
 using namespace llvm;
 
 void X86InstPrinterCommon::printCondCode(const MCInst *MI, unsigned Op,
                                          raw_ostream &O) {
   int64_t Imm = MI->getOperand(Op).getImm();
   switch (Imm) {
   default: llvm_unreachable("Invalid condcode argument!");

 void X86InstPrinterCommon::printOptionalSegReg(const MCInst *MI, unsigned OpNo,
                                                raw_ostream &O) {
   if (MI->getOperand(OpNo).getReg()) {
     printOperand(MI, OpNo, O);
     O << ':';
   }
 }
 
-void X86InstPrinterCommon::printInstFlags(const MCInst *MI, raw_ostream &O) {
+void X86InstPrinterCommon::printInstFlags(const MCInst *MI, raw_ostream &O,
+                                          const MCSubtargetInfo &STI) {
   const MCInstrDesc &Desc = MII.get(MI->getOpcode());
   uint64_t TSFlags = Desc.TSFlags;
   unsigned Flags = MI->getFlags();
 
   if ((TSFlags & X86II::LOCK) || (Flags & X86::IP_HAS_LOCK))
     O << "\tlock\t";
 
   if ((TSFlags & X86II::NOTRACK) || (Flags & X86::IP_HAS_NOTRACK))
else if (Flags & X86::IP_USE_VEX3)
     O << "\t{vex3}";
   else if (Flags & X86::IP_USE_EVEX)
     O << "\t{evex}";
 
   if (Flags & X86::IP_USE_DISP8)
     O << "\t{disp8}";
   else if (Flags & X86::IP_USE_DISP32)
     O << "\t{disp32}";
+
+  // Determine where the memory operand starts, if present
+  int MemoryOperand = X86II::getMemoryOperandNo(TSFlags);
+  if (MemoryOperand != -1)
+    MemoryOperand += X86II::getOperandBias(Desc);
+
+  // Address-Size override prefix
+  if (Flags & X86::IP_HAS_AD_SIZE &&
+      !X86_MC::needsAddressSizeOverride(*MI, STI, MemoryOperand, TSFlags)) {
+    if (STI.hasFeature(X86::Mode16Bit) || STI.hasFeature(X86::Mode64Bit))
+      O << "\taddr32\t";
+    else if (STI.hasFeature(X86::Mode32Bit))
+      O << "\taddr16\t";
+  }
 }
 
 void X86InstPrinterCommon::printVKPair(const MCInst *MI, unsigned OpNo,
                                        raw_ostream &OS) {
   // In assembly listings, a pair is represented by one of its members, any
   // of the two.  Here, we pick k0, k2, k4, k6, but we could as well
   // print K2_K3 as "k3".  It would probably make a lot more sense, if
   // the assembly would look something like:

llvm/lib/Target/X86/MCTargetDesc/X86IntelInstPrinter.cpp

 
 void X86IntelInstPrinter::printRegName(raw_ostream &OS, unsigned RegNo) const {
   OS << getRegisterName(RegNo);
 }
 
 void X86IntelInstPrinter::printInst(const MCInst *MI, uint64_t Address,
                                     StringRef Annot, const MCSubtargetInfo &STI,
                                     raw_ostream &OS) {
-  printInstFlags(MI, OS);
+  printInstFlags(MI, OS, STI);
 
   // In 16-bit mode, print data16 as data32.
   if (MI->getOpcode() == X86::DATA16_PREFIX &&
       STI.getFeatureBits()[X86::Mode16Bit]) {
     OS << "\tdata32";
   } else if (!printAliasInstr(MI, Address, OS) && !printVecCompareInstr(MI, OS))
     printInstruction(MI, Address, OS);
 

llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp

default:
       llvm_unreachable("Unsupported signed fixup size!");
     case 4:
       return MCFixupKind(X86::reloc_signed_4byte);
     }
   }
   return MCFixup::getKindForSize(Size, isPCRel);
 }
 
-/// \param Op operand # of the memory operand.
-///
-/// \returns true if the specified instruction has a 16-bit memory operand.
-static bool is16BitMemOperand(const MCInst &MI, unsigned Op,
-                              const MCSubtargetInfo &STI) {
-  const MCOperand &Base = MI.getOperand(Op + X86::AddrBaseReg);
-  const MCOperand &Index = MI.getOperand(Op + X86::AddrIndexReg);
-
-  unsigned BaseReg = Base.getReg();
-  unsigned IndexReg = Index.getReg();
-
-  if (STI.hasFeature(X86::Mode16Bit) && BaseReg == 0 && IndexReg == 0)
-    return true;
-  if ((BaseReg != 0 &&
-       X86MCRegisterClasses[X86::GR16RegClassID].contains(BaseReg)) ||
-      (IndexReg != 0 &&
-       X86MCRegisterClasses[X86::GR16RegClassID].contains(IndexReg)))
-    return true;
-  return false;
-}
-
-/// \param Op operand # of the memory operand.
-///
-/// \returns true if the specified instruction has a 32-bit memory operand.
-static bool is32BitMemOperand(const MCInst &MI, unsigned Op) {
-  const MCOperand &BaseReg = MI.getOperand(Op + X86::AddrBaseReg);
-  const MCOperand &IndexReg = MI.getOperand(Op + X86::AddrIndexReg);
-
-  if ((BaseReg.getReg() != 0 &&
-       X86MCRegisterClasses[X86::GR32RegClassID].contains(BaseReg.getReg())) ||
-      (IndexReg.getReg() != 0 &&
-       X86MCRegisterClasses[X86::GR32RegClassID].contains(IndexReg.getReg())))
-    return true;
-  if (BaseReg.getReg() == X86::EIP) {
-    assert(IndexReg.getReg() == 0 && "Invalid eip-based address.");
-    return true;
-  }
-  if (IndexReg.getReg() == X86::EIZ)
-    return true;
-  return false;
-}
-
-/// \param Op operand # of the memory operand.
-///
-/// \returns true if the specified instruction has a 64-bit memory operand.
-#ifndef NDEBUG
-static bool is64BitMemOperand(const MCInst &MI, unsigned Op) {
-  const MCOperand &BaseReg = MI.getOperand(Op + X86::AddrBaseReg);
-  const MCOperand &IndexReg = MI.getOperand(Op + X86::AddrIndexReg);
-
-  if ((BaseReg.getReg() != 0 &&
-       X86MCRegisterClasses[X86::GR64RegClassID].contains(BaseReg.getReg())) ||
-      (IndexReg.getReg() != 0 &&
-       X86MCRegisterClasses[X86::GR64RegClassID].contains(IndexReg.getReg())))
-    return true;
-  return false;
-}
-#endif
-
 enum GlobalOffsetTableExprKind { GOT_None, GOT_Normal, GOT_SymDiff };
 
 /// Check if this expression starts with  _GLOBAL_OFFSET_TABLE_ and if it is
 /// of the form _GLOBAL_OFFSET_TABLE_-symbol. This is needed to support PIC on
 /// ELF i386 as _GLOBAL_OFFSET_TABLE_ is magical. We check only simple case that
 /// are know to be used: _GLOBAL_OFFSET_TABLE_ by itself or at the start of a
 /// binary expression.
 static GlobalOffsetTableExprKind
emitImmediate(Disp, MI.getLoc(), 4, MCFixupKind(FixupKind), StartByte, OS,
                   Fixups, -ImmSize);
     return;
   }
 
   unsigned BaseRegNo = BaseReg ? getX86RegNum(Base) : -1U;
 
   // 16-bit addressing forms of the ModR/M byte have a different encoding for
   // the R/M field and are far more limited in which registers can be used.
-  if (is16BitMemOperand(MI, Op, STI)) {
+  if (X86_MC::is16BitMemOperand(MI, Op, STI)) {
     if (BaseReg) {
       // For 32-bit addressing, the row and column values in Table 2-2 are
       // basically the same. It's AX/CX/DX/BX/SP/BP/SI/DI in that order, with
       // some special cases. And getX86RegNum reflects that numbering.
       // For 16-bit addressing it's more fun, as shown in the SDM Vol 2A,
       // Table 2-1 "16-Bit Addressing Forms with the ModR/M byte". We can only
       // use SI/DI/BP/BX, which have "row" values 4-7 in no particular order,
       // while values 0-3 indicate the allowed combinations (base+index) of
bool X86MCCodeEmitter::emitPrefixImpl(unsigned &CurOp, const MCInst &MI,
   // Emit the repeat opcode prefix as needed.
   unsigned Flags = MI.getFlags();
   if (TSFlags & X86II::REP || Flags & X86::IP_HAS_REPEAT)
     emitByte(0xF3, OS);
   if (Flags & X86::IP_HAS_REPEAT_NE)
     emitByte(0xF2, OS);
 
   // Emit the address size opcode prefix as needed.
-  bool NeedAddressOverride;
-  uint64_t AdSize = TSFlags & X86II::AdSizeMask;
-  if ((STI.hasFeature(X86::Mode16Bit) && AdSize == X86II::AdSize32) ||
-      (STI.hasFeature(X86::Mode32Bit) && AdSize == X86II::AdSize16) ||
-      (STI.hasFeature(X86::Mode64Bit) && AdSize == X86II::AdSize32)) {
-    NeedAddressOverride = true;
-  } else if (MemoryOperand < 0) {
-    NeedAddressOverride = false;
-  } else if (STI.hasFeature(X86::Mode64Bit)) {
-    assert(!is16BitMemOperand(MI, MemoryOperand, STI));
-    NeedAddressOverride = is32BitMemOperand(MI, MemoryOperand);
-  } else if (STI.hasFeature(X86::Mode32Bit)) {
-    assert(!is64BitMemOperand(MI, MemoryOperand));
-    NeedAddressOverride = is16BitMemOperand(MI, MemoryOperand, STI);
-  } else {
-    assert(STI.hasFeature(X86::Mode16Bit));
-    assert(!is64BitMemOperand(MI, MemoryOperand));
-    NeedAddressOverride = !is16BitMemOperand(MI, MemoryOperand, STI);
-  }
-
-  if (NeedAddressOverride)
+  if (X86_MC::needsAddressSizeOverride(MI, STI, MemoryOperand, TSFlags) ||
+      Flags & X86::IP_HAS_AD_SIZE)
     emitByte(0x67, OS);
 
   // Encoding type for this instruction.
   uint64_t Encoding = TSFlags & X86II::EncodingMask;
   bool HasREX = false;
   if (Encoding)
     emitVEXOpcodePrefix(MemoryOperand, MI, OS);
   else
     HasREX = emitOpcodePrefix(MemoryOperand, MI, STI, OS);
 
   uint64_t Form = TSFlags & X86II::FormMask;
   switch (Form) {
   default:
     break;
   case X86II::RawFrmDstSrc: {
-    unsigned siReg = MI.getOperand(1).getReg();
-    assert(((siReg == X86::SI && MI.getOperand(0).getReg() == X86::DI) ||
-            (siReg == X86::ESI && MI.getOperand(0).getReg() == X86::EDI) ||
-            (siReg == X86::RSI && MI.getOperand(0).getReg() == X86::RDI)) &&
-           "SI and DI register sizes do not match");
     // Emit segment override opcode prefix as needed (not for %ds).
     if (MI.getOperand(2).getReg() != X86::DS)
       emitSegmentOverridePrefix(2, MI, OS);
-    // Emit AdSize prefix as needed.
-    if ((!STI.hasFeature(X86::Mode32Bit) && siReg == X86::ESI) ||
-        (STI.hasFeature(X86::Mode32Bit) && siReg == X86::SI))
-      emitByte(0x67, OS);
     CurOp += 3; // Consume operands.
     break;
   }
   case X86II::RawFrmSrc: {
-    unsigned siReg = MI.getOperand(0).getReg();
     // Emit segment override opcode prefix as needed (not for %ds).
     if (MI.getOperand(1).getReg() != X86::DS)
       emitSegmentOverridePrefix(1, MI, OS);
-    // Emit AdSize prefix as needed.
-    if ((!STI.hasFeature(X86::Mode32Bit) && siReg == X86::ESI) ||
-        (STI.hasFeature(X86::Mode32Bit) && siReg == X86::SI))
-      emitByte(0x67, OS);
     CurOp += 2; // Consume operands.
     break;
   }
   case X86II::RawFrmDst: {
-    unsigned siReg = MI.getOperand(0).getReg();
-    // Emit AdSize prefix as needed.
-    if ((!STI.hasFeature(X86::Mode32Bit) && siReg == X86::EDI) ||
-        (STI.hasFeature(X86::Mode32Bit) && siReg == X86::DI))
-      emitByte(0x67, OS);
     ++CurOp; // Consume operand.
     break;
   }
   case X86II::RawFrmMemOffs: {
     // Emit segment override opcode prefix as needed.
     emitSegmentOverridePrefix(1, MI, OS);
     break;
   }

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h

 unsigned getDwarfRegFlavour(const Triple &TT, bool isEH);
 
 void initLLVMToSEHAndCVRegMapping(MCRegisterInfo *MRI);
 
 
 /// Returns true if this instruction has a LOCK prefix.
 bool hasLockPrefix(const MCInst &MI);
 
+/// \param Op operand # of the memory operand.
+///
+/// \returns true if the specified instruction has a 16-bit memory operand.
+bool is16BitMemOperand(const MCInst &MI, unsigned Op,
+                       const MCSubtargetInfo &STI);
+
+/// \param Op operand # of the memory operand.
+///
+/// \returns true if the specified instruction has a 32-bit memory operand.
+bool is32BitMemOperand(const MCInst &MI, unsigned Op);
+
+/// \param Op operand # of the memory operand.
+///
+/// \returns true if the specified instruction has a 64-bit memory operand.
+#ifndef NDEBUG
+bool is64BitMemOperand(const MCInst &MI, unsigned Op);
+#endif
+
+/// Returns true if this instruction needs an Address-Size override prefix.
+bool needsAddressSizeOverride(const MCInst &MI, const MCSubtargetInfo &STI,
+                              int MemoryOperand, uint64_t TSFlags);
+
 /// Create a X86 MCSubtargetInfo instance. This is exposed so Asm parser, etc.
 /// do not need to go through TargetRegistry.
 MCSubtargetInfo *createX86MCSubtargetInfo(const Triple &TT, StringRef CPU,
                                           StringRef FS);
 }
 
 MCCodeEmitter *createX86MCCodeEmitter(const MCInstrInfo &MCII,
                                       MCContext &Ctx);

llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp

if (TT.isOSCygMing())
     return DWARFFlavour::X86_32_Generic;
   return DWARFFlavour::X86_32_Generic;
 }
 
 bool X86_MC::hasLockPrefix(const MCInst &MI) {
   return MI.getFlags() & X86::IP_HAS_LOCK;
 }
 
+bool isMemOperand(const MCInst &MI, unsigned Op, unsigned RegClassID) {
+  const MCOperand &Base = MI.getOperand(Op + X86::AddrBaseReg);
+  const MCOperand &Index = MI.getOperand(Op + X86::AddrIndexReg);
+  const MCRegisterClass &RC = X86MCRegisterClasses[RegClassID];
+
+  return (Base.isReg() && Base.getReg() != 0 && RC.contains(Base.getReg())) ||
+         (Index.isReg() && Index.getReg() != 0 && RC.contains(Index.getReg()));
+}
+
+bool X86_MC::is16BitMemOperand(const MCInst &MI, unsigned Op,
+                               const MCSubtargetInfo &STI) {
+  const MCOperand &Base = MI.getOperand(Op + X86::AddrBaseReg);
+  const MCOperand &Index = MI.getOperand(Op + X86::AddrIndexReg);
+
+  if (STI.hasFeature(X86::Mode16Bit) && Base.isReg() && Base.getReg() == 0 &&
+      Index.isReg() && Index.getReg() == 0)
+    return true;
+  return isMemOperand(MI, Op, X86::GR16RegClassID);
+}
+
+bool X86_MC::is32BitMemOperand(const MCInst &MI, unsigned Op) {
+  const MCOperand &Base = MI.getOperand(Op + X86::AddrBaseReg);
+  const MCOperand &Index = MI.getOperand(Op + X86::AddrIndexReg);
+  if (Base.isReg() && Base.getReg() == X86::EIP) {
+    assert(Index.isReg() && Index.getReg() == 0 && "Invalid eip-based address");
+    return true;
+  }
+  if (Index.isReg() && Index.getReg() == X86::EIZ)
+    return true;
+  return isMemOperand(MI, Op, X86::GR32RegClassID);
+}
+
+#ifndef NDEBUG
+bool X86_MC::is64BitMemOperand(const MCInst &MI, unsigned Op) {
+  return isMemOperand(MI, Op, X86::GR64RegClassID);
+}
+#endif
+
+bool X86_MC::needsAddressSizeOverride(const MCInst &MI,
+                                      const MCSubtargetInfo &STI,
+                                      int MemoryOperand, uint64_t TSFlags) {
+  uint64_t AdSize = TSFlags & X86II::AdSizeMask;
+  bool Is16BitMode = STI.hasFeature(X86::Mode16Bit);
+  bool Is32BitMode = STI.hasFeature(X86::Mode32Bit);
+  bool Is64BitMode = STI.hasFeature(X86::Mode64Bit);
+  if ((Is16BitMode && AdSize == X86II::AdSize32) ||
+      (Is32BitMode && AdSize == X86II::AdSize16) ||
+      (Is64BitMode && AdSize == X86II::AdSize32))
+    return true;
+  uint64_t Form = TSFlags & X86II::FormMask;
+  switch (Form) {
+  default:
+    break;
+  case X86II::RawFrmDstSrc: {
+    unsigned siReg = MI.getOperand(1).getReg();
+    assert(((siReg == X86::SI && MI.getOperand(0).getReg() == X86::DI) ||
+            (siReg == X86::ESI && MI.getOperand(0).getReg() == X86::EDI) ||
+            (siReg == X86::RSI && MI.getOperand(0).getReg() == X86::RDI)) &&
+           "SI and DI register sizes do not match");
+    return (!Is32BitMode && siReg == X86::ESI) ||
+           (Is32BitMode && siReg == X86::SI);
+  }
+  case X86II::RawFrmSrc: {
+    unsigned siReg = MI.getOperand(0).getReg();
+    return (!Is32BitMode && siReg == X86::ESI) ||
+           (Is32BitMode && siReg == X86::SI);
+  }
+  case X86II::RawFrmDst: {
+    unsigned siReg = MI.getOperand(0).getReg();
+    return (!Is32BitMode && siReg == X86::EDI) ||
+           (Is32BitMode && siReg == X86::DI);
+  }
+  }
+
+  // Determine where the memory operand starts, if present.
+  if (MemoryOperand < 0)
+    return false;
+
+  if (STI.hasFeature(X86::Mode64Bit)) {
+    assert(!is16BitMemOperand(MI, MemoryOperand, STI));
+    return is32BitMemOperand(MI, MemoryOperand);
+  }
+  if (STI.hasFeature(X86::Mode32Bit)) {
+    assert(!is64BitMemOperand(MI, MemoryOperand));
+    return is16BitMemOperand(MI, MemoryOperand, STI);
+  }
+  assert(STI.hasFeature(X86::Mode16Bit));
+  assert(!is64BitMemOperand(MI, MemoryOperand));
+  return !is16BitMemOperand(MI, MemoryOperand, STI);
+}
+
 void X86_MC::initLLVMToSEHAndCVRegMapping(MCRegisterInfo *MRI) {
   // FIXME: TableGen these.
   for (unsigned Reg = X86::NoRegister + 1; Reg < X86::NUM_TARGET_REGS; ++Reg) {
     unsigned SEH = MRI->getEncodingValue(Reg);
     MRI->mapLLVMRegToSEHReg(Reg, SEH);
   }
 
   // Mapping from CodeView to MC register id.

llvm/test/MC/Disassembler/X86/addr32.s

+# Check that we don't accidentally strip addr32 prefix
+
+# RUN: llvm-mc -disassemble %s -triple=x86_64 | FileCheck %s
+# CHECK: addr32 callq
+0x67 0xe8 0x00 0x00 0x00 0x00

llvm/test/MC/X86/code16gcc.s

 // RUN: llvm-mc -triple i386-unknown-unknown-unknown --show-encoding %s | FileCheck %s 
 
   .code16gcc
   //CHECK:  .code16
   nop
   //CHECK:  nop                             # encoding: [0x90]
   lodsb
   //CHECK:  lodsb (%esi), %al             # encoding: [0x67,0xac]
   lodsb (%si), %al
   //CHECK:  lodsb (%si), %al              # encoding: [0xac]
   lodsb (%esi), %al
   //CHECK:  lodsb (%esi), %al             # encoding: [0x67,0xac]
   lodsl %gs:(%esi)
-  //CHECK:  lodsl %gs:(%esi), %eax        # encoding: [0x66,0x65,0x67,0xad]
+  //CHECK:  lodsl %gs:(%esi), %eax        # encoding: [0x67,0x66,0x65,0xad]
   lods (%esi), %ax
   //CHECK:  lodsw (%esi), %ax             # encoding: [0x67,0xad]
   stosw
   //CHECK:  stosw %ax, %es:(%edi)         # encoding: [0x67,0xab]
   stos %eax, (%edi)
-  //CHECK:  stosl %eax, %es:(%edi)        # encoding: [0x66,0x67,0xab]
+  //CHECK:  stosl %eax, %es:(%edi)        # encoding: [0x67,0x66,0xab]
   stosb %al, %es:(%edi)
   //CHECK:  stosb %al, %es:(%edi)         # encoding: [0x67,0xaa]
   scas %es:(%edi), %al
   //CHECK:  scasb %es:(%edi), %al         # encoding: [0x67,0xae]
   scas %es:(%di), %ax
   //CHECK:  scasw %es:(%di), %ax          # encoding: [0xaf]
   cmpsb
   //CHECK:  cmpsb %es:(%edi), (%esi)      # encoding: [0x67,0xa6]
   cmpsw (%edi), (%esi)
   //CHECK:  cmpsw %es:(%edi), (%esi)      # encoding: [0x67,0xa7]
   cmpsl %es:(%edi), %ss:(%esi)
-  //CHECK:  cmpsl %es:(%edi), %ss:(%esi)  # encoding: [0x66,0x36,0x67,0xa7]
+  //CHECK:  cmpsl %es:(%edi), %ss:(%esi)  # encoding: [0x67,0x66,0x36,0xa7]
   movsb (%esi), (%edi)
   //CHECK:  movsb (%esi), %es:(%edi)      # encoding: [0x67,0xa4]
   movsl %gs:(%esi), (%edi)
-  //CHECK:  movsl %gs:(%esi), %es:(%edi)  # encoding: [0x66,0x65,0x67,0xa5]
+  //CHECK:  movsl %gs:(%esi), %es:(%edi)  # encoding: [0x67,0x66,0x65,0xa5]
   outsb
   //CHECK:  outsb (%esi), %dx             # encoding: [0x67,0x6e]
   outsw %fs:(%esi), %dx
-  //CHECK:  outsw %fs:(%esi), %dx         # encoding: [0x64,0x67,0x6f]
+  //CHECK:  outsw %fs:(%esi), %dx         # encoding: [0x67,0x64,0x6f]
   insw %dx, (%di)
   //CHECK:  insw  %dx, %es:(%di)          # encoding: [0x6d]
   call $0x7ace,$0x7ace
   //CHECK:  lcalll  $31438, $31438          # encoding: [0x66,0x9a,0xce,0x7a,0x00,0x00,0xce,0x7a]
   ret
   //CHECK:  retl                            # encoding: [0x66,0xc3]
   pop %ss
   //CHECK:  popl  %ss                     # encoding: [0x66,0x17]

llvm/test/MC/X86/index-operations.s

 // 16: lodsb (%esi), %al # encoding: [0x67,0xac]
 
 lodsb (%si), %al
 // ERR64: invalid 16-bit base register
 // 32: lodsb (%si), %al # encoding: [0x67,0xac]
 // 16: lodsb (%si), %al # encoding: [0xac]
 
 lodsl %gs:(%esi)
-// 64: lodsl %gs:(%esi), %eax # encoding: [0x65,0x67,0xad]
+// 64: lodsl %gs:(%esi), %eax # encoding: [0x67,0x65,0xad]
 // 32: lodsl %gs:(%esi), %eax # encoding: [0x65,0xad]
-// 16: lodsl %gs:(%esi), %eax # encoding: [0x66,0x65,0x67,0xad]
+// 16: lodsl %gs:(%esi), %eax # encoding: [0x67,0x66,0x65,0xad]
 
 lodsl (%edi), %eax
 // ERR64: invalid operand
 // ERR32: invalid operand
 // ERR16: invalid operand
 
 lodsl 44(%edi), %eax
 // ERR64: invalid operand
 // ERR32: invalid operand
 // ERR16: invalid operand
 
 lods (%esi), %ax
-// 64: lodsw (%esi), %ax # encoding: [0x66,0x67,0xad]
+// 64: lodsw (%esi), %ax # encoding: [0x67,0x66,0xad]
 // 32: lodsw (%esi), %ax # encoding: [0x66,0xad]
 // 16: lodsw (%esi), %ax # encoding: [0x67,0xad]
 
 stosw
 // 64: stosw %ax, %es:(%rdi) # encoding: [0x66,0xab]
 // 32: stosw %ax, %es:(%edi) # encoding: [0x66,0xab]
 // 16: stosw %ax, %es:(%di) # encoding: [0xab]
 
 stos %eax, (%edi)
 // 64: stosl %eax, %es:(%edi) # encoding: [0x67,0xab]
 // 32: stosl %eax, %es:(%edi) # encoding: [0xab]
-// 16: stosl %eax, %es:(%edi) # encoding: [0x66,0x67,0xab]
+// 16: stosl %eax, %es:(%edi) # encoding: [0x67,0x66,0xab]
 
 stosb %al, %fs:(%edi)
 // ERR64: invalid operand for instruction
 // ERR32: invalid operand for instruction
 // ERR16: invalid operand for instruction
 
 stosb %al, %es:(%edi)
 // 64: stosb %al, %es:(%edi) # encoding: [0x67,0xaa]
 // 32: stosb %al, %es:(%edi) # encoding: [0xaa]
 // 16: stosb %al, %es:(%edi) # encoding: [0x67,0xaa]
 
 stosq
 // 64: stosq %rax, %es:(%rdi) # encoding: [0x48,0xab]
 // ERR32: 64-bit
 // ERR16: 64-bit
 
 stos %rax, (%edi)
-// 64:  stosq %rax, %es:(%edi) # encoding: [0x48,0x67,0xab]
+// 64:  stosq %rax, %es:(%edi) # encoding: [0x67,0x48,0xab]
 // ERR32: only available in 64-bit mode
 // ERR16: only available in 64-bit mode
 
 scas %es:(%edi), %al
 // 64: scasb %es:(%edi), %al # encoding: [0x67,0xae]
 // 32: scasb %es:(%edi), %al # encoding: [0xae]
 // 16: scasb %es:(%edi), %al # encoding: [0x67,0xae]
 
 scasq %es:(%edi)
-// 64: scasq %es:(%edi), %rax # encoding: [0x48,0x67,0xaf]
+// 64: scasq %es:(%edi), %rax # encoding: [0x67,0x48,0xaf]
 // ERR32: 64-bit
 // ERR16: 64-bit
 
 scasl %es:(%edi), %al
 // ERR64: invalid operand
 // ERR32: invalid operand
 // ERR16: invalid operand
 
 scas %es:(%di), %ax
 // ERR64: invalid 16-bit base register
 // 16: scasw %es:(%di), %ax # encoding: [0xaf]
-// 32: scasw %es:(%di), %ax # encoding: [0x66,0x67,0xaf]
+// 32: scasw %es:(%di), %ax # encoding: [0x67,0x66,0xaf]
 
 cmpsb
 // 64: cmpsb %es:(%rdi), (%rsi) # encoding: [0xa6]
 // 32: cmpsb %es:(%edi), (%esi) # encoding: [0xa6]
 // 16: cmpsb %es:(%di), (%si) # encoding: [0xa6]
 
 cmpsw (%edi), (%esi)
-// 64: cmpsw %es:(%edi), (%esi) # encoding: [0x66,0x67,0xa7]
+// 64: cmpsw %es:(%edi), (%esi) # encoding: [0x67,0x66,0xa7]
 // 32: cmpsw %es:(%edi), (%esi) # encoding: [0x66,0xa7]
 // 16: cmpsw %es:(%edi), (%esi) # encoding: [0x67,0xa7]
 
 cmpsb (%di), (%esi)
 // ERR64: invalid 16-bit base register
 // ERR32: mismatching source and destination
 // ERR16: mismatching source and destination
 
 cmpsl %es:(%edi), %ss:(%esi)
-// 64: cmpsl %es:(%edi), %ss:(%esi) # encoding: [0x36,0x67,0xa7]
+// 64: cmpsl %es:(%edi), %ss:(%esi) # encoding: [0x67,0x36,0xa7]
 // 32: cmpsl %es:(%edi), %ss:(%esi) # encoding: [0x36,0xa7]
-// 16: cmpsl %es:(%edi), %ss:(%esi) # encoding: [0x66,0x36,0x67,0xa7]
+// 16: cmpsl %es:(%edi), %ss:(%esi) # encoding: [0x67,0x66,0x36,0xa7]
 
 cmpsq (%rdi), (%rsi)
 // 64: cmpsq %es:(%rdi), (%rsi) # encoding: [0x48,0xa7]
 // ERR32: 64-bit
 // ERR16: 64-bit
 
 movsb (%esi), (%edi)
 // 64: movsb (%esi), %es:(%edi) # encoding: [0x67,0xa4]
 // 32: movsb (%esi), %es:(%edi) # encoding: [0xa4]
 // 16: movsb (%esi), %es:(%edi) # encoding: [0x67,0xa4]
 
 movsl %gs:(%esi), (%edi)
-// 64: movsl %gs:(%esi), %es:(%edi) # encoding: [0x65,0x67,0xa5]
+// 64: movsl %gs:(%esi), %es:(%edi) # encoding: [0x67,0x65,0xa5]
 // 32: movsl %gs:(%esi), %es:(%edi) # encoding: [0x65,0xa5]
-// 16: movsl %gs:(%esi), %es:(%edi) # encoding: [0x66,0x65,0x67,0xa5]
+// 16: movsl %gs:(%esi), %es:(%edi) # encoding: [0x67,0x66,0x65,0xa5]
 
 outsb
 // 64: outsb (%rsi), %dx # encoding: [0x6e]
 // 32: outsb (%esi), %dx # encoding: [0x6e]
 // 16: outsb (%si), %dx # encoding: [0x6e]
 
 outsw %fs:(%esi), %dx
-// 64: outsw %fs:(%esi), %dx # encoding: [0x66,0x64,0x67,0x6f]
+// 64: outsw %fs:(%esi), %dx # encoding: [0x67,0x66,0x64,0x6f]
 // 32: outsw %fs:(%esi), %dx # encoding: [0x66,0x64,0x6f]
-// 16: outsw %fs:(%esi), %dx # encoding: [0x64,0x67,0x6f]
+// 16: outsw %fs:(%esi), %dx # encoding: [0x67,0x64,0x6f]
 
 insw %dx, (%edi)
-// 64: insw %dx, %es:(%edi) # encoding: [0x66,0x67,0x6d]
+// 64: insw %dx, %es:(%edi) # encoding: [0x67,0x66,0x6d]
 // 32: insw %dx, %es:(%edi) # encoding: [0x66,0x6d]
 // 16: insw %dx, %es:(%edi) # encoding: [0x67,0x6d]
 
 insw %dx, (%bx)
 // ERR64: invalid 16-bit base register
-// 32: insw %dx, %es:(%di) # encoding: [0x66,0x67,0x6d]
+// 32: insw %dx, %es:(%di) # encoding: [0x67,0x66,0x6d]
 // 16: insw %dx, %es:(%di) # encoding: [0x6d]
 
 insw %dx, (%ebx)
-// 64: insw %dx, %es:(%edi) # encoding: [0x66,0x67,0x6d]
+// 64: insw %dx, %es:(%edi) # encoding: [0x67,0x66,0x6d]
 // 32: insw %dx, %es:(%edi) # encoding: [0x66,0x6d]
 // 16: insw %dx, %es:(%edi) # encoding: [0x67,0x6d]
 
 insw %dx, (%rbx)
 // 64: insw %dx, %es:(%rdi) # encoding: [0x66,0x6d]
 // ERR32: 64-bit
 // ERR16: 64-bit