[PowerPC] fix a bug in TCO eligibility check

[inouehrs]

Summary

If the callee and caller use different calling convensions, we cannot apply TCO if the callee requires arguments on stack; e.g. C calling convention and Fast CC use the same registers for parameter passing, but the stack offset is not necessarily same.

Due to this problem, Brotli failed in make test on PowerPC.

This patch also recommit r319218 "[PowerPC] Allow tail calls of fastcc functions from C CallingConv functions." since the problem reported in r320106 should be fixed.

[sfertile]

Looks good to me.

test/CodeGen/PowerPC/ppc64-sibcall.ll

44

We should add internal linkage here so the stack slot usage is the only thing blocking the tail-call.

[inouehrs]

addressed a comment from @sfertile

[echristo]

Can you recombine this fix in with the patch that was reverted in r319218?

Thanks!

[inouehrs]

combined with r319218 "[PowerPC] Allow tail calls of fastcc functions from C CallingConv functions."

[echristo]

In D40893#950338, @inouehrs wrote:

combined with r319218 "[PowerPC] Allow tail calls of fastcc functions from C CallingConv functions."

Can you also update the patch description? One inline request as well. You can also handle the inline request as a separate commit.

Thanks!

-eric

lib/Target/PowerPC/PPCISelLowering.cpp

4442

Since you're here can you update this with the actual problem and not an external link? We don't really do that.

[inouehrs]

Can you also update the patch description? One inline request as well. You can also handle the inline request as a separate commit.

I updated the description.

About the URL in the comment, it is an example of a case where GCC can do sibling call optimization even with a byval parameter rather tan an additional explanation on why this is a workaround; the example is too large to embed in a comment.
How about changing See: https://... to GCC can do such sibling call optimization such as https://... or just remove See: https://...? Any preference?
Anyway I like to touch up this comment later in another patch.

[hfinkel]

In D40893#952121, @inouehrs wrote:

Can you also update the patch description? One inline request as well. You can also handle the inline request as a separate commit.

I updated the description.

About the URL in the comment, it is an example of a case where GCC can do sibling call optimization even with a byval parameter rather tan an additional explanation on why this is a workaround; the example is too large to embed in a comment.
How about changing See: https://... to GCC can do such sibling call optimization such as https://... or just remove See: https://...? Any preference?
Anyway I like to touch up this comment later in another patch.

What value is the link adding? Is it just the test case? If so:

1. The test case is actually short enough for a comment, and
2. You could generate IR from that test case and add it (with a FIXME note in the test case that we can do better).

lib/Target/PowerPC/PPCISelLowering.cpp

4400–4404

tail -> Tail

[jtony]

LGTM.

lib/Target/PowerPC/PPCISelLowering.cpp

4408

then->than

[inouehrs]

updated comments

[inouehrs]

lib/Target/PowerPC/PPCISelLowering.cpp

4442

How about this? I included the example in the URL here.

[nemanjai]

LGTM

test/CodeGen/PowerPC/ppc64-sibcall.ll

44

We aren't super strict about formatting in test cases, but please move the right brace to the next line.

45

Please add a comment as to why we don't want to tail call here.

lib/Target/PowerPC/PPCISelLowering.cpp

hasSameArgumentList(const Function *CallerFn, ImmutableCallSite CS) {
   return true;
 }
 
 // Returns true if TCO is possible between the callers and callees
 // calling conventions.
 static bool
 areCallingConvEligibleForTCO_64SVR4(CallingConv::ID CallerCC,
                                     CallingConv::ID CalleeCC) {
   // tail calls are possible with fastcc and ccc.
   auto isTailCallableCC  = [] (CallingConv::ID CC){
       return  CC == CallingConv::C || CC == CallingConv::Fast;
   };
   if (!isTailCallableCC(CallerCC) || !isTailCallableCC(CalleeCC))
     return false;
 
   // We can safely tail call both fastcc and ccc callees from a c calling
   // convention caller. If the caller is fastcc, we may have less stack space
   // then a non-fastcc caller with the same signature so disable tail-calls in
   // that case.
   return CallerCC == CallingConv::C || CallerCC == CalleeCC;
 }
 
 bool
 PPCTargetLowering::IsEligibleForTailCallOptimization_64SVR4(
                                     SDValue Callee,
PPCTargetLowering::IsEligibleForTailCallOptimization_64SVR4(
 
   // Caller contains any byval parameter is not supported.
   if (any_of(Ins, [](const ISD::InputArg &IA) { return IA.Flags.isByVal(); }))
     return false;
 
   // Callee contains any byval parameter is not supported, too.
   // Note: This is a quick work around, because in some cases, e.g.
   // caller's stack size > callee's stack size, we are still able to apply
   // sibling call optimization. See: https://reviews.llvm.org/D23441#513574
   if (any_of(Outs, [](const ISD::OutputArg& OA) { return OA.Flags.isByVal(); }))
     return false;
 
+  // If callee and caller use different calling conventions, we cannot pass
+  // parameters on stack since offsets for the parameter area may be different.
+  if (Caller->getCallingConv() != CalleeCC &&
+      needStackSlotPassParameters(Subtarget, Outs))
+    return false;
+
   // No TCO/SCO on indirect call because Caller have to restore its TOC
   if (!isFunctionGlobalAddress(Callee) &&
       !isa<ExternalSymbolSDNode>(Callee))
     return false;
 
   // If the caller and callee potentially have different TOC bases then we
   // cannot tail call since we need to restore the TOC pointer after the call.
   // ref: https://bugzilla.mozilla.org/show_bug.cgi?id=973977

test/CodeGen/PowerPC/ppc64-sibcall.ll

 define void @caller_64_64_copy([8 x i64] %a, [8 x i64] %b) #1 {
   tail call void @callee_64_64_copy([8 x i64] %a, [8 x i64] %b)
   ret void
 
 ; CHECK-SCO-LABEL: caller_64_64_copy:
 ; CHECK-SCO: b callee_64_64_copy
 }
 
+define fastcc void @callee_64_64_copy_fastcc([8 x i64] %a, [8 x i64] %b) #0 { ret void }
+define void @caller_64_64_copy_ccc([8 x i64] %a, [8 x i64] %b) #1 {
+  tail call fastcc void @callee_64_64_copy_fastcc([8 x i64] %a, [8 x i64] %b)
+  ret void
+
+; CHECK-SCO-LABEL: caller_64_64_copy_ccc:
+; CHECK-SCO: bl callee_64_64_copy_fastcc
+}
+
 define void @caller_64_64_reorder_copy([8 x i64] %a, [8 x i64] %b) #1 {
   tail call void @callee_64_64_copy([8 x i64] %b, [8 x i64] %a)
   ret void
 
 ; CHECK-SCO-LABEL: caller_64_64_reorder_copy:
 ; CHECK-SCO: bl callee_64_64_copy
 }