[X86] PR26554: Use not all set of alternative nops in 64 bit mode, but only those which are correct

[aturetsk]

Summary

Alternative nops added in http://reviews.llvm.org/D14178 are not actually nops in x86-64. However all 64 bit CPUs support true long nops.
This patch makes AsmBackend to use restricted set of alternative long nops for x86-64.

Ref: https://llvm.org/bugs/show_bug.cgi?id=26554

[aturetsk]

Ping.

[aturetsk]

Ping.

[aturetsk]

Ping.

[aturetsk]

Ping.

[bruno]

Hi Andrey,

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

353

Any reason why this shouldn't be done as a target feature as suggested in the FIXME above?

[aturetsk]

Hi Bruno,

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

353

Target feature can be used as indicator of true long nop support, but that won't fix the issue.

The alternative nop instructions I introduced in http://reviews.llvm.org/D14178 are not actually nops for x86-64 arch. But all CPUs which don't support true long nops are 32-bit-only. So I propose to use true long nops if we compile for x86-64 arch for any CPU even if it's marked as 'doesn't support true long nops'.
Well, we do generate 64-bit instructions for 32-bit-only CPUs when x86-64 arch is used, why not generate true long nops as well?
And I think 'generic' should use true long nops in 64-bit mode (since every real 64 bit CPU seems to support them) and use alternative long nops in 32 bit mode.

[aturetsk]

Ping.

[bruno]

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

353

I see. However, we could have the opposite logic, something like "FeatureShortNop", and mark generic, i386, etc with them. I looked up these CPUs and none of them have Feature64Bit, so guess HasNopl will simply become !FeatureShortNop.

[RKSimon]

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

353

Basing this off target features would also make it much easier to chose the optimal nop length for intel/amd (as discussed on PR22965).

[aturetsk]

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

353

Oh, I think my understanding of 'generic' CPU wasn't quite right. It's not supposed to be used in 64-bit mode, right? ('x86-64' CPU seems to be 64-bit 'generic')
If that's so then I see no reason to avoid using target features and I'll rework the patch (using FeatureLongNop).

Simon, how do you suggest to choose the optimal nop length in this case?
Use different target features for different max lenghts like this:
FeatureLongNop - allow nops of any length
FeatureLongNop7 - only allow nops smaller than 7 bytes
... etc?

[RKSimon]

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

353

Probably provide a X86Subtarget::getMaxNopLength() helper function that uses feature bits internally.

[aturetsk]

Use subtarget features to determine whether true long nops are supported or not.

[aturetsk]

I made AsmBackend to use subtarget features to determine whether true long nops are supported or not, as suggested.
Yet to fix the PR we still need additional code, see lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp : 61.
The most part of the patch is adding MCSubtargetInfo as argument for AsmBackend creator functions for all targets, that's why the patch is so big. But I didn't find any way to do it better.

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

62

This is to fix https://llvm.org/bugs/show_bug.cgi?id=26554.

95

Before this patch llvm would use true long nops when CPU is not specified, so I kept the behavior unchanged.

[bruno]

include/llvm/Support/TargetRegistry.h

116 ↗

