[PATCH] D19040: Remove unnecessary load via GOT when accessing globals with PIE in x86_64

[Sriraman Tallam via llvm-commits]

tmsriram added inline comments.

================
Comment at: lib/Target/X86/X86Subtarget.cpp:91
@@ +90,3 @@
+      bool isPIEDefinition =
+          GV->hasExactDefinition() && TM.Options.PositionIndependentExecutable;
+
----------------
AsafBadouh wrote:
> rnk wrote:
> > 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()
> 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))
> 
> ```
> 
> 
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.


http://reviews.llvm.org/D19040