[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 ↗

(On Diff #125711)

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 ↗

(On Diff #126264)

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 ↗

(On Diff #126264)

tail -> Tail

[jtony]

LGTM.

lib/Target/PowerPC/PPCISelLowering.cpp

4408 ↗

(On Diff #126264)

then->than

[inouehrs]

updated comments

[inouehrs]

lib/Target/PowerPC/PPCISelLowering.cpp

4442 ↗

(On Diff #126264)

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

[nemanjai]

LGTM

test/CodeGen/PowerPC/ppc64-sibcall.ll

44 ↗

(On Diff #128003)

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

45 ↗

(On Diff #128003)

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

llvm/trunk/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 or Sibling call optimization (TCO/SCO) needs callee and caller to
-  // have the same calling convention.
-  if (CallerCC != 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;
 
-  // Tail or Sibling calls can be done with fastcc/ccc.
-  return (CallerCC == CallingConv::Fast || CallerCC == CallingConv::C);
+  // 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
+  // than 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,
                                     CallingConv::ID CalleeCC,
                                     ImmutableCallSite CS,
                                     bool isVarArg,
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
+  // sibling call optimization. For example, gcc is able to do SCO for caller1
+  // in the following example, but not for caller2.
+  //   struct test {
+  //     long int a;
+  //     char ary[56];
+  //   } gTest;
+  //   __attribute__((noinline)) int callee(struct test v, struct test *b) {
+  //     b->a = v.a;
+  //     return 0;
+  //   }
+  //   void caller1(struct test a, struct test c, struct test *b) {
+  //     callee(gTest, b); }
+  //   void caller2(struct test *b) { callee(gTest, b); }
   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

llvm/trunk/test/CodeGen/PowerPC/duplicate-returns-for-tailcall.ll

 
 if.end4:                                          ; preds = %if.end
   %cmp5 = icmp sgt i32 %b, %c
   br i1 %cmp5, label %if.then6, label %return
 
 if.then6:                                         ; preds = %if.end4
   %call7 = tail call fastcc signext i32 @call3(i32 signext %a, i32 signext %b, i32 signext %c)
   br label %return
-; No duplication here because the calling convention mismatch means we won't tail-call
+; tail calling a fastcc function from a ccc function is supported.
 ; CHECK_LABEL: if.then13:
-; CHECK:       tail call fastcc signext i32 @call3
-; CHECK-NEXT:  br
+; CHECK:       %[[T2:[a-zA-Z0-9]+]] = tail call fastcc signext i32 @call3
+; CHECK-NEXT:  ret i32 %[[T2]]
 
 return:                                           ; preds = %if.end4, %if.then6, %if.then2, %if.then
   %retval.0 = phi i32 [ %call, %if.then ], [ %call3, %if.then2 ], [ %call7, %if.then6 ], [ %c, %if.end4 ]
   ret i32 %retval.0
 }
 
 declare signext i32 @call2(i32 signext, i32 signext, i32 signext) local_unnamed_addr #2
 
 ; Function Attrs: noinline norecurse nounwind readnone
 define internal fastcc signext i32 @call3(i32 signext %a, i32 signext %b, i32 signext %c) unnamed_addr #0 {
 entry:
   %mul = mul nsw i32 %b, %a
   %mul1 = mul nsw i32 %mul, %c
   ret i32 %mul1
 }

llvm/trunk/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 internal 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
+; If caller and callee use different calling convensions, we cannot apply TCO.
+; 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
 }