Enable sibling-call optimization for functions returning structs

[xbolva00]

Summary

This is a attempt for fix issue PR28417.

Currently clang doesn't do sibling call optimization when function returns a struct by value:

mytype f();

mytype g()
{

return f();

}
Generated IR (-O2):

define void @g()(%struct.mytype* noalias sret) local_unnamed_addr #0 !dbg !7 {

tail call void @f()(%struct.mytype* sret %0), !dbg !21
ret void, !dbg !22

}
Generated code:

g(): # @g()

push rbx
mov rbx, rdi
call f()
mov rax, rbx
pop rbx
ret

On the other hand clang can do sibling call optimization when struct is passed by pointer:

struct mytype
{

char const *a, *b, *c, *d;

};

void f(mytype*);

void g(mytype* a)
{

return f(a);

}
Generated IR (-O2):

define void @g(mytype*)(%struct.mytype*) local_unnamed_addr #0 !dbg !7 {

call void @llvm.dbg.value(metadata %struct.mytype* %0, metadata !13, metadata !DIExpression()), !dbg !14
tail call void @f(mytype*)(%struct.mytype* %0), !dbg !15
ret void, !dbg !16

}
Generated code:

g(mytype*): # @g(mytype*)

jmp f(mytype*) # TAILCALL

The difference between these two IRs are the presence of sret attribute. I believe tail call optimization is possible in the first case too. The reason why tail call optimization is not performed is that X86TargetLowering::IsEligibleForTailCallOptimization has the following if:

Also avoid sibcall optimization if either caller or callee uses struct
return semantics.
if (isCalleeStructRet || isCallerStructRet)

return false;

Unfortunately when this if was added, no explanation was given why it is needed here. The corresponding test case is marked rdar://7726868, but I can not see what it is about.

As far as I understand sibling call can be performed when both caller and callee are marked sret. The case when caller and callee have mismatched sret specification is trickier.

As I understand attribute sret serves two purposes:

(from documentation) This indicates that the pointer parameter specifies the address of a structure that is the return value of the function in the source program. This pointer must be guaranteed by the caller to be valid: loads and stores to the structure may be assumed by the callee not to trap and to be properly aligned. This is not a valid attribute for return values.
(from Itanium ABI) callee function should return its sret argument in RAX.
From (2) sret caller can not sibling-call non-sret callee. The reverse is allowed. Based on this I updated X86TargetLowering::IsEligibleForTailCallOptimization.

Base patch by https://reviews.llvm.org/D45653

[spatel]

Please abandon this. The changes should be made in the original patch by the original author. If the original author has agreed to let you commandeer D45653, then that should be stated there, and the review should continue there.

[xbolva00]

In D46262#1082740, @spatel wrote:

Please abandon this. The changes should be made in the original patch by the original author. If the original author has agreed to let you commandeer D45653, then that should be stated there, and the review should continue there.

Ok, I understand.

[xbolva00]

Reopened, since original D45653 was abandoned.

[xbolva00]

Revert deleted testcase.

[xbolva00]

One more test added.

[efriedma]

Please fix the "summary" to include the full expected commit message.

I had the following comment on the original revision:

If both the caller and callee are sret, do you need to check that the sret argument is the same? Consider something like tail call void @f(%struct.foo* noalias sret @aa) nounwind.

Did this ever get fixed?

[efriedma]

Also, it looks like you never cc'ed llvm-commits; please abandon this and post a new revision with llvm-commits properly cc'ed

[xbolva00]

In D46262#1542590, @efriedma wrote:

Please fix the "summary" to include the full expected commit message.

I had the following comment on the original revision:

If both the caller and callee are sret, do you need to check that the sret argument is the same? Consider something like tail call void @f(%struct.foo* noalias sret @aa) nounwind.

Did this ever get fixed?

t21_sret_to_sret_second_arg_sret or maybe t21_sret_to_sret_alloca ?

How do you propose to create test case like you want

define fastcc void @t21_sret_to_sret_more_args2(%struct.foo* noalias sret %agg.result) nounwind  {
   tail call fastcc void @t21_f_sret(%struct.foo* noalias sret %aa) nounwind
   ret void
}

?

[xbolva00]