(On Diff #59576)

I'm not sure about threading this around would yield any unwanted dependency. @Akira and @Eric, is this a problem?

lib/Target/X86/X86.td

42 ↗

(On Diff #59576)

These should be two distinct commits/patches, one for FeatureLongNop and another for FeatureFastNop7.

[RKSimon]

include/llvm/Support/TargetRegistry.h

118 ↗

(On Diff #59576)

The threading issue is the most important but can't Triple and CPU be gotten from MCSubtargetInfo? I realise this would be even more upheaval requiring the patch to be split further.

[aturetsk]

Get Triple and CPU from STI in AsmBackend.

[aturetsk]

I modified the patch as Simon suggested: AsmBackend now gets Triple and CPU from MCSubtargetInfo.
Note that the CPU from MCSubtargetInfo is a bit different from the CPU we used to pass as argument: if -mcpu isn't specified the CPU used to be "", but now it's "generic". I think that this is actually how it should be, however the change in behavior required to fix a couple of tests by specifying -mcpu option (the tests were expecting long nop generation but "generic" doesn't support them).

I'm planning to split this change-set into five commits into LLVM:

1. Use MCSubtargetInfo argument instead of CPU and Triple in create*AsmBackend functions
2. Replace CPU argument with MCSubtargetInfo in all AsmBackend classes in X86AsmBackend.cpp
3. Introduction of FeatureLongNop
4. Introduction of FeatureFastNop7
5. Fix for alternative long nop generation for x86-64

Also there is a small patch to be committed in clang: http://reviews.llvm.org/D21066

[echristo]

I don't think that MCAsmBackend is the right place for storing this information (and so the rest of the change is also not OK).

Taking a step back this needs to be looked at from two directions:

a) overall module/.o specific things
b) subtarget specific things

The problem is that the way the patch is written you're using the base subtarget all the time and not taking into account any other ones at the function level. So your nops won't vary depending on which function is requesting which cpu.

What I think we'll want to do is take writeNopData off of MCAsmBackend and work it into something that already has the subtarget and then for global nop data we'll use the default subtarget that comes with the module.

Make sense?

[aturetsk]

New version in https://reviews.llvm.org/D21374

lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

static unsigned getFixupKindLog2Size(unsigned Kind) {
   case FK_Data_8:
   case X86::reloc_global_offset_table8:
     return 3;
   }
 }
 
 namespace {
 
 class X86ELFObjectWriter : public MCELFObjectTargetWriter {
 public:
   X86ELFObjectWriter(bool is64Bit, uint8_t OSABI, uint16_t EMachine,
                      bool HasRelocationAddend, bool foobar)
     : MCELFObjectTargetWriter(is64Bit, OSABI, EMachine, HasRelocationAddend) {}
 };
 
 class X86AsmBackend : public MCAsmBackend {
   const StringRef CPU;
   bool HasNopl;
   uint64_t MaxNopLength;
+
+protected:
+  bool Is64Bit;
+
 public:
-  X86AsmBackend(const Target &T, StringRef CPU) : MCAsmBackend(), CPU(CPU) {
-    HasNopl = CPU != "generic" && CPU != "i386" && CPU != "i486" &&
+  X86AsmBackend(const Target &T, bool Is64Bit, StringRef CPU)
+      : MCAsmBackend(), CPU(CPU), Is64Bit(Is64Bit) {
+    // Only some 32 bit CPUs don't support true long nop instructions.
+    HasNopl =
+        Is64Bit || (CPU != "generic" && CPU != "i386" && CPU != "i486" &&
                     CPU != "i586" && CPU != "pentium" && CPU != "pentium-mmx" &&
-              CPU != "i686" && CPU != "k6" && CPU != "k6-2" && CPU != "k6-3" &&
-              CPU != "geode" && CPU != "winchip-c6" && CPU != "winchip2" &&
-              CPU != "c3" && CPU != "c3-2";
+                    CPU != "i686" && CPU != "k6" && CPU != "k6-2" &&
+                    CPU != "k6-3" && CPU != "geode" && CPU != "winchip-c6" &&
+                    CPU != "winchip2" && CPU != "c3" && CPU != "c3-2");
 
     // Max length of true long nop instruction is 15 bytes.
     // Max length of long nop replacement instruction is 7 bytes.
     // Taking into account SilverMont architecture features max length of nops
     // is reduced for it to achieve better performance.
     MaxNopLength = (!HasNopl || CPU == "slm") ? 7 : 15;
   }
 
   unsigned getNumFixupKinds() const override {
     return X86::NumTargetFixupKinds;
   }
 
   const MCFixupKindInfo &getFixupKindInfo(MCFixupKind Kind) const override {
     const static MCFixupKindInfo Infos[X86::NumTargetFixupKinds] = {
       { "reloc_riprel_4byte", 0, 4 * 8, MCFixupKindInfo::FKF_IsPCRel, },
       { "reloc_riprel_4byte_movq_load", 0, 4 * 8, MCFixupKindInfo::FKF_IsPCRel,},
       { "reloc_signed_4byte", 0, 4 * 8, 0},
static const uint8_t AltNops[7][10] = {
       {0x90, 0x8d, 0x74, 0x26, 0x00},
       // lea 0x0(%esi),%esi
       {0x8d, 0xb6, 0x00, 0x00, 0x00, 0x00},
       // lea 0x0(%esi),%esi
       {0x8d, 0xb4, 0x26, 0x00, 0x00, 0x00, 0x00},
   };
 
   // Select the right NOP table.
   // FIXME: Can we get if CPU supports long nops from the subtarget somehow?
   const uint8_t (*Nops)[10] = HasNopl ? TrueNops : AltNops;
-  assert(HasNopl || MaxNopLength <= 7);
+
+  // The alternative instructions are nops only on 32 bit CPUs.
+  assert(HasNopl || (MaxNopLength <= 7 && !Is64Bit));
 
   // Emit as many largest nops as needed, then emit a nop of the remaining
   // length.
   do {
     const uint8_t ThisNopLength = (uint8_t) std::min(Count, MaxNopLength);
     const uint8_t Prefixes = ThisNopLength <= 10 ? 0 : ThisNopLength - 10;
     for (uint8_t i = 0; i < Prefixes; i++)
       OW->write8(0x66);
     const uint8_t Rest = ThisNopLength - Prefixes;
     for (uint8_t i = 0; i < Rest; i++)
       OW->write8(Nops[Rest - 1][i]);
     Count -= ThisNopLength;
   } while (Count != 0);
 
   return true;
 }
 
 /* *** */
 
 namespace {
 
 class ELFX86AsmBackend : public X86AsmBackend {
 public:
   uint8_t OSABI;
-  ELFX86AsmBackend(const Target &T, uint8_t OSABI, StringRef CPU)
-      : X86AsmBackend(T, CPU), OSABI(OSABI) {}
+  ELFX86AsmBackend(const Target &T, bool Is64Bit, uint8_t OSABI, StringRef CPU)
+      : X86AsmBackend(T, Is64Bit, CPU), OSABI(OSABI) {}
 };
 
 class ELFX86_32AsmBackend : public ELFX86AsmBackend {
 public:
   ELFX86_32AsmBackend(const Target &T, uint8_t OSABI, StringRef CPU)
-    : ELFX86AsmBackend(T, OSABI, CPU) {}
+      : ELFX86AsmBackend(T, false, OSABI, CPU) {}
 
   MCObjectWriter *createObjectWriter(raw_pwrite_stream &OS) const override {
     return createX86ELFObjectWriter(OS, /*IsELF64*/ false, OSABI, ELF::EM_386);
   }
 };
 
 class ELFX86_X32AsmBackend : public ELFX86AsmBackend {
 public:
   ELFX86_X32AsmBackend(const Target &T, uint8_t OSABI, StringRef CPU)
-      : ELFX86AsmBackend(T, OSABI, CPU) {}
+      : ELFX86AsmBackend(T, false, OSABI, CPU) {}
 
   MCObjectWriter *createObjectWriter(raw_pwrite_stream &OS) const override {
     return createX86ELFObjectWriter(OS, /*IsELF64*/ false, OSABI,
                                     ELF::EM_X86_64);
   }
 };
 
 class ELFX86_IAMCUAsmBackend : public ELFX86AsmBackend {
 public:
   ELFX86_IAMCUAsmBackend(const Target &T, uint8_t OSABI, StringRef CPU)
-      : ELFX86AsmBackend(T, OSABI, CPU) {}
+      : ELFX86AsmBackend(T, false, OSABI, CPU) {}
 
   MCObjectWriter *createObjectWriter(raw_pwrite_stream &OS) const override {
     return createX86ELFObjectWriter(OS, /*IsELF64*/ false, OSABI,
                                     ELF::EM_IAMCU);
   }
 };
 
 class ELFX86_64AsmBackend : public ELFX86AsmBackend {
 public:
   ELFX86_64AsmBackend(const Target &T, uint8_t OSABI, StringRef CPU)
-    : ELFX86AsmBackend(T, OSABI, CPU) {}
+      : ELFX86AsmBackend(T, true, OSABI, CPU) {}
 
   MCObjectWriter *createObjectWriter(raw_pwrite_stream &OS) const override {
     return createX86ELFObjectWriter(OS, /*IsELF64*/ true, OSABI, ELF::EM_X86_64);
   }
 };
 
 class WindowsX86AsmBackend : public X86AsmBackend {
-  bool Is64Bit;
-
 public:
   WindowsX86AsmBackend(const Target &T, bool is64Bit, StringRef CPU)
-    : X86AsmBackend(T, CPU)
-    , Is64Bit(is64Bit) {
-  }
+      : X86AsmBackend(T, is64Bit, CPU) {}
 
   Optional<MCFixupKind> getFixupKind(StringRef Name) const override {
     return StringSwitch<Optional<MCFixupKind>>(Name)
         .Case("dir32", FK_Data_4)
         .Case("secrel32", FK_SecRel_4)
         .Case("secidx", FK_SecRel_2)
         .Default(MCAsmBackend::getFixupKind(Name));
   }

 
 class DarwinX86AsmBackend : public X86AsmBackend {
   const MCRegisterInfo &MRI;
 
   /// \brief Number of registers that can be saved in a compact unwind encoding.
   enum { CU_NUM_SAVED_REGS = 6 };
 
   mutable unsigned SavedRegs[CU_NUM_SAVED_REGS];
-  bool Is64Bit;
 
   unsigned OffsetSize;                   ///< Offset of a "push" instruction.
   unsigned MoveInstrSize;                ///< Size of a "move" instruction.
   unsigned StackDivide;                  ///< Amount to adjust stack size by.
 protected:
   /// \brief Size of a "push" instruction for the given register.
   unsigned PushInstrSize(unsigned Reg) const {
     switch (Reg) {
uint32_t encodeCompactUnwindRegistersWithoutFrame(unsigned RegCount) const {
     assert((permutationEncoding & 0x3FF) == permutationEncoding &&
            "Invalid compact register encoding!");
     return permutationEncoding;
   }
 
 public:
   DarwinX86AsmBackend(const Target &T, const MCRegisterInfo &MRI, StringRef CPU,
                       bool Is64Bit)
-    : X86AsmBackend(T, CPU), MRI(MRI), Is64Bit(Is64Bit) {
+      : X86AsmBackend(T, Is64Bit, CPU), MRI(MRI) {
     memset(SavedRegs, 0, sizeof(SavedRegs));
     OffsetSize = Is64Bit ? 8 : 4;
     MoveInstrSize = Is64Bit ? 3 : 2;
     StackDivide = Is64Bit ? 8 : 4;
   }
 };
 
 class DarwinX86_32AsmBackend : public DarwinX86AsmBackend {

test/MC/X86/x86_long_nop.s

 # RUN: llvm-mc -filetype=obj -arch=x86 -triple=x86_64-pc-linux-gnu %s | llvm-objdump -d -no-show-raw-insn - | FileCheck %s
 # RUN: llvm-mc -filetype=obj -arch=x86 -triple=i686-pc-linux-gnu %s | llvm-objdump -d -no-show-raw-insn - | FileCheck %s
 # RUN: llvm-mc -filetype=obj -arch=x86 -triple=x86_64-apple-darwin10.0 %s | llvm-objdump -d -no-show-raw-insn - | FileCheck %s
 # RUN: llvm-mc -filetype=obj -arch=x86 -triple=i686-apple-darwin8 %s | llvm-objdump -d -no-show-raw-insn - | FileCheck %s
 # RUN: llvm-mc -filetype=obj -arch=x86 -triple=i686-pc-linux-gnu -mcpu=slm %s | llvm-objdump -d -no-show-raw-insn - | FileCheck --check-prefix=SLM %s
+# RUN: llvm-mc -filetype=obj -arch=x86-64 -triple=x86_64-pc-linux-gnu -mcpu=i386 %s | llvm-objdump -d -no-show-raw-insn - | FileCheck --check-prefix=64BIT %s
 
 # Ensure alignment directives also emit sequences of 15-byte NOPs on processors
 # capable of using long NOPs.
 inc %eax
 .p2align 5
 inc %eax
 # CHECK: 0:  inc
 # CHECK-NEXT: 1:  nop
 # CHECK-NEXT: 10:  nop
 # CHECK-NEXT: 1f:  nop
 # CHECK-NEXT: 20:  inc
 
 # On Silvermont we emit only 7 byte NOPs since longer NOPs are not profitable
 # SLM: 0:  inc
 # SLM-NEXT: 1:  nop
 # SLM-NEXT: 8:  nop
 # SLM-NEXT: f:  nop
 # SLM-NEXT: 16:  nop
 # SLM-NEXT: 1d:  nop
 # SLM-NEXT: 20:  inc
+
+# Ensure that for x86-64 we emit true long nops regardless of -mcpu value.
+# 64BIT: 0:  inc
+# 64BIT-NEXT: 2:  nop
+# 64BIT-NEXT: 11:  nop
+# 64BIT-NEXT: 20:  inc