[X86] Preserve redundant Address-Size override prefix

aaupov · aaupov · commit 1d3719820faf · 2022-03-16T08:30:29.000-07:00
Print and emit redundant Address-Size override prefix if it's set on the instruction. Reviewed By: skan Differential Revision: https://reviews.llvm.org/D120592
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp b/llvm/lib/Target/X86/MCTargetDesc/X86ATTInstPrinter.cpp
@@ -46,7 +46,7 @@ void X86ATTInstPrinter::printInst(const MCInst *MI, uint64_t Address,
   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".
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp b/llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.cpp
@@ -18,10 +18,11 @@
 #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;
 
@@ -349,7 +350,8 @@ void X86InstPrinterCommon::printOptionalSegReg(const MCInst *MI, unsigned OpNo,
   }
 }
 
-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();
@@ -379,6 +381,20 @@ void X86InstPrinterCommon::printInstFlags(const MCInst *MI, raw_ostream &O) {
     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,
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.h b/llvm/lib/Target/X86/MCTargetDesc/X86InstPrinterCommon.h
@@ -33,7 +33,8 @@ class X86InstPrinterCommon : public MCInstPrinter {
                      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);
 };
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86IntelInstPrinter.cpp b/llvm/lib/Target/X86/MCTargetDesc/X86IntelInstPrinter.cpp
@@ -40,7 +40,7 @@ void X86IntelInstPrinter::printRegName(raw_ostream &OS, unsigned RegNo) const {
 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 &&
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp b/llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp
@@ -156,65 +156,6 @@ static MCFixupKind getImmFixupKind(uint64_t TSFlags) {
   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
@@ -463,7 +404,7 @@ void X86MCCodeEmitter::emitMemModRMByte(const MCInst &MI, unsigned Op,
 
   // 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
@@ -672,27 +613,8 @@ bool X86MCCodeEmitter::emitPrefixImpl(unsigned &CurOp, const MCInst &MI,
     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.
@@ -708,39 +630,20 @@ bool X86MCCodeEmitter::emitPrefixImpl(unsigned &CurOp, const MCInst &MI,
   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;
   }
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp b/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp
@@ -72,6 +72,97 @@ bool X86_MC::hasLockPrefix(const MCInst &MI) {
   return MI.getFlags() & X86::IP_HAS_LOCK;
 }
 
+static 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) {
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h b/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h
@@ -63,6 +63,28 @@ 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,
diff --git a/llvm/test/MC/Disassembler/X86/addr32.s b/llvm/test/MC/Disassembler/X86/addr32.s
@@ -0,0 +1,5 @@
+# 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
diff --git a/llvm/test/MC/X86/code16gcc.s b/llvm/test/MC/X86/code16gcc.s
@@ -11,13 +11,13 @@
 	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
@@ -29,15 +29,15 @@
 	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
diff --git a/llvm/test/MC/X86/index-operations.s b/llvm/test/MC/X86/index-operations.s
