diff mbox

Allow more PowerPC sibling calls

Message ID 20110409025146.GF19002@bubble.grove.modra.org
State New
Headers show

Commit Message

Alan Modra April 9, 2011, 2:51 a.m. UTC 
This patch enables sibling calls for powerpc in a few more cases, and
fixes bugs exposed by that change.  We now
a) Allow sibling calls via function pointer.  At the time
   rs6000_function_ok_for_sibcall was written, I don't think access to
   arg types of function pointer calls was available in the target
   hook/macro.
b) Allow sibling calls to functions with vector parameters.  Contrary
   to the previous comment about vector parameters, a problem occurs
   with VRSAVE only when the called function has more vector
   parameters than the caller (because VRSAVE bits will already be set
   for the caller's arguments).  Also, the old code did not make any
   attempt to determine whether altivec/vsx registers would actually
   be used to pass args.  Not all vector types fit in these regs.
c) Allow SYSV4 ABI code to make sibling calls to non-local functions
   so long as not both flag_pic and TARGET_SECURE_PLT are set.
   The PIC -msecure-plt call stubs use r30.

sibcall_value_nonlocal_sysv was trying to read the call cookie from
the wrong operand, a latent bug since the pattern wasn't used until
now.  I also fixed a minor problem with CALL_LIBCALL in the call
cookie, which could cause various call insns to match an "n"
constraint rather than a "0" constraint and so give the wrong insn
length.  Bootstrapped and regression tested powerpc64-linux and
powerpc-linux.

This patch does affect darwin, not only in removing what are now
redundant insn patterns, but also (a) and (b) above.  Would someone
please run a bootstrap and regression test on darwin for me?  I don't
have a machine handy to confirm that I didn't break anything there.

I've attached a vector testcase I used to verify some of these
changes.

gcc/
	* config/rs6000/rs6000.c (rs6000_function_arg): Remove CALL_LIBCALL
	when returning call_cookie.
	(rs6000_function_ok_for_sibcall): Allow sibcalls via function
	pointers, to functions with no more vector args than the current
	function, and some non-local calls for ABI_V4.
	* config/rs6000/rs6000.md (sibcall_nonlocal_aix32,
	sibcall_nonlocal_aix64): Combine to ..
	(sibcall_nonlocal_aix<mode>): ..this.  Handle function pointer calls.
	(sibcall_value_nonlocal_aix32, sibcall_value_nonlocal_aix64): Combine..
	(sibcall_value_nonlocal_aix<mode>): ..likewise.
	(*sibcall_nonlocal_sysv<mode>): Handle function pointer calls.
	(sibcall_value_nonlocal_sysv<mode>): Likewise.  Correct call cookie
	operand.
	* config/rs6000/darwin.md (sibcall_nonlocal_darwin64,
	sibcall_value_nonlocal_darwin64, sibcall_symbolic_64,
	sibcall_value_symbolic_64): Delete.
gcc/testsuite/
	* gcc.target/powerpc/ppc-pow.c: Allow for tail calls.

Comments

