Remove unnecessary load via GOT when accessing globals with PIE in x86_64

[tmsriram]

Summary

PR #24964

LLVM generates the following code with PIE for this example:

hello.cpp

int a = 0;

int main() {

return a;
}

$ clang -O2 -fPIE hello.cc -S
$ cat hello.s

main: # @main
movq a@GOTPCREL(%rip), %rax
movl (%rax), %eax
retq

Creating a GOT entry for global 'a' and storing its address there is unnecessary as 'a' is defined in hello.cpp which will be linked into a position independent executable (fPIE). Hence, the definition of 'a' cannot be overridden and we can remove a load. The efficient access is this:

main: # @main
movl a(%rip), %eax
retq

This patch fixes the problem.

[rnk]

This needs a new LLVM IR lit test. Take what you have in the comments and add -S -emit-llvm to the clang command line, and you'll have the beginning of one. You should probably test stores, loads, and taking the address of such a global. You should probably also exhaustively test the linkage types. The non-ODR linkage types in particular probably can't use this optimization.

lib/Target/X86/X86Subtarget.cpp

91 ↗

(On Diff #53485)

You don't want hasExactDefinition, it will be false for linkonce_odr and weak_odr globals, which can benefit from this more efficient access. I think you want something equivalent to this condition:

TM.Options.PositionIndependentExecutable && !GV->isDeclaration() && !GV->mayBeOverridden()

[DavidKreitzer]

lib/Target/X86/X86Subtarget.cpp

103 ↗

(On Diff #53485)

Do you want to fix the 32-bit case too? Under the same isPIEDefinition condition as in the 64-bit case, you can use X86II::MO_GOTOFF which also saves a load from the GOT.

[hjl.tools]

Does it fix

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

[AsafBadouh]

lib/Target/X86/X86Subtarget.cpp

91 ↗

(On Diff #53485)

I agree with RNK,
I'm not sure if it will handle weak attribute and reference to function.
for example:

long  bar_add; 
int __attribute__((weak)) weak_x = 3; 
int ext_bar(void);  // extern function

int bar()
{
  return (long)&ext_bar; // reading address of extern function should use GOT
}

long foo()
{
  bar_add = (long)&bar;       // reading address of bar should be directly (not by GOT)
  return bar_add
          + weak_x;
}

bar() should be compute directly, (not loaded from GOT like extern ext_bar()
so it should generate code like:

bar():
[...]
        mov     rax, QWORD PTR ext_bar()@GOTPCREL[rip]
[...]
        ret
foo():
[...]
        lea     rax, bar()[rip]
        mov     QWORD PTR bar_add[rip], rax
        mov     eax, DWORD PTR weak_x[rip]
[...]
        ret

I think the condition should be something similar to this :

TM.Options.PositionIndependentExecutable && !(GV->isDeclarationForLinker() && isa<Function>(GV))

[tmsriram]

lib/Target/X86/X86Subtarget.cpp

91 ↗

(On Diff #53485)

It handles function pointer references fine. FWIW, I tried the same example with my patch and I got the code you desired. Checking explicity for functions is not necessary. I will incorporate rnk@'s changes.

[tmsriram]

• Fixed the check that decides when to not use GOT.
• Extended the test case for x86-64.

[tmsriram]

Handle global access for 32-bit.

[tmsriram]

Fix emutls-pie.ll

[AsafBadouh]

test/CodeGen/X86/global-access-pie.ll

30 ↗

(On Diff #54442)

I think that weak global symbols should not be referred with GOT.
gcc5.3.0 for example will generate:

mov b(%rip), %rax

[tmsriram]

test/CodeGen/X86/global-access-pie.ll

30 ↗

(On Diff #54442)

I agree and I know about it. I am going to work on another patch that uses copy relocations to avoid the GOT for any kind of global access. This what happens in GCC now with PIE. I punted on this as this case would be handled then. Maybe I should not and just fix this too.

[tmsriram]

• Moved check for Global Definition in PIE to a new function.
• Fixed weak global symbol access for PIE to not use the GOT.

[rnk]

lib/Target/X86/X86Subtarget.cpp

62–63 ↗

(On Diff #54544)

This isn't the usual formatting, consider running clang-format

65–67 ↗

(On Diff #54544)

After doing some experiments, I think the predicate we want here is really TM.Options.PositionIndependentExecutable && !GV->isDeclarationForLinker().

Basically, once you know you are looking at a symbol definition that will become part of an executable, the only thing that can replace it is another definition within the executable. The glibc loader does *not* appear to replace weak symbols in an executable with strong symbols from a DSO. I mistakenly thought it would work the same way the linker does, where strong symbols beat weak symbols.

[tmsriram]

• Fix check for global definition.
• Move the definition of isGlobalDefinedInPIE into X86Subtarget.h

[rnk]

lgtm

We might consider merging the new tests with pie.ll, but that is not essential.

llvm/trunk/lib/Target/X86/X86Subtarget.h

bool isCallingConvWin64(CallingConv::ID CC) const {
     case CallingConv::X86_64_SysV:
       return false;
     // Otherwise, who knows what this is.
     default:
       return false;
     }
   }
 
+  /// Determine if this global is defined in a Position Independent
+  /// Executable (PIE) where its definition cannot be interposed.
+  bool isGlobalDefinedInPIE(const GlobalValue *GV,
+                            const TargetMachine &TM) const {
+    return TM.Options.PositionIndependentExecutable &&
+           !GV->isDeclarationForLinker();
+  }
+
   /// ClassifyGlobalReference - Classify a global variable reference for the
   /// current subtarget according to how we should reference it in a non-pcrel
   /// context.
   unsigned char ClassifyGlobalReference(const GlobalValue *GV,
                                         const TargetMachine &TM)const;
 
   /// classifyGlobalFunctionReference - Classify a global function reference
   /// for the current subtarget.

llvm/trunk/lib/Target/X86/X86Subtarget.cpp

if (isTargetDarwin()) {
       // If symbol visibility is hidden, the extra load is not needed if
       // target is x86-64 or the symbol is definitely defined in the current
       // translation unit.
       if (GV->hasDefaultVisibility() && !isDef)
         return X86II::MO_GOTPCREL;
     } else if (!isTargetWin64()) {
       assert(isTargetELF() && "Unknown rip-relative target");
 
-      // Extra load is needed for all externally visible globals.
-      if (!GV->hasLocalLinkage() && GV->hasDefaultVisibility())
+      // Extra load is needed for all externally visible globals except with
+      // PIE as the definition of the global in an executable is not
+      // overridden.
+
+      if (!GV->hasLocalLinkage() && GV->hasDefaultVisibility() &&
+          !isGlobalDefinedInPIE(GV, TM))
         return X86II::MO_GOTPCREL;
     }
 
     return X86II::MO_NO_FLAG;
   }
 
   if (isPICStyleGOT()) {   // 32-bit ELF targets.
-    // Extra load is needed for all externally visible.
-    if (GV->hasLocalLinkage() || GV->hasHiddenVisibility())
+    // Extra load is needed for all externally visible globals except with
+    // PIE as the definition of the global in an executable is not overridden.
+
+    if (GV->hasLocalLinkage() || GV->hasHiddenVisibility() ||
+        isGlobalDefinedInPIE(GV, TM))
       return X86II::MO_GOTOFF;
     return X86II::MO_GOT;
   }
 
   if (isPICStyleStubPIC()) {  // Darwin/32 in PIC mode.
     // Determine whether we have a stub reference and/or whether the reference
     // is relative to the PIC base or not.
 

llvm/trunk/test/CodeGen/X86/emutls-pie.ll

entry:
   ret i32 %1
 }
 
 @i = thread_local global i32 15
 @i2 = external thread_local global i32
 
 define i32 @f1() {
 ; X32-LABEL: f1:
-; X32:      movl __emutls_v.i@GOT(%ebx), %eax
+; X32:      leal __emutls_v.i@GOTOFF(%ebx), %eax
 ; X32-NEXT: movl %eax, (%esp)
 ; X32-NEXT: calll __emutls_get_address@PLT
 ; X32-NEXT: movl (%eax), %eax
 ; X32-NEXT: addl $8, %esp
 ; X32-NEXT: popl %ebx
 ; X32-NEXT: retl
 ; X64-LABEL: f1:
-; X64:      movq __emutls_v.i@GOTPCREL(%rip), %rdi
+; X64:      leaq __emutls_v.i(%rip), %rdi
 ; X64-NEXT: callq __emutls_get_address@PLT
 ; X64-NEXT: movl (%rax), %eax
 ; X64-NEXT: popq %rcx
 ; X64-NEXT: retq
 
 entry:
   %tmp1 = load i32, i32* @i
   ret i32 %tmp1
 }
 
 define i32* @f2() {
 ; X32-LABEL: f2:
-; X32:      movl __emutls_v.i@GOT(%ebx), %eax
+; X32:      leal __emutls_v.i@GOTOFF(%ebx), %eax
 ; X32-NEXT: movl %eax, (%esp)
 ; X32-NEXT: calll __emutls_get_address@PLT
 ; X64-LABEL: f2:
-; X64:      movq __emutls_v.i@GOTPCREL(%rip), %rdi
+; X64:      leaq __emutls_v.i(%rip), %rdi
 ; X64-NEXT: callq __emutls_get_address@PLT
 
 entry:
   ret i32* @i
 }
 
 define i32 @f3() {
 ; X32-LABEL: f3:

llvm/trunk/test/CodeGen/X86/global-access-pie.ll

+; RUN: llc < %s -march=x86-64 -mcpu=generic -mtriple=x86_64-linux-gnu -relocation-model=pic -enable-pie \
+; RUN:   | FileCheck -check-prefix=X64 %s
+; RUN: llc < %s -emulated-tls -march=x86 -mcpu=generic -mtriple=i386-linux-gnu -relocation-model=pic -enable-pie \
+; RUN:   | FileCheck -check-prefix=X32 %s
+
+; External Linkage
+@a = global i32 0, align 4
+
+define i32 @my_access_global_a() #0 {
+; X32-LABEL: my_access_global_a:
+; X32:       addl $_GLOBAL_OFFSET_TABLE_{{.*}}, %eax
+; X32-NEXT:  movl a@GOTOFF(%eax), %eax
+; X64-LABEL: my_access_global_a:
+; X64:       movl a(%rip), %eax
+
+entry:
+  %0 = load i32, i32* @a, align 4
+  ret i32 %0
+}
+
+; WeakAny Linkage
+@b = weak global i32 0, align 4
+
+define i32 @my_access_global_b() #0 {
+; X32-LABEL: my_access_global_b:
+; X32:       addl $_GLOBAL_OFFSET_TABLE_{{.*}}, %eax
+; X32-NEXT:  movl b@GOTOFF(%eax), %eax
+; X64-LABEL: my_access_global_b:
+; X64:       movl b(%rip), %eax
+
+entry:
+  %0 = load i32, i32* @b, align 4
+  ret i32 %0
+}
+
+; Internal Linkage
+@c = internal global i32 0, align 4
+
+define i32 @my_access_global_c() #0 {
+; X32-LABEL: my_access_global_c:
+; X32:       addl $_GLOBAL_OFFSET_TABLE_{{.*}}, %eax
+; X32-NEXT:  movl c@GOTOFF(%eax), %eax
+; X64-LABEL: my_access_global_c:
+; X64:       movl c(%rip), %eax
+
+entry:
+  %0 = load i32, i32* @c, align 4
+  ret i32 %0
+}
+
+; External Linkage, only declaration.
+@d = external global i32, align 4
+
+define i32 @my_access_global_load_d() #0 {
+; X32-LABEL: my_access_global_load_d:
+; X32:       addl $_GLOBAL_OFFSET_TABLE_{{.*}}, %eax
+; X32-NEXT:  movl d@GOT(%eax), %eax
+; X32-NEXT:  movl (%eax), %eax
+; X64-LABEL: my_access_global_load_d:
+; X64:       movq d@GOTPCREL(%rip), %rax
+; X64-NEXT:  movl (%rax), %eax
+
+entry:
+  %0 = load i32, i32* @d, align 4
+  ret i32 %0
+}
+
+; External Linkage, only declaration, store a value.
+
+define i32 @my_access_global_store_d() #0 {
+; X32-LABEL: my_access_global_store_d:
+; X32:       addl $_GLOBAL_OFFSET_TABLE_{{.*}}, %eax
+; X32-NEXT:  movl d@GOT(%eax), %eax
+; X32-NEXT:  movl $2, (%eax)
+; X64-LABEL: my_access_global_store_d:
+; X64:       movq d@GOTPCREL(%rip), %rax
+; X64-NEXT:  movl $2, (%rax)
+
+entry:
+  store i32 2, i32* @d, align 4
+  ret i32 0
+}
+
+; External Linkage, function pointer access.
+declare i32 @access_fp(i32 ()*)
+declare i32 @foo()
+
+define i32 @my_access_fp_foo() #0 {
+; X32-LABEL: my_access_fp_foo:
+; X32:       addl $_GLOBAL_OFFSET_TABLE_{{.*}}, %ebx
+; X32-NEXT:  movl foo@GOT(%ebx), %eax
+; X64-LABEL: my_access_fp_foo:
+; X64:       movq foo@GOTPCREL(%rip), %rdi
+
+entry:
+  %call = call i32 @access_fp(i32 ()* @foo)
+  ret i32 %call
+}
+
+; LinkOnceODR Linkage, function pointer access.
+
+$bar = comdat any
+
+define linkonce_odr i32 @bar() comdat {
+entry:
+  ret i32 0
+}
+
+define i32 @my_access_fp_bar() #0 {
+; X32-LABEL: my_access_fp_bar:
+; X32:       addl $_GLOBAL_OFFSET_TABLE_{{.*}}, %ebx
+; X32-NEXT:  leal bar@GOTOFF(%ebx), %eax
+; X64-LABEL: my_access_fp_bar:
+; X64:       leaq bar(%rip), %rdi
+
+entry:
+  %call = call i32 @access_fp(i32 ()* @bar)
+  ret i32 %call
+}