Ping

[arsenm]

lib/Target/X86/X86ISelLowering.cpp

4164–4166

No else after return

[xbolva00]

Addressed review notes, thanks.

[efriedma]

The testcase I actually wanted, which incorrectly forms a tail call:

define fastcc void @t21_sret_to_sret_arg_mismatch(%struct.foo* noalias sret %agg.result, %struct.foo* noalias %a) nounwind  {
  %b = call fastcc %struct.foo* @ret_struct()
  tail call fastcc void @t21_f_sret(%struct.foo* noalias sret %a, %struct.foo* noalias %b) nounwind
  ret void
}
declare ccc %struct.foo* @ret_struct() nounwind
declare fastcc void @t21_f_sret(%struct.foo* noalias sret, %struct.foo* noalias) nounwind
%struct.foo = type { [4 x i32] }

Apparently other, more obvious constructs don't actually trigger the issue; there was a check introduced in https://reviews.llvm.org/D8510 which suppresses tail calls in some, but not all, of the relevant cases.

lib/Target/X86/X86ISelLowering.cpp

4161

Not sure why the CallerF.arg_size() != Outs.size() check is necessary? In any case, needs a comment to explain.

[xbolva00]

lib/Target/X86/X86ISelLowering.cpp

4161

Some "args_mismatch" test was miscompiled without it , I think.

lib/Target/X86/X86ISelLowering.cpp