David Edelsohn April 18, 2011, 5:24 p.m. UTC | #1 
On Fri, Apr 8, 2011 at 10:51 PM, Alan Modra <amodra@gmail.com> wrote:
> This patch enables sibling calls for powerpc in a few more cases, and
> fixes bugs exposed by that change.  We now
> a) Allow sibling calls via function pointer.  At the time
>   rs6000_function_ok_for_sibcall was written, I don't think access to
>   arg types of function pointer calls was available in the target
>   hook/macro.
> b) Allow sibling calls to functions with vector parameters.  Contrary
>   to the previous comment about vector parameters, a problem occurs
>   with VRSAVE only when the called function has more vector
>   parameters than the caller (because VRSAVE bits will already be set
>   for the caller's arguments).  Also, the old code did not make any
>   attempt to determine whether altivec/vsx registers would actually
>   be used to pass args.  Not all vector types fit in these regs.
> c) Allow SYSV4 ABI code to make sibling calls to non-local functions
>   so long as not both flag_pic and TARGET_SECURE_PLT are set.
>   The PIC -msecure-plt call stubs use r30.
>
> sibcall_value_nonlocal_sysv was trying to read the call cookie from
> the wrong operand, a latent bug since the pattern wasn't used until
> now.  I also fixed a minor problem with CALL_LIBCALL in the call
> cookie, which could cause various call insns to match an "n"
> constraint rather than a "0" constraint and so give the wrong insn
> length.  Bootstrapped and regression tested powerpc64-linux and
> powerpc-linux.
>
> This patch does affect darwin, not only in removing what are now
> redundant insn patterns, but also (a) and (b) above.  Would someone
> please run a bootstrap and regression test on darwin for me?  I don't
> have a machine handy to confirm that I didn't break anything there.
>
> I've attached a vector testcase I used to verify some of these
> changes.
>
> gcc/
>        * config/rs6000/rs6000.c (rs6000_function_arg): Remove CALL_LIBCALL
>        when returning call_cookie.
>        (rs6000_function_ok_for_sibcall): Allow sibcalls via function
>        pointers, to functions with no more vector args than the current
>        function, and some non-local calls for ABI_V4.
>        * config/rs6000/rs6000.md (sibcall_nonlocal_aix32,
>        sibcall_nonlocal_aix64): Combine to ..
>        (sibcall_nonlocal_aix<mode>): ..this.  Handle function pointer calls.
>        (sibcall_value_nonlocal_aix32, sibcall_value_nonlocal_aix64): Combine..
>        (sibcall_value_nonlocal_aix<mode>): ..likewise.
>        (*sibcall_nonlocal_sysv<mode>): Handle function pointer calls.
>        (sibcall_value_nonlocal_sysv<mode>): Likewise.  Correct call cookie
>        operand.
>        * config/rs6000/darwin.md (sibcall_nonlocal_darwin64,
>        sibcall_value_nonlocal_darwin64, sibcall_symbolic_64,
>        sibcall_value_symbolic_64): Delete.
> gcc/testsuite/
>        * gcc.target/powerpc/ppc-pow.c: Allow for tail calls.

This is okay.

It would be nice if someone tested on Darwin, although I am sure we
will hear soon enough if it broke anything.

Thanks, David
Nathan Froyd April 18, 2011, 5:29 p.m. UTC | #2 
On Sat, Apr 09, 2011 at 12:21:46PM +0930, Alan Modra wrote:
> a) Allow sibling calls via function pointer.  At the time
>    rs6000_function_ok_for_sibcall was written, I don't think access to
>    arg types of function pointer calls was available in the target
>    hook/macro.
>
> +      /* Functions with vector parameters are required to have a
> +	 prototype, so the argument type info must be available
> +	 here.  */
> +      for (type = TYPE_ARG_TYPES (fntype);
> +	   type;
> +	   type = TREE_CHAIN (type))
> +	if (TREE_CODE (TREE_VALUE (type)) == VECTOR_TYPE
> +	    && (ALTIVEC_VECTOR_MODE (TYPE_MODE (TREE_VALUE (type)))
> +		|| VSX_VECTOR_MODE (TYPE_MODE (TREE_VALUE (type)))))
> +	  nvreg++;
> +
> +      for (type = TYPE_ARG_TYPES (TREE_TYPE (current_function_decl));
> +	   type;
> +	   type = TREE_CHAIN (type))
> +	if (TREE_CODE (TREE_VALUE (type)) == VECTOR_TYPE
> +	    && (ALTIVEC_VECTOR_MODE (TYPE_MODE (TREE_VALUE (type)))
> +		|| VSX_VECTOR_MODE (TYPE_MODE (TREE_VALUE (type)))))
> +	  nvreg--;

Could I request that you use FOREACH_FUNCTION_ARGS in these two cases?
The conversion is trivial, and avoiding more exposed TYPE_ARG_TYPES
calls is a good thing.

Thanks,
-Nathan
Mike Stump April 18, 2011, 9:18 p.m. UTC | #3 
On Apr 18, 2011, at 10:24 AM, David Edelsohn wrote:
>> This patch does affect darwin, not only in removing what are now
>> redundant insn patterns, but also (a) and (b) above.  Would someone
>> please run a bootstrap and regression test on darwin for me?  I don't
>> have a machine handy to confirm that I didn't break anything there.

