Disable x86 tail call optimizations that jump through GOT

[chh]

Summary

For x86 targets, when in PIC mode, do not optimize tail calls.

[chh]

ping.

[rnk]

lib/Target/X86/X86ISelLowering.cpp

2759 ↗

(On Diff #25891)

We can't do this, musttail guarantees that a tail call will occur. I think the old behavior of forcing relocations to be resolved eagerly was preferable. If we cannot honor musttail, we must report an error somehow.

[chh]

Okay, I removed the change to isMustTailCall in the new diff.

[rnk]

You removed the musttail call change, but also removed the supporting code that arranges to jump through ECX, so I don't think this will work either.

I commented on the bug, I'm trying to understand why the GOT reference causes a problem. I don't yet see a codegen bug here.

[chh]

The purpose of this change is to remove the jump (through ECX or not) completely since the use of tailcallee@GOT is incorrect under lazy dynamic linking.

[davidxl]

lib/Target/X86/X86ISelLowering.cpp

2929 ↗

(On Diff #26338)

It is a valid optimization with eager binding, so it seems the behavior should be controlled by an option.

[chh]

lib/Target/X86/X86ISelLowering.cpp

2929 ↗

(On Diff #26338)

David, I suppose you prefer the default to have tail call optimization.
Then, we don't need to change llvm.
We can use -fno-optimize-sibling-calls now to turn off tail call optimization.
Or we can add another -fno-optimize-tail-calls.

I would rather llvm for x86 to play safe as gcc, with no tail call optimization by default.
Then we could have another option like -foptimize-tail-calls.

On the other hand, I think I should not remove line 2928 to 2943,
for they might be needed for IsMustTail functions.

[chh]

Keep IsMustTail working as before.

[davidxl]

I am still not convinced this is a compiler bug. See my comment in the bug.

[joerg]

This code really should be under an option. As mentioned in the original PR, the origin of the problem is the misdesigned XFree86/Xorg driver interface. We should not pessimise everyone who doesn't create brain dead code. The executive summary of the X bug is:
(1) A module is loaded with a reference to foo. foo is not yet defined.
(2) Another module is lodead that defines foo. X requires the first module to call this.

Ignoring must-tailcall would break all languages that require the optimisation.

[chh]

I read the new discussions in https://llvm.org/bugs/show_bug.cgi?id=15086.

• If it is not considered an optimization bug, we need to change semantics of dlopen() with RTLD_LAZY.
• A fix to have both tail call optimization and RTLD_LAZY seems infeasible, to setup %ebx before and restore %ebx after the call.
• Both X.org and Android hit this problem. I have spent weeks to trace down this problem in Android and hence my motivation to change llvm and save future debug time.
• This problem has been worked around with a global -fno-optimize-sibling-calls flag. So it is not urgent to have a fix. But that flag also disables other valid cases such as non-PIC mode.

In my new diff3, I keep current isMustTail feature and disable isTailCall only under PIC mode.
That should not lose performance compared with gcc.

If we want to keep this optimization selectable under PIC mode,
then we need a new flag and the question is whether the optimization is on or off
under PIC mode by default. Considering the difficulty of debugging this problem
and current competition from gcc, I would rather the default to be off.

Suggestions?

[davidxl]

Changing the default may affect users who depend on the current behavior which has been the case for a couple of releases. Another possible solution is to emit a warning for cases like this so that debugging similar issues in the future will become easier.

[rnk]

I'm coming around to the position that we should simply disable this
optimization for PIC calls to symbols with default visibility. The current
change disables all tail calls when generating PIC code, which is
undesirable.

We can add a flag later if someone asks, and call it
-f[no-]optimize-pic-sibling-calls.

[chh]

Please review my new diff 4. This should allow tail call optimizations for protected and hidden symbols.
AFAIK, jump to those symbols do not use GOT.
On the other hand, I found some oddity about "sin" and "sin2" symbols in one test case,
which made me to use the strange condition:

if (!(G && (.... || ...))) isTailCall = false;

[rnk]

Thanks for the patch! I simplified the conditional a bit and tweaked the tests to try to cover some more corner cases that I could think of. During testing I found that this disabled tail calling of internal (think static in C) functions, so I added hasLocalLinkage() to the conditional. I committed the result as r238487.

[chh]

Reid, thank you very much for the review, advices, and clean up of this patch.

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

X86TargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
   bool IsWin64        = Subtarget->isCallingConvWin64(CallConv);
   StructReturnType SR = callIsStructReturn(Outs);
   bool IsSibcall      = false;
   X86MachineFunctionInfo *X86Info = MF.getInfo<X86MachineFunctionInfo>();
 
   if (MF.getTarget().Options.DisableTailCalls)
     isTailCall = false;
 
+  if (Subtarget->isPICStyleGOT() &&
+      !MF.getTarget().Options.GuaranteedTailCallOpt) {
+    // If we are using a GOT, disable tail calls to external symbols with
+    // default visibility. Tail calling such a symbol requires using a GOT
+    // relocation, which forces early binding of the symbol. This breaks code
+    // that require lazy function symbol resolution. Using musttail or
+    // GuaranteedTailCallOpt will override this.
+    GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee);
+    if (!G || (!G->getGlobal()->hasLocalLinkage() &&
+               G->getGlobal()->hasDefaultVisibility())) {
+      isTailCall = false;
+      if (G) {
+        llvm::errs() << "disabling tail call for default visibility symbol\n";
+        G->getGlobal()->dump();
+      }
+    }
+  }
+
   bool IsMustTail = CLI.CS && CLI.CS->isMustTailCall();
   if (IsMustTail) {
     // Force this to be a tail call.  The verifier rules are enough to ensure
     // that we can lower this successfully without moving the return address
     // around.
     isTailCall = true;
   } else if (isTailCall) {
     // Check if it's really possible to do a tail call.
if (!isTailCall) {
       // the tail jump. This is done to circumvent the ebx/callee-saved problem
       // for tail calls on PIC/GOT architectures. Normally we would just put the
       // address of GOT into ebx and then call target@PLT. But for tail calls
       // ebx would be restored (since ebx is callee saved) before jumping to the
       // target@PLT.
 
       // Note: The actual moving to ECX is done further down.
       GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee);
-      if (G && !G->getGlobal()->hasHiddenVisibility() &&
-          !G->getGlobal()->hasProtectedVisibility())
+      if (G && !G->getGlobal()->hasLocalLinkage() &&
+          G->getGlobal()->hasDefaultVisibility())
         Callee = LowerGlobalAddress(Callee, DAG);
       else if (isa<ExternalSymbolSDNode>(Callee))
         Callee = LowerExternalSymbol(Callee, DAG);
     }
   }
 
   if (Is64Bit && isVarArg && !IsWin64 && !IsMustTail) {
     // From AMD64 ABI document:

llvm/trunk/test/CodeGen/X86/pic.ll

 ; LINUX:   addl $_GLOBAL_OFFSET_TABLE_+(.L{{.*}}-.L7$pb),
 ; LINUX:   .LJTI7_0@GOTOFF(
 ; LINUX:   jmpl *
 
 ; LINUX: .align 4
 ; LINUX-NEXT: .LJTI7_0:
 ; LINUX:   .long   .LBB7_2@GOTOFF
 ; LINUX:   .long   .LBB7_8@GOTOFF
-; LINUX:   .long   .LBB7_14@GOTOFF
-; LINUX:   .long   .LBB7_9@GOTOFF
-; LINUX:   .long   .LBB7_10@GOTOFF
+; LINUX:   .long   .LBB7_4@GOTOFF
+; LINUX:   .long   .LBB7_6@GOTOFF
+; LINUX:   .long   .LBB7_5@GOTOFF
+; LINUX:   .long   .LBB7_8@GOTOFF
+; LINUX:   .long   .LBB7_7@GOTOFF
 }
 
 declare void @foo1(...)
 declare void @foo2(...)
 declare void @foo6(...)
 declare void @foo3(...)
 declare void @foo4(...)
 declare void @foo5(...)

llvm/trunk/test/CodeGen/X86/tail-call-got.ll

 ; RUN: llc < %s -relocation-model=pic -mattr=+sse2 | FileCheck %s
 
+; We used to do tail calls through the GOT for these symbols, but it was
+; disabled due to PR15086.
+
 target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:32:32-n8:16:32"
 target triple = "i386-unknown-freebsd9.0"
 
 define double @test1(double %x) nounwind readnone {
 ; CHECK-LABEL: test1:
-; CHECK: movl foo@GOT
-; CHECK-NEXT: jmpl
+; CHECK:  calll foo@PLT
   %1 = tail call double @foo(double %x) nounwind readnone
   ret double %1
 }
 
 declare double @foo(double) readnone
 
 define double @test2(double %x) nounwind readnone {
 ; CHECK-LABEL: test2:
-; CHECK: movl sin@GOT
-; CHECK-NEXT: jmpl
+; CHECK:  calll sin@PLT
   %1 = tail call double @sin(double %x) nounwind readnone
   ret double %1
 }
 
 declare double @sin(double) readnone
+
+define double @test3(double %x) nounwind readnone {
+; CHECK-LABEL: test3:
+; CHECK:  calll sin2@PLT
+  %1 = tail call double @sin2(double %x) nounwind readnone
+  ret double %1
+}
+
+declare double @sin2(double) readnone

llvm/trunk/test/CodeGen/X86/tailcallpic1.ll

 ; RUN: llc < %s  -tailcallopt -mtriple=i686-pc-linux-gnu -relocation-model=pic | FileCheck %s
 
+; This test uses guaranteed TCO so these will be tail calls, despite the early
+; binding issues.
+
 define protected fastcc i32 @tailcallee(i32 %a1, i32 %a2, i32 %a3, i32 %a4) {
 entry:
   ret i32 %a3
 }
 
 define fastcc i32 @tailcaller(i32 %in1, i32 %in2) {
 entry:
   %tmp11 = tail call fastcc i32 @tailcallee( i32 %in1, i32 %in2, i32 %in1, i32 %in2 )    ; <i32> [#uses=1]
   ret i32 %tmp11
 ; CHECK: jmp tailcallee
 }

llvm/trunk/test/CodeGen/X86/tailcallpic3.ll

+; RUN: llc < %s -mtriple=i686-pc-linux-gnu -relocation-model=pic | FileCheck %s
+
+; While many of these could be tail called, we don't do it because it forces
+; early binding.
+
+declare void @external()
+
+define hidden void @tailcallee_hidden() {
+entry:
+  ret void
+}
+
+define void @tailcall_hidden() {
+entry:
+  tail call void @tailcallee_hidden()
+  ret void
+}
+; CHECK: tailcall_hidden:
+; CHECK: jmp tailcallee_hidden
+
+define internal void @tailcallee_internal() {
+entry:
+  ret void
+}
+
+define void @tailcall_internal() {
+entry:
+  tail call void @tailcallee_internal()
+  ret void
+}
+; CHECK: tailcall_internal:
+; CHECK: jmp tailcallee_internal
+
+define default void @tailcallee_default() {
+entry:
+  ret void
+}
+
+define void @tailcall_default() {
+entry:
+  tail call void @tailcallee_default()
+  ret void
+}
+; CHECK: tailcall_default:
+; CHECK: calll tailcallee_default@PLT
+
+define void @tailcallee_default_implicit() {
+entry:
+  ret void
+}
+
+define void @tailcall_default_implicit() {
+entry:
+  tail call void @tailcallee_default_implicit()
+  ret void
+}
+; CHECK: tailcall_default_implicit:
+; CHECK: calll tailcallee_default_implicit@PLT
+
+define void @tailcall_external() {
+  tail call void @external()
+  ret void
+}
+; CHECK: tailcall_external:
+; CHECK: calll external@PLT
+
+define void @musttail_external() {
+  musttail call void @external()
+  ret void
+}
+; CHECK: musttail_external:
+; CHECK: movl external@GOT
+; CHECK: jmpl