bool X86TargetLowering::IsEligibleForTailCallOptimization(
   // require ABI changes. This is what gcc calls sibcall.
 
   // Can't do sibcall if stack needs to be dynamically re-aligned. PEI needs to
   // emit a special epilogue.
   const X86RegisterInfo *RegInfo = Subtarget.getRegisterInfo();
   if (RegInfo->needsStackRealignment(MF))
     return false;
 
-  // Also avoid sibcall optimization if either caller or callee uses struct
-  // return semantics.
-  if (isCalleeStructRet || isCallerStructRet)
+  // Struct-return functions need to return its argument in RAX, so they can not
+  // sibcall non-struct-return functions.
+  if (!isCalleeStructRet && isCallerStructRet)
     return false;
 
   // Do not sibcall optimize vararg calls unless all arguments are passed via
   // registers.
   LLVMContext &C = *DAG.getContext();
   if (isVarArg && !Outs.empty()) {
     // Optimizing for varargs on Win64 is unlikely to be safe without
     // additional testing.
     if (IsCalleeWin64 || IsCallerWin64)
       return false;
 
     SmallVector<CCValAssign, 16> ArgLocs;

test/CodeGen/X86/sibcall.ll

 ; X32-NEXT:    calll f
 ; X32-NEXT:    movl %esi, %eax
 ; X32-NEXT:    addl $8, %esp
 ; X32-NEXT:    popl %esi
 ; X32-NEXT:    retl $4
 ;
 ; X64-LABEL: t15:
 ; X64:       # %bb.0:
-; X64-NEXT:    pushq %rbx
-; X64-NEXT:    movq %rdi, %rbx
-; X64-NEXT:    callq f
-; X64-NEXT:    movq %rbx, %rax
-; X64-NEXT:    popq %rbx
-; X64-NEXT:    retq
+; X64-NEXT:    jmp f # TAILCALL
 ;
 ; X32ABI-LABEL: t15:
 ; X32ABI:       # %bb.0:
-; X32ABI-NEXT:    pushq %rbx
-; X32ABI-NEXT:    movl %edi, %ebx
-; X32ABI-NEXT:    callq f
-; X32ABI-NEXT:    movl %ebx, %eax
-; X32ABI-NEXT:    popq %rbx
-; X32ABI-NEXT:    retq
+; X32ABI-NEXT:    jmp f # TAILCALL
   tail call fastcc void @f(%struct.foo* noalias sret %agg.result) nounwind
   ret void
 }
 
 declare void @f(%struct.foo* noalias sret) nounwind
 
 define void @t16() nounwind ssp {
 ; X32-LABEL: t16:

 ; X32ABI:       # %bb.0: # %entry
 ; X32ABI-NEXT:    jmp foo20 # TAILCALL
 entry:
   %0 = tail call fastcc double @foo20(double %x) nounwind
   ret double %0
 }
 
 declare fastcc double @foo20(double) nounwind
+
+; bug 28417
+define fastcc void @t21_sret_to_sret(%struct.foo* noalias sret %agg.result) nounwind  {
+; 32-LABEL: t21_sret_to_sret:
+; 32: jmp {{_?}}t21_f_sret
+; 64-LABEL: t21_sret_to_sret:
+; 64: jmp {{_?}}t21_f_sret
+; X32-LABEL: t21_sret_to_sret:
+; X32:       # %bb.0:
+; X32-NEXT:    jmp t21_f_sret # TAILCALL
+;
+; X64-LABEL: t21_sret_to_sret:
+; X64:       # %bb.0:
+; X64-NEXT:    jmp t21_f_sret # TAILCALL
+;
+; X32ABI-LABEL: t21_sret_to_sret:
+; X32ABI:       # %bb.0:
+; X32ABI-NEXT:    jmp t21_f_sret # TAILCALL
+  tail call fastcc void @t21_f_sret(%struct.foo* noalias sret %agg.result) nounwind
+  ret void
+}
+
+define fastcc void @t21_sret_to_non_sret(%struct.foo* noalias sret %agg.result) nounwind  {
+; 32-LABEL: t21_sret_to_non_sret:
+; 32: calll {{_?}}t21_f_non_sret
+; 32: retl
+; 64-LABEL: t21_sret_to_non_sret:
+; 64: callq {{_?}}t21_f_non_sret
+; 64: retq
+; X32-LABEL: t21_sret_to_non_sret:
+; X32:       # %bb.0:
+; X32-NEXT:    pushl %esi
+; X32-NEXT:    subl $8, %esp
+; X32-NEXT:    movl %ecx, %esi
+; X32-NEXT:    calll t21_f_non_sret
+; X32-NEXT:    movl %esi, %eax
+; X32-NEXT:    addl $8, %esp
+; X32-NEXT:    popl %esi
+; X32-NEXT:    retl
+;
+; X64-LABEL: t21_sret_to_non_sret:
+; X64:       # %bb.0:
+; X64-NEXT:    pushq %rbx
+; X64-NEXT:    movq %rdi, %rbx
+; X64-NEXT:    callq t21_f_non_sret
+; X64-NEXT:    movq %rbx, %rax
+; X64-NEXT:    popq %rbx
+; X64-NEXT:    retq
+;
+; X32ABI-LABEL: t21_sret_to_non_sret:
+; X32ABI:       # %bb.0:
+; X32ABI-NEXT:    pushq %rbx
+; X32ABI-NEXT:    movl %edi, %ebx
+; X32ABI-NEXT:    callq t21_f_non_sret
+; X32ABI-NEXT:    movl %ebx, %eax
+; X32ABI-NEXT:    popq %rbx
+; X32ABI-NEXT:    retq
+  tail call fastcc void @t21_f_non_sret(%struct.foo* %agg.result) nounwind
+  ret void
+}
+
+define fastcc void @t21_non_sret_to_sret(%struct.foo* %agg.result) nounwind  {
+; 32-LABEL: t21_non_sret_to_sret:
+; 32: jmp {{_?}}t21_f_sret
+; 64-LABEL: t21_non_sret_to_sret:
+; 64: jmp {{_?}}t21_f_sret
+; X32-LABEL: t21_non_sret_to_sret:
+; X32:       # %bb.0:
+; X32-NEXT:    jmp t21_f_sret # TAILCALL
+;
+; X64-LABEL: t21_non_sret_to_sret:
+; X64:       # %bb.0:
+; X64-NEXT:    jmp t21_f_sret # TAILCALL
+;
+; X32ABI-LABEL: t21_non_sret_to_sret:
+; X32ABI:       # %bb.0:
+; X32ABI-NEXT:    jmp t21_f_sret # TAILCALL
+  tail call fastcc void @t21_f_sret(%struct.foo* noalias sret %agg.result) nounwind
+  ret void
+}
+
+declare fastcc void @t21_f_sret(%struct.foo* noalias sret) nounwind
+declare fastcc void @t21_f_non_sret(%struct.foo*) nounwind