> It would be nice if someone tested on Darwin, although I am sure we
> will hear soon enough if it broke anything.

I'm happy if you just want to check it in and then check Geoff's regression tester after 20 hours or whatever its cycle time is currently.
Alan Modra April 22, 2011, 3:59 a.m. UTC | #4 
On Mon, Apr 18, 2011 at 10:29:17AM -0700, Nathan Froyd wrote:
> Could I request that you use FOREACH_FUNCTION_ARGS in these two cases?
> The conversion is trivial, and avoiding more exposed TYPE_ARG_TYPES
> calls is a good thing.

Committed revision 172855.
diff mbox

Patch

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 172094)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -19305,39 +19302,73 @@  rs6000_return_addr (int count, rtx frame
   return get_hard_reg_initial_val (Pmode, LR_REGNO);
 }
 
-/* Say whether a function is a candidate for sibcall handling or not.
-   We do not allow indirect calls to be optimized into sibling calls.
-   Also, we can't do it if there are any vector parameters; there's
-   nowhere to put the VRsave code so it works; note that functions with
-   vector parameters are required to have a prototype, so the argument
-   type info must be available here.  (The tail recursion case can work
-   with vector parameters, but there's no way to distinguish here.) */
+/* Say whether a function is a candidate for sibcall handling or not.  */
+
 static bool
-rs6000_function_ok_for_sibcall (tree decl, tree exp ATTRIBUTE_UNUSED)
+rs6000_function_ok_for_sibcall (tree decl, tree exp)
 {
-  tree type;
+  tree fntype;
+
   if (decl)
+    fntype = TREE_TYPE (decl);
+  else
+    fntype = TREE_TYPE (TREE_TYPE (CALL_EXPR_FN (exp)));
+
+  /* We can't do it if the called function has more vector parameters
+     than the current function; there's nowhere to put the VRsave code.  */
+  if (TARGET_ALTIVEC_ABI
+      && TARGET_ALTIVEC_VRSAVE
+      && !(decl && decl == current_function_decl))
     {
-      if (TARGET_ALTIVEC_VRSAVE)
-	{
-	  for (type = TYPE_ARG_TYPES (TREE_TYPE (decl));
-	       type; type = TREE_CHAIN (type))
-	    {
-	      if (TREE_CODE (TREE_VALUE (type)) == VECTOR_TYPE)
-		return false;
-	    }
-	}
-      if (DEFAULT_ABI == ABI_DARWIN
-	  || ((*targetm.binds_local_p) (decl)
-	      && (DEFAULT_ABI != ABI_AIX || !DECL_EXTERNAL (decl))))
-	{
-	  tree attr_list = TYPE_ATTRIBUTES (TREE_TYPE (decl));
+      tree type;
+      int nvreg = 0;
 
-	  if (!lookup_attribute ("longcall", attr_list)
-	      || lookup_attribute ("shortcall", attr_list))
-	    return true;
-	}
+      /* Functions with vector parameters are required to have a
+	 prototype, so the argument type info must be available
+	 here.  */
+      for (type = TYPE_ARG_TYPES (fntype);
+	   type;
+	   type = TREE_CHAIN (type))
+	if (TREE_CODE (TREE_VALUE (type)) == VECTOR_TYPE
+	    && (ALTIVEC_VECTOR_MODE (TYPE_MODE (TREE_VALUE (type)))
+		|| VSX_VECTOR_MODE (TYPE_MODE (TREE_VALUE (type)))))
+	  nvreg++;
+
+      for (type = TYPE_ARG_TYPES (TREE_TYPE (current_function_decl));
+	   type;
+	   type = TREE_CHAIN (type))
+	if (TREE_CODE (TREE_VALUE (type)) == VECTOR_TYPE
+	    && (ALTIVEC_VECTOR_MODE (TYPE_MODE (TREE_VALUE (type)))
+		|| VSX_VECTOR_MODE (TYPE_MODE (TREE_VALUE (type)))))
+	  nvreg--;
+
+      if (nvreg > 0)
+	return false;
     }
+
+  /* Under the AIX ABI we can't allow calls to non-local functions,
+     because the callee may have a different TOC pointer to the
+     caller and there's no way to ensure we restore the TOC when we
+     return.  With the secure-plt SYSV ABI we can't make non-local
+     calls when -fpic/PIC because the plt call stubs use r30.  */
+  if (DEFAULT_ABI == ABI_DARWIN
+      || (DEFAULT_ABI == ABI_AIX
+	  && decl
+	  && !DECL_EXTERNAL (decl)
+	  && (*targetm.binds_local_p) (decl))
+      || (DEFAULT_ABI == ABI_V4
+	  && (!TARGET_SECURE_PLT
+	      || !flag_pic
+	      || (decl
+		  && (*targetm.binds_local_p) (decl)))))
+    {
+      tree attr_list = TYPE_ATTRIBUTES (fntype);
+
+      if (!lookup_attribute ("longcall", attr_list)
+	  || lookup_attribute ("shortcall", attr_list))
+	return true;
+    }
+
   return false;
 }
 
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 172094)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -12963,68 +12963,43 @@  (define_insn "*sibcall_value_local64"
   [(set_attr "type" "branch")
    (set_attr "length" "4,8")])
 
