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

2763

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

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

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.

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;
 
+  // Cannot jump to address through GOT, see https://llvm.org/bugs/show_bug.cgi?id=15086
+  if (Subtarget->isPICStyleGOT())
+    isTailCall = false;
+
   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.
+    // around. User should be aware of lazy dynamic linking issue in bug 15086.
     isTailCall = true;
   } else if (isTailCall) {
     // Check if it's really possible to do a tail call.
     isTailCall = IsEligibleForTailCallOptimization(Callee, CallConv,
                     isVarArg, SR != NotStructReturn,
                     MF.getFunction()->hasStructRetAttr(), CLI.RetTy,
                     Outs, OutVals, Ins, DAG);
 
X86TargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
 
   if (Subtarget->isPICStyleGOT()) {
     // ELF / PIC requires GOT in the EBX register before function calls via PLT
     // GOT pointer.
     if (!isTailCall) {
       RegsToPass.push_back(std::make_pair(unsigned(X86::EBX),
                DAG.getNode(X86ISD::GlobalBaseReg, SDLoc(), getPointerTy())));
     } else {
       // If we are tail calling and generating PIC/GOT style code load the
       // address of the callee into ECX. The value in ecx is used as target of
       // 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.

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(...)

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

 ; RUN: llc < %s -relocation-model=pic -mattr=+sse2 | FileCheck %s
 
 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

test/CodeGen/X86/tailcallpic1.ll

 ; RUN: llc < %s  -tailcallopt -mtriple=i686-pc-linux-gnu -relocation-model=pic | FileCheck %s
 
 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
+; CHECK: calll tailcallee
 }

test/CodeGen/X86/tailcallpic2.ll

 ; RUN: llc < %s  -tailcallopt -mtriple=i686-pc-linux-gnu -relocation-model=pic | FileCheck %s
 
 define 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: movl tailcallee@GOT
-; CHECK: jmpl
+; CHECK: calll tailcallee@PLT
 }