diff mbox

Tighten check for whether a sibcall references local variables

Message ID 87polnq46z.fsf@e105548-lin.cambridge.arm.com
State New
Headers show

Commit Message

Richard Sandiford Nov. 22, 2016, 9 a.m. UTC
This loop:

  /* Make sure the tail invocation of this function does not refer
     to local variables.  */
  FOR_EACH_LOCAL_DECL (cfun, idx, var)
    {
      if (TREE_CODE (var) != PARM_DECL
          && auto_var_in_fn_p (var, cfun->decl)
          && (ref_maybe_used_by_stmt_p (call, var)
              || call_may_clobber_ref_p (call, var)))
        return;
    }

triggered even for local variables that are passed by value.
This meant that we didn't allow local aggregates to be passed
to a sibling call but did (for example) allow global aggregates
to be passed.

I think the loop is really checking for indirect references,
so should be able to skip any variables that never have their
address taken.

Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Thanks,
Richard


gcc/
	* tree-tailcall.c (find_tail_calls): Allow calls to reference
	local variables if all references are known to be direct.

gcc/testsuite/
	* gcc.dg/tree-ssa/tailcall-8.c: New test.

Comments

Richard Biener Nov. 22, 2016, 11:38 a.m. UTC | #1
On Tue, Nov 22, 2016 at 10:00 AM, Richard Sandiford
<richard.sandiford@arm.com> wrote:
> This loop:
>
>   /* Make sure the tail invocation of this function does not refer
>      to local variables.  */
>   FOR_EACH_LOCAL_DECL (cfun, idx, var)
>     {
>       if (TREE_CODE (var) != PARM_DECL
>           && auto_var_in_fn_p (var, cfun->decl)
>           && (ref_maybe_used_by_stmt_p (call, var)
>               || call_may_clobber_ref_p (call, var)))
>         return;
>     }
>
> triggered even for local variables that are passed by value.
> This meant that we didn't allow local aggregates to be passed
> to a sibling call but did (for example) allow global aggregates
> to be passed.
>
> I think the loop is really checking for indirect references,
> so should be able to skip any variables that never have their
> address taken.
>
> Tested on aarch64-linux-gnu and x86_64-linux-gnu.  OK to install?

Ok.  We've had various correctness issues in this part in the past so
I'd prefer if you can rewrite your dg-do compile tests to dg-do run ones
that verify the code works correctly.  I suggest to use a dg-additional-sources
with a separate TU for the execution driver.

Thanks,
Richard.