-(define_insn "*sibcall_nonlocal_aix32"
-  [(call (mem:SI (match_operand:SI 0 "symbol_ref_operand" "s"))
-	 (match_operand 1 "" "g"))
-   (use (match_operand:SI 2 "immediate_operand" "O"))
+(define_insn "*sibcall_nonlocal_aix<mode>"
+  [(call (mem:SI (match_operand:P 0 "call_operand" "s,c"))
+	 (match_operand 1 "" "g,g"))
+   (use (match_operand:SI 2 "immediate_operand" "O,O"))
    (use (reg:SI LR_REGNO))
    (return)]
-  "TARGET_32BIT
-   && DEFAULT_ABI == ABI_AIX
+  "DEFAULT_ABI == ABI_AIX
    && (INTVAL (operands[2]) & CALL_LONG) == 0"
-  "b %z0"
-  [(set_attr "type" "branch")
-   (set_attr "length" "4")])
-
-(define_insn "*sibcall_nonlocal_aix64"
-  [(call (mem:SI (match_operand:DI 0 "symbol_ref_operand" "s"))
-	 (match_operand 1 "" "g"))
-   (use (match_operand:SI 2 "immediate_operand" "O"))
-   (use (reg:SI LR_REGNO))
-   (return)]
-  "TARGET_64BIT
-   && DEFAULT_ABI == ABI_AIX
-   && (INTVAL (operands[2]) & CALL_LONG) == 0"
-  "b %z0"
-  [(set_attr "type" "branch")
-   (set_attr "length" "4")])
-
-(define_insn "*sibcall_value_nonlocal_aix32"
-  [(set (match_operand 0 "" "")
-	(call (mem:SI (match_operand:SI 1 "symbol_ref_operand" "s"))
-	      (match_operand 2 "" "g")))
-   (use (match_operand:SI 3 "immediate_operand" "O"))
-   (use (reg:SI LR_REGNO))
-   (return)]
-  "TARGET_32BIT
-   && DEFAULT_ABI == ABI_AIX
-   && (INTVAL (operands[3]) & CALL_LONG) == 0"
-  "b %z1"
+  "@
+   b %z0
+   b%T0"
   [(set_attr "type" "branch")
    (set_attr "length" "4")])
 
-(define_insn "*sibcall_value_nonlocal_aix64"
+(define_insn "*sibcall_value_nonlocal_aix<mode>"
   [(set (match_operand 0 "" "")
-	(call (mem:SI (match_operand:DI 1 "symbol_ref_operand" "s"))
-	      (match_operand 2 "" "g")))
-   (use (match_operand:SI 3 "immediate_operand" "O"))
+	(call (mem:SI (match_operand:P 1 "call_operand" "s,c"))
+	      (match_operand 2 "" "g,g")))
+   (use (match_operand:SI 3 "immediate_operand" "O,O"))
    (use (reg:SI LR_REGNO))
    (return)]
-  "TARGET_64BIT
-   && DEFAULT_ABI == ABI_AIX
+  "DEFAULT_ABI == ABI_AIX
    && (INTVAL (operands[3]) & CALL_LONG) == 0"
-  "b %z1"
+  "@
+   b %z1
+   b%T1"
   [(set_attr "type" "branch")
    (set_attr "length" "4")])
 
 (define_insn "*sibcall_nonlocal_sysv<mode>"
-  [(call (mem:SI (match_operand:P 0 "symbol_ref_operand" "s,s"))
+  [(call (mem:SI (match_operand:P 0 "call_operand" "s,s,c,c"))
 	 (match_operand 1 "" ""))
-   (use (match_operand 2 "immediate_operand" "O,n"))
+   (use (match_operand 2 "immediate_operand" "O,n,O,n"))
    (use (reg:SI LR_REGNO))
    (return)]
   "(DEFAULT_ABI == ABI_DARWIN
-     || DEFAULT_ABI == ABI_V4)
+    || DEFAULT_ABI == ABI_V4)
    && (INTVAL (operands[2]) & CALL_LONG) == 0"
   "*
 {
@@ -13034,7 +13009,9 @@  (define_insn "*sibcall_nonlocal_sysv<mod
   else if (INTVAL (operands[2]) & CALL_V4_CLEAR_FP_ARGS)
     output_asm_insn (\"creqv 6,6,6\", operands);
 
-  if (DEFAULT_ABI == ABI_V4 && flag_pic)
+  if (which_alternative >= 2)
+    return \"b%T0\";
+  else if (DEFAULT_ABI == ABI_V4 && flag_pic)
     {
       gcc_assert (!TARGET_SECURE_PLT);
       return \"b %z0@plt\";
@@ -13042,8 +13019,8 @@  (define_insn "*sibcall_nonlocal_sysv<mod
   else
     return \"b %z0\";
 }"
-  [(set_attr "type" "branch,branch")
-   (set_attr "length" "4,8")])
+  [(set_attr "type" "branch")
+   (set_attr "length" "4,8,4,8")])
 
 (define_expand "sibcall_value"
   [(parallel [(set (match_operand 0 "register_operand" "")
@@ -13068,23 +13045,25 @@  (define_expand "sibcall_value"
 
 (define_insn "*sibcall_value_nonlocal_sysv<mode>"
   [(set (match_operand 0 "" "")
-	(call (mem:SI (match_operand:P 1 "symbol_ref_operand" "s,s"))
+	(call (mem:SI (match_operand:P 1 "call_operand" "s,s,c,c"))
 	      (match_operand 2 "" "")))
-   (use (match_operand:SI 3 "immediate_operand" "O,n"))
+   (use (match_operand:SI 3 "immediate_operand" "O,n,O,n"))
    (use (reg:SI LR_REGNO))
    (return)]
   "(DEFAULT_ABI == ABI_DARWIN
-       || DEFAULT_ABI == ABI_V4)
+    || DEFAULT_ABI == ABI_V4)
    && (INTVAL (operands[3]) & CALL_LONG) == 0"
   "*
 {
-  if (INTVAL (operands[2]) & CALL_V4_SET_FP_ARGS)
+  if (INTVAL (operands[3]) & CALL_V4_SET_FP_ARGS)
     output_asm_insn (\"crxor 6,6,6\", operands);
 
-  else if (INTVAL (operands[2]) & CALL_V4_CLEAR_FP_ARGS)
+  else if (INTVAL (operands[3]) & CALL_V4_CLEAR_FP_ARGS)
     output_asm_insn (\"creqv 6,6,6\", operands);
 
-  if (DEFAULT_ABI == ABI_V4 && flag_pic)
+  if (which_alternative >= 2)
+    return \"b%T1\";
+  else if (DEFAULT_ABI == ABI_V4 && flag_pic)
     {
       gcc_assert (!TARGET_SECURE_PLT);
       return \"b %z1@plt\";
@@ -13092,8 +13071,8 @@  (define_insn "*sibcall_value_nonlocal_sy
   else
     return \"b %z1\";
 }"
-  [(set_attr "type" "branch,branch")
-   (set_attr "length" "4,8")])
+  [(set_attr "type" "branch")
+   (set_attr "length" "4,8,4,8")])
 
 (define_expand "sibcall_epilogue"
   [(use (const_int 0))]
Index: gcc/config/rs6000/darwin.md
===================================================================
--- gcc/config/rs6000/darwin.md	(revision 172094)
+++ gcc/config/rs6000/darwin.md	(working copy)
@@ -370,73 +370,3 @@  (define_insn "*call_value_nonlocal_darwi
 }
   [(set_attr "type" "branch,branch")
    (set_attr "length" "4,8")])
-
-(define_insn "*sibcall_nonlocal_darwin64"
-  [(call (mem:SI (match_operand:DI 0 "symbol_ref_operand" "s,s"))
-	 (match_operand 1 "" ""))
-   (use (match_operand 2 "immediate_operand" "O,n"))
-   (use (reg:SI 65))
-   (return)]
-  "(DEFAULT_ABI == ABI_DARWIN)
-   && (INTVAL (operands[2]) & CALL_LONG) == 0"
-{
-  return "b %z0";
-}
-  [(set_attr "type" "branch,branch")
-   (set_attr "length" "4,8")])
-
-(define_insn "*sibcall_value_nonlocal_darwin64"
-  [(set (match_operand 0 "" "")
-	(call (mem:SI (match_operand:DI 1 "symbol_ref_operand" "s,s"))
-	      (match_operand 2 "" "")))
-   (use (match_operand:SI 3 "immediate_operand" "O,n"))
-   (use (reg:SI 65))
-   (return)]
-  "(DEFAULT_ABI == ABI_DARWIN)
-   && (INTVAL (operands[3]) & CALL_LONG) == 0"
-  "*
-{
-  return \"b %z1\";
-}"
-  [(set_attr "type" "branch,branch")
-   (set_attr "length" "4,8")])
-
-
-(define_insn "*sibcall_symbolic_64"
-  [(call (mem:SI (match_operand:DI 0 "call_operand" "s,c")) ; 64
-	 (match_operand 1 "" ""))
-   (use (match_operand 2 "" ""))
-   (use (reg:SI 65))
-   (return)]
-  "TARGET_64BIT && DEFAULT_ABI == ABI_DARWIN"
-  "*
-{
-  switch (which_alternative)
-    {
-      case 0:  return \"b %z0\";
-      case 1:  return \"b%T0\";
-      default:  gcc_unreachable ();
-    }
-}"
-  [(set_attr "type" "branch")
-   (set_attr "length" "4")])
-
-(define_insn "*sibcall_value_symbolic_64"
-  [(set (match_operand 0 "" "")
-	(call (mem:SI (match_operand:DI 1 "call_operand" "s,c"))
-	      (match_operand 2 "" "")))
-   (use (match_operand:SI 3 "" ""))
-   (use (reg:SI 65))
-   (return)]
-  "TARGET_64BIT && DEFAULT_ABI == ABI_DARWIN"
-  "*
-{
-  switch (which_alternative)
-    {
-      case 0:  return \"b %z1\";
-      case 1:  return \"b%T1\";
-      default:  gcc_unreachable ();
-    }
-}"
-  [(set_attr "type" "branch")
-   (set_attr "length" "4")])
Index: gcc/testsuite/gcc.target/powerpc/ppc-pow.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/ppc-pow.c	(revision 172094)
+++ gcc/testsuite/gcc.target/powerpc/ppc-pow.c	(working copy)
@@ -2,8 +2,8 @@ 
 /* { dg-options "-O2 -ffast-math -mcpu=power6" } */
 /* { dg-final { scan-assembler-times "fsqrt" 3 } } */
 /* { dg-final { scan-assembler-times "fmul" 1 } } */
-/* { dg-final { scan-assembler-times "bl pow" 1 } } */
-/* { dg-final { scan-assembler-times "bl sqrt" 1 } } */
+/* { dg-final { scan-assembler-times "bl? pow" 1 } } */
+/* { dg-final { scan-assembler-times "bl? sqrt" 1 } } */
 
 double
 do_pow_0_75_default (double a)