X86: Don't fold TEST into ADD ...@GOTTPOFF/GOTNTPOFF/INDNTPOFF

[jyknight]

Summary

The linker may convert such an ADD into a LEA, so we must not
use the EFLAGS output.

This causes miscompiles with ubsan after bacdf80f42b46044262e97e98398d1bd0b75900d
added llvm.threadlocal.address -- previously, global variables were
known to be non-null, but the intrinsic is not currently known to return
nonnull. (That should be corrected, but it shouldn't've caused
miscompiles!)

[craig.topper]

llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.cpp

82

Unrelated change?

llvm/lib/Target/X86/X86InstrInfo.cpp

4091

80 columns

[MaskRay]

llvm/lib/Target/X86/X86InstrInfo.cpp

4091

Can use llvm::is_contained({...}, Flags) to make this shorter.

[MaskRay]

This causes miscompiles with ubsan after bacdf80f42b46044262e97e98398d1bd0b75900d

ubsan can be more specific, -fsanitize=null

llvm/lib/Target/X86/X86InstrInfo.cpp

4085

x86-64 psABI should be more authoritative than ""ELF Handling for Thread-Local Storage"".

[zuban32]

llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.cpp

82

Agreed, pls remove

[jyknight]

Address minor issues.

llvm/lib/Target/X86/X86InstrInfo.cpp

4085

OTHER references in the code point to this document. E.g. X86TargetLowering::shouldReduceLoadWidth in llvm/lib/Target/X86/X86ISelLowering.cpp:

// "ELF Handling for Thread-Local Storage" specifies that R_X86_64_GOTTPOFF
// relocation target a movq or addq instruction: don't let the load shrink.

and the definition of MO_GOTTPOFF in llvm/lib/Target/X86/MCTargetDesc/X86BaseInfo.h:

/// See 'ELF Handling for Thread-Local Storage' for more details.
///    SYMBOL_LABEL @GOTTPOFF
MO_GOTTPOFF,

I'd prefer to be consistent, so will leave this.

4091

Turns out this is a little annoying, given that the first arg turns out to be initializer_list<X86II::TOF>, but getTargetFlags returns unsigned int, and llvm::is_contained is template <typename T> constexpr bool is_contained(std::initializer_list<T> Set, T Value).

I could workaround with explicit casts here, but that feels uglier than just using || so I'll just leave this alone. (I expect that is_contained should be fixed, but I'm not doing that in this change).

[zuban32]

LGTM regarding removal of unrelated SPIR-V code

llvm/lib/Target/SPIRV/SPIRVDuplicatesTracker.cpp

if (MMI) {
     const Module *M = MMI->getModule();
     for (auto F = M->begin(), E = M->end(); F != E; ++F) {
       const MachineFunction *MF = MMI->getMachineFunction(*F);
       if (!MF)
         continue;
       for (const MachineBasicBlock &MBB : *MF) {
         for (const MachineInstr &CMI : MBB) {
           MachineInstr &MI = const_cast<MachineInstr &>(CMI);
+#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
           MI.dump();
+#endif
           if (MI.getNumExplicitDefs() > 0 &&
               Reg2Entry.count(&MI.getOperand(0))) {
             dbgs() << "\t[";
             for (SPIRV::DTSortableEntry *D :
                  Reg2Entry.lookup(&MI.getOperand(0))->getDeps())
               dbgs() << Register::virtReg2Index(D->lookup(MF)) << ", ";
             dbgs() << "]\n";
           }
         }
       }
     }
   }
 }