> Thanks,
> Richard
>
>
> gcc/
>         * tree-tailcall.c (find_tail_calls): Allow calls to reference
>         local variables if all references are known to be direct.
>
> gcc/testsuite/
>         * gcc.dg/tree-ssa/tailcall-8.c: New test.
>
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/tailcall-8.c b/gcc/testsuite/gcc.dg/tree-ssa/tailcall-8.c
> new file mode 100644
> index 0000000..ffeabe5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/tailcall-8.c
> @@ -0,0 +1,80 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-tailc-details" } */
> +
> +struct s { int x; };
> +void f_direct (struct s);
> +void f_indirect (struct s *);
> +void f_void (void);
> +
> +/* Tail call.  */
> +void
> +g1 (struct s param)
> +{
> +  f_direct (param);
> +}
> +
> +/* Tail call.  */
> +void
> +g2 (struct s *param_ptr)
> +{
> +  f_direct (*param_ptr);
> +}
> +
> +/* Tail call.  */
> +void
> +g3 (struct s *param_ptr)
> +{
> +  f_indirect (param_ptr);
> +}
> +
> +/* Tail call.  */
> +void
> +g4 (struct s *param_ptr)
> +{
> +  f_indirect (param_ptr);
> +  f_void ();
> +}
> +
> +/* Tail call.  */
> +void
> +g5 (struct s param)
> +{
> +  struct s local = param;
> +  f_direct (local);
> +}
> +
> +/* Tail call.  */
> +void
> +g6 (struct s param)
> +{
> +  struct s local = param;
> +  f_direct (local);
> +  f_void ();
> +}
> +
> +/* Not a tail call.  */
> +void
> +g7 (struct s param)
> +{
> +  struct s local = param;
> +  f_indirect (&local);
> +}
> +
> +/* Not a tail call.  */
> +void
> +g8 (struct s *param_ptr)
> +{
> +  struct s local = *param_ptr;
> +  f_indirect (&local);
> +}
> +
> +/* Not a tail call.  */
> +void
> +g9 (struct s *param_ptr)
> +{
> +  struct s local = *param_ptr;
> +  f_indirect (&local);
> +  f_void ();
> +}
> +
> +/* { dg-final { scan-tree-dump-times "Found tail call" 6 "tailc" } } */
> diff --git a/gcc/tree-tailcall.c b/gcc/tree-tailcall.c
> index f97541d..66a0a4c 100644
> --- a/gcc/tree-tailcall.c
> +++ b/gcc/tree-tailcall.c
> @@ -504,12 +504,14 @@ find_tail_calls (basic_block bb, struct tailcall **ret)
>         tail_recursion = true;
>      }
>
> -  /* Make sure the tail invocation of this function does not refer
> -     to local variables.  */
> +  /* Make sure the tail invocation of this function does not indirectly
> +     refer to local variables.  (Passing variables directly by value
> +     is OK.)  */
>    FOR_EACH_LOCAL_DECL (cfun, idx, var)
>      {
>        if (TREE_CODE (var) != PARM_DECL
>           && auto_var_in_fn_p (var, cfun->decl)
> +         && may_be_aliased (var)
>           && (ref_maybe_used_by_stmt_p (call, var)
>               || call_may_clobber_ref_p (call, var)))
>         return;
>
diff mbox

Patch

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/tailcall-8.c b/gcc/testsuite/gcc.dg/tree-ssa/tailcall-8.c
new file mode 100644
index 0000000..ffeabe5
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/tailcall-8.c
@@ -0,0 +1,80 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-tailc-details" } */
+
+struct s { int x; };
+void f_direct (struct s);
+void f_indirect (struct s *);
+void f_void (void);
+
+/* Tail call.  */
+void
+g1 (struct s param)
+{
+  f_direct (param);
+}
+
+/* Tail call.  */
+void
+g2 (struct s *param_ptr)
+{
+  f_direct (*param_ptr);
+}
+
+/* Tail call.  */
+void
+g3 (struct s *param_ptr)
+{
+  f_indirect (param_ptr);
+}
+
+/* Tail call.  */
+void
+g4 (struct s *param_ptr)
+{
+  f_indirect (param_ptr);
+  f_void ();
+}
+
+/* Tail call.  */
+void
+g5 (struct s param)
+{
+  struct s local = param;
+  f_direct (local);
+}
+
+/* Tail call.  */
+void
+g6 (struct s param)
+{
+  struct s local = param;
+  f_direct (local);
+  f_void ();
+}
+
+/* Not a tail call.  */
+void
+g7 (struct s param)
+{
+  struct s local = param;
+  f_indirect (&local);
+}
+
+/* Not a tail call.  */
+void
+g8 (struct s *param_ptr)
+{
+  struct s local = *param_ptr;
+  f_indirect (&local);
+}
+
+/* Not a tail call.  */
+void
+g9 (struct s *param_ptr)
+{
+  struct s local = *param_ptr;
+  f_indirect (&local);
+  f_void ();
+}
+
+/* { dg-final { scan-tree-dump-times "Found tail call" 6 "tailc" } } */
diff --git a/gcc/tree-tailcall.c b/gcc/tree-tailcall.c
index f97541d..66a0a4c 100644
--- a/gcc/tree-tailcall.c
+++ b/gcc/tree-tailcall.c
@@ -504,12 +504,14 @@  find_tail_calls (basic_block bb, struct tailcall **ret)
 	tail_recursion = true;
     }
 
-  /* Make sure the tail invocation of this function does not refer
-     to local variables.  */
+  /* Make sure the tail invocation of this function does not indirectly
+     refer to local variables.  (Passing variables directly by value
+     is OK.)  */
   FOR_EACH_LOCAL_DECL (cfun, idx, var)
     {
       if (TREE_CODE (var) != PARM_DECL
 	  && auto_var_in_fn_p (var, cfun->decl)
+	  && may_be_aliased (var)
 	  && (ref_maybe_used_by_stmt_p (call, var)
 	      || call_may_clobber_ref_p (call, var)))
 	return;