[BOLT] Strip redundant AdSize override prefix

[Amir]

Summary

Since LLVM MC now preserves redundant AdSize override prefix (0x67), remove it
in BOLT explicitly (-x86-strip-redundant-adsize, on by default).

Test Plan:
bin/llvm-lit -a bolt/test/X86/addr32.s

[Amir]

Update test

[Amir]

Update test, fix shared build

[Amir]

@maksfb, @rafaelauler
Ping. We need this functionality to strip redundant addr32 prefix in BOLT which would otherwise be preserved as a result of D120592.

[rafauler]

LGTM (nit below)

bolt/lib/Target/X86/X86MCPlusBuilder.cpp

43–44

Can we call this (...)AddressSize and -address-size? I find adsize to be a bit confusing.

[Amir]

Address nit

[Amir]

Fix test

[Amir]

Rebase

bolt/include/bolt/Core/MCPlusBuilder.h

 
   /// Morph an indirect call into a load where \p Reg holds the call target.
   virtual void convertIndirectCallToLoad(MCInst &Inst, MCPhysReg Reg) {
     llvm_unreachable("not implemented");
   }
 
   /// Replace instruction with a shorter version that could be relaxed later
   /// if needed.
-  virtual bool shortenInstruction(MCInst &Inst) const {
+  virtual bool shortenInstruction(MCInst &Inst,
+                                  const MCSubtargetInfo &STI) const {
     llvm_unreachable("not implemented");
     return false;
   }
 
   /// Convert a move instruction into a conditional move instruction, given a
   /// condition code.
   virtual bool
   convertMoveToConditionalMove(MCInst &Inst, unsigned CC,

bolt/lib/Passes/BinaryPasses.cpp

uint64_t ShortenInstructions::shortenInstructions(BinaryFunction &Function) {
   uint64_t Count = 0;
   const BinaryContext &BC = Function.getBinaryContext();
   for (BinaryBasicBlock &BB : Function) {
     for (MCInst &Inst : BB) {
       MCInst OriginalInst;
       if (opts::Verbosity > 2)
         OriginalInst = Inst;
 
-      if (!BC.MIB->shortenInstruction(Inst))
+      if (!BC.MIB->shortenInstruction(Inst, *BC.STI))
         continue;
 
       if (opts::Verbosity > 2) {
         outs() << "BOLT-INFO: shortening:\nBOLT-INFO:    ";
         BC.printInstruction(outs(), OriginalInst, 0, &Function);
         outs() << "BOLT-INFO: to:";
         BC.printInstruction(outs(), Inst, 0, &Function);
       }

bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp

public:
   }
 
   bool hasEVEXEncoding(const MCInst &) const override { return false; }
 
   bool isMacroOpFusionPair(ArrayRef<MCInst> Insts) const override {
     return false;
   }
 
-  bool shortenInstruction(MCInst &) const override { return false; }
+  bool shortenInstruction(MCInst &, const MCSubtargetInfo &) const override {
+    return false;
+  }
 
   bool isADRP(const MCInst &Inst) const override {
     return Inst.getOpcode() == AArch64::ADRP;
   }
 
   bool isADR(const MCInst &Inst) const override {
     return Inst.getOpcode() == AArch64::ADR;
   }

bolt/lib/Target/X86/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS
   BOLTCore
+  BOLTUtils
   MC
   Object
   Support
   ${LLVM_TARGETS_TO_BUILD}
   )
 
 add_llvm_library(LLVMBOLTTargetX86
   X86MCPlusBuilder.cpp
 
   DEPENDS
   intrinsics_gen
   X86CommonTableGen
   )
 
 include_directories(${LLVM_MAIN_SRC_DIR}/lib/Target/X86 ${LLVM_BINARY_DIR}/lib/Target/X86)

bolt/lib/Target/X86/X86MCPlusBuilder.cpp

 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCFixupKindInfo.h"
 #include "llvm/MC/MCInst.h"
 #include "llvm/MC/MCInstBuilder.h"
 #include "llvm/MC/MCInstrInfo.h"
 #include "llvm/MC/MCRegister.h"
 #include "llvm/MC/MCRegisterInfo.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/DataExtractor.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ErrorOr.h"
 #include <set>
 
 #define DEBUG_TYPE "mcplus"
 
 using namespace llvm;
 using namespace bolt;
 
+namespace opts {
+
+extern cl::OptionCategory BoltOptCategory;
+
+static cl::opt<bool> X86StripRedundantAddressSize(
+    "x86-strip-redundant-address-size",
+    cl::desc("Remove redundant Address-Size override prefix"), cl::init(true),
+    cl::ZeroOrMore, cl::cat(BoltOptCategory));
+
+} // namespace opts
+
 namespace {
 
 unsigned getShortBranchOpcode(unsigned Opcode) {
   switch (Opcode) {
   default:
     return Opcode;
   case X86::JMP_2: return X86::JMP_1;
   case X86::JMP_4: return X86::JMP_1;
if (Inst.getOpcode() == X86::CALL64r ||
       Inst.setOpcode(X86::MOV64rr);
       Inst.insert(Inst.begin(), MCOperand::createReg(Reg));
       return;
     }
     LLVM_DEBUG(Inst.dump());
     llvm_unreachable("not implemented");
   }
 
-  bool shortenInstruction(MCInst &Inst) const override {
+  bool shortenInstruction(MCInst &Inst,
+                          const MCSubtargetInfo &STI) const override {
     unsigned OldOpcode = Inst.getOpcode();
     unsigned NewOpcode = OldOpcode;
 
-    // Check and remove EIZ/RIZ. These cases represent ambiguous cases where SIB
-    // byte is present, but no index is used and modrm alone shoud have been
-    // enough. Converting to NoRegister effectively removes the SIB byte.
     int MemOpNo = getMemoryOperandNo(Inst);
+
+    // Check and remove redundant Address-Size override prefix.
+    if (opts::X86StripRedundantAddressSize) {
+      uint64_t TSFlags = Info->get(OldOpcode).TSFlags;
+      unsigned Flags = Inst.getFlags();
+
+      if (!X86_MC::needsAddressSizeOverride(Inst, STI, MemOpNo, TSFlags) &&
+          Flags & X86::IP_HAS_AD_SIZE)
+        Inst.setFlags(Flags ^ X86::IP_HAS_AD_SIZE);
+    }
+
+    // Check and remove EIZ/RIZ. These cases represent ambiguous cases where
+    // SIB byte is present, but no index is used and modrm alone should have
+    // been enough. Converting to NoRegister effectively removes the SIB byte.
     if (MemOpNo >= 0) {
       MCOperand &IndexOp =
           Inst.getOperand(static_cast<unsigned>(MemOpNo) + X86::AddrIndexReg);
       if (IndexOp.getReg() == X86::EIZ || IndexOp.getReg() == X86::RIZ)
         IndexOp = MCOperand::createReg(X86::NoRegister);
     }
 
     if (isBranch(Inst)) {
for (unsigned int i = 0; i < Targets.size(); ++i) {
       CompareInst.addOperand(MCOperand::createReg(IndexReg));
 
       const uint64_t CaseIdx = Targets[i].second;
       // Immediate address is out of sign extended 32 bit range.
       if (int64_t(CaseIdx) != int64_t(int32_t(CaseIdx)))
         return BlocksVectorTy();
 
       CompareInst.addOperand(MCOperand::createImm(CaseIdx));
-      shortenInstruction(CompareInst);
+      shortenInstruction(CompareInst, *Ctx->getSubtargetInfo());
 
       // jump to next target compare.
       NextTarget =
           Ctx->createNamedTempSymbol(); // generate label for the next block
       CurBB->push_back(MCInst());
 
       MCInst &JEInst = CurBB->back();
       JEInst.setLoc(IJmpInst.getLoc());

bolt/test/X86/addr32.s

+# Check that we don't accidentally strip addr32 prefix
+
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o
+# RUN: ld.lld %t.o -o %t.exe -nostdlib
+# RUN: llvm-objdump -d %t.exe | FileCheck %s
+# RUN: llvm-bolt %t.exe -o %t.out -lite=0 -x86-strip-redundant-address-size=false
+# RUN: llvm-objdump -d %t.out | FileCheck %s
+# CHECK: 67 e8 {{.*}} addr32 callq {{.*}} <foo>
+# RUN: llvm-bolt %t.exe -o %t.out -lite=0 -x86-strip-redundant-address-size=true
+# remove test name from objdump output, to only search for addr32 in disassembly
+# RUN: llvm-objdump -d %t.out | grep -v addr32.s | FileCheck %s --check-prefix=CHECK-STRIP
+# CHECK-STRIP-NOT: addr32
+
+.globl  _start
+.type _start, @function
+_start:
+.code64
+  addr32 callq foo
+  ret
+  .size _start, .-_start
+
+.globl  foo
+.type foo, @function
+foo:
+  ud2