llvm/lib/Target/X86/X86InstrInfo.cpp

 
 /// Check whether the definition can be converted
 /// to remove a comparison against zero.
 inline static bool isDefConvertible(const MachineInstr &MI, bool &NoSignFlag,
                                     bool &ClearsOverflowFlag) {
   NoSignFlag = false;
   ClearsOverflowFlag = false;
 
+  // "ELF Handling for Thread-Local Storage" specifies that x86-64 GOTTPOFF, and
+  // i386 GOTNTPOFF/INDNTPOFF relocations can convert an ADD to a LEA during
+  // Initial Exec to Local Exec relaxation. In these cases, we must not depend
+  // on the EFLAGS modification of ADD actually happening in the final binary.
+  if (MI.getOpcode() == X86::ADD64rm || MI.getOpcode() == X86::ADD32rm) {
+    unsigned Flags = MI.getOperand(5).getTargetFlags();
+    if (Flags == X86II::MO_GOTTPOFF || Flags == X86II::MO_INDNTPOFF || Flags == X86II::MO_GOTNTPOFF)
+      return false;
+  }
+
   switch (MI.getOpcode()) {
   default: return false;
 
   // The shift instructions only modify ZF if their shift count is non-zero.
   // N.B.: The processor truncates the shift count depending on the encoding.
   case X86::SAR8ri:    case X86::SAR16ri:  case X86::SAR32ri:case X86::SAR64ri:
   case X86::SHR8ri:    case X86::SHR16ri:  case X86::SHR32ri:case X86::SHR64ri:
      return getTruncatedShiftCount(MI, 2) != 0;

llvm/test/CodeGen/X86/peephole-nofold-tpoff-x86.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -run-pass=peephole-opt -mtriple=i386-- %s -o - | FileCheck %s
+
+# Linkers may change `addq xx@GOTNTPOFF, %reg` to `leaq OFFSET(%reg), %reg`,
+# so we must not depend upon the EFLAGS output. Verify that the TEST
+# instruction won't be folded into the ADD.
+
+# NOTE: the IR will no longer actually produce the input MIR after
+# llvm.threadlocal.address intrinsic is annotated as having a nonnull
+# result.
+
+# NOTE2: the foo_nopic MIR was produced from IR with --relocation-model=static
+# while foo_pic's MIR was produced with --relocation-model=pic.
+
+--- |
+  target datalayout = "e-m:e-p:32:32-p270:32:32-p271:32:32-p272:64:64-f64:32:64-f80:32-n8:16:32-S128"
+  target triple = "i386-unknown-linux-gnu"
+
+  @x = external thread_local(initialexec) global i32, align 4
+
+  define i32 @foo_nopic() {
+    %1 = tail call ptr @llvm.threadlocal.address.p0(ptr nonnull @x)
+    %cmp = icmp eq ptr %1, null
+    %zext = zext i1 %cmp to i32
+    ret i32 %zext
+  }
+
+  define i32 @foo_pic() {
+    %1 = tail call ptr @llvm.threadlocal.address.p0(ptr nonnull @x)
+    %cmp = icmp eq ptr %1, null
+    %zext = zext i1 %cmp to i32
+    ret i32 %zext
+  }
+
+  ; Function Attrs: nocallback nofree nosync nounwind readnone speculatable willreturn
+  declare ptr @llvm.threadlocal.address.p0(ptr) #0
+
+  attributes #0 = { nocallback nofree nosync nounwind readnone speculatable willreturn }
+
+...
+---
+name:            foo_nopic
+alignment:       16
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gr32 }
+  - { id: 1, class: gr32 }
+  - { id: 2, class: gr8 }
+  - { id: 3, class: gr32 }
+frameInfo:
+  maxAlignment:    1
+machineFunctionInfo: {}
+body:             |
+  bb.0 (%ir-block.0):
+    ; CHECK-LABEL: name: foo_nopic
+    ; CHECK: [[MOV32rm:%[0-9]+]]:gr32 = MOV32rm $noreg, 1, $noreg, 0, $gs :: (load (s32) from `ptr addrspace(256) null`, addrspace 256)
+    ; CHECK-NEXT: [[ADD32rm:%[0-9]+]]:gr32 = ADD32rm [[MOV32rm]], $noreg, 1, $noreg, target-flags(x86-indntpoff) @x, $noreg, implicit-def dead $eflags :: (load (s32) from got)
+    ; CHECK-NEXT: TEST32rr [[ADD32rm]], [[ADD32rm]], implicit-def $eflags
+    ; CHECK-NEXT: [[SETCCr:%[0-9]+]]:gr8 = SETCCr 4, implicit $eflags
+    ; CHECK-NEXT: [[MOVZX32rr8_:%[0-9]+]]:gr32 = MOVZX32rr8 killed [[SETCCr]]
+    ; CHECK-NEXT: $eax = COPY [[MOVZX32rr8_]]
+    ; CHECK-NEXT: RET 0, $eax
+    %0:gr32 = MOV32rm $noreg, 1, $noreg, 0, $gs :: (load (s32) from `ptr addrspace(256) null`, addrspace 256)
+    %1:gr32 = ADD32rm %0, $noreg, 1, $noreg, target-flags(x86-indntpoff) @x, $noreg, implicit-def dead $eflags :: (load (s32) from got)
+    TEST32rr %1, %1, implicit-def $eflags
+    %2:gr8 = SETCCr 4, implicit $eflags
+    %3:gr32 = MOVZX32rr8 killed %2
+    $eax = COPY %3
+    RET 0, $eax
+
+...
+---
+name:            foo_pic
+alignment:       16
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gr32_nosp }
+  - { id: 1, class: gr32 }
+  - { id: 2, class: gr32 }
+  - { id: 3, class: gr8 }
+  - { id: 4, class: gr32 }
+  - { id: 5, class: gr32 }
+frameInfo:
+  maxAlignment:    1
+machineFunctionInfo: {}
+body:             |
+  bb.0 (%ir-block.0):
+    ; CHECK-LABEL: name: foo_pic
+    ; CHECK: [[MOVPC32r:%[0-9]+]]:gr32 = MOVPC32r 0, implicit $esp, implicit $ssp
+    ; CHECK-NEXT: [[ADD32ri:%[0-9]+]]:gr32_nosp = ADD32ri [[MOVPC32r]], target-flags(x86-got-absolute-address) &_GLOBAL_OFFSET_TABLE_, implicit-def $eflags
+    ; CHECK-NEXT: [[MOV32rm:%[0-9]+]]:gr32 = MOV32rm $noreg, 1, $noreg, 0, $gs :: (load (s32) from `ptr addrspace(256) null`, addrspace 256)
+    ; CHECK-NEXT: [[ADD32rm:%[0-9]+]]:gr32 = ADD32rm [[MOV32rm]], [[ADD32ri]], 1, $noreg, target-flags(x86-gotntpoff) @x, $noreg, implicit-def dead $eflags :: (load (s32) from got)
+    ; CHECK-NEXT: TEST32rr [[ADD32rm]], [[ADD32rm]], implicit-def $eflags
+    ; CHECK-NEXT: [[SETCCr:%[0-9]+]]:gr8 = SETCCr 4, implicit $eflags
+    ; CHECK-NEXT: [[MOVZX32rr8_:%[0-9]+]]:gr32 = MOVZX32rr8 killed [[SETCCr]]
+    ; CHECK-NEXT: $eax = COPY [[MOVZX32rr8_]]
+    ; CHECK-NEXT: RET 0, $eax
+    %5:gr32 = MOVPC32r 0, implicit $esp, implicit $ssp
+    %0:gr32_nosp = ADD32ri %5, target-flags(x86-got-absolute-address) &_GLOBAL_OFFSET_TABLE_, implicit-def $eflags
+    %1:gr32 = MOV32rm $noreg, 1, $noreg, 0, $gs :: (load (s32) from `ptr addrspace(256) null`, addrspace 256)
+    %2:gr32 = ADD32rm %1, %0, 1, $noreg, target-flags(x86-gotntpoff) @x, $noreg, implicit-def dead $eflags :: (load (s32) from got)
+    TEST32rr %2, %2, implicit-def $eflags
+    %3:gr8 = SETCCr 4, implicit $eflags
+    %4:gr32 = MOVZX32rr8 killed %3
+    $eax = COPY %4
+    RET 0, $eax
+...

llvm/test/CodeGen/X86/peephole-nofold-tpoff-x86_64.mir

+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -run-pass=peephole-opt -mtriple=x86_64-- %s -o - | FileCheck %s
+
+# Linkers may change `addq xx@GOTTPOFF, %reg` to `leaq OFFSET(%reg), %reg`,
+# so we must not depend upon the EFLAGS output. Verify that the TEST
+# instruction won't be folded into the ADD.
+
+# NOTE: the IR will no longer actually produce the input MIR after
+# llvm.threadlocal.address intrinsic is annotated as having a nonnull
+# result.
+
+--- |
+  target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
+
+  @x = external thread_local(initialexec) global i32, align 4
+
+  define i32 @foo() {
+    %1 = tail call ptr @llvm.threadlocal.address.p0(ptr nonnull @x)
+    %cmp = icmp eq ptr %1, null
+    %zext = zext i1 %cmp to i32
+    ret i32 %zext
+  }
+
+  ; Function Attrs: nocallback nofree nosync nounwind readnone speculatable willreturn
+  declare ptr @llvm.threadlocal.address.p0(ptr) #0
+
+  attributes #0 = { nocallback nofree nosync nounwind readnone speculatable willreturn }
+
+...
+---
+name:            foo
+alignment:       16
+tracksRegLiveness: true
+registers:
+  - { id: 0, class: gr64 }
+  - { id: 1, class: gr64 }
+  - { id: 2, class: gr8 }
+  - { id: 3, class: gr32 }
+frameInfo:
+  maxAlignment:    1
+machineFunctionInfo: {}
+body:             |
+  bb.0 (%ir-block.0):
+    ; CHECK-LABEL: name: foo
+    ; CHECK: [[MOV64rm:%[0-9]+]]:gr64 = MOV64rm $noreg, 1, $noreg, 0, $fs :: (load (s64) from `ptr addrspace(257) null`, addrspace 257)
+    ; CHECK-NEXT: [[ADD64rm:%[0-9]+]]:gr64 = ADD64rm [[MOV64rm]], $rip, 1, $noreg, target-flags(x86-gottpoff) @x, $noreg, implicit-def dead $eflags :: (load (s64) from got)
+    ; CHECK-NEXT: TEST64rr [[ADD64rm]], [[ADD64rm]], implicit-def $eflags
+    ; CHECK-NEXT: [[SETCCr:%[0-9]+]]:gr8 = SETCCr 4, implicit $eflags
+    ; CHECK-NEXT: [[MOVZX32rr8_:%[0-9]+]]:gr32 = MOVZX32rr8 killed [[SETCCr]]
+    ; CHECK-NEXT: $eax = COPY [[MOVZX32rr8_]]
+    ; CHECK-NEXT: RET 0, $eax
+    %0:gr64 = MOV64rm $noreg, 1, $noreg, 0, $fs :: (load (s64) from `ptr addrspace(257) null`, addrspace 257)
+    %1:gr64 = ADD64rm %0, $rip, 1, $noreg, target-flags(x86-gottpoff) @x, $noreg, implicit-def dead $eflags :: (load (s64) from got)
+    TEST64rr %1, %1, implicit-def $eflags
+    %2:gr8 = SETCCr 4, implicit $eflags
+    %3:gr32 = MOVZX32rr8 killed %2
+    $eax = COPY %3
+    RET 0, $eax
+
+...