Re: Tighten check for whether a sibcall references local variables
On 11/23/2016 10:09 AM, Richard Sandiford wrote:
Richard Biener writes:
On Tue, Nov 22, 2016 at 10:00 AM, Richard Sandiford
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.
OK, how's this? Tested on aarch64-linux-gnu and x86_64-linux-gnu.
Thanks,
Richard
gcc/testsuite/
* gcc.dg/tree-ssa/tailcall-7-run.c: New test.
* gcc.dg/tree-ssa/tailcall-8-run.c: Likewise.
Works for me.
jeff
Re: Tighten check for whether a sibcall references local variables
Richard Biener writes:
> On Tue, Nov 22, 2016 at 10:00 AM, Richard Sandiford
> 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.
OK, how's this? Tested on aarch64-linux-gnu and x86_64-linux-gnu.
Thanks,
Richard
gcc/testsuite/
* gcc.dg/tree-ssa/tailcall-7-run.c: New test.
* gcc.dg/tree-ssa/tailcall-8-run.c: Likewise.
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/tailcall-7-run.c
b/gcc/testsuite/gcc.dg/tree-ssa/tailcall-7-run.c
new file mode 100644
index 000..d18ff9d
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/tailcall-7-run.c
@@ -0,0 +1,72 @@
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+/* { dg-additional-sources "tailcall-7.c" } */
+
+struct s { int x; };
+extern struct s global;
+
+void g1 (void);
+void g2 (void);
+void g3 (struct s *);
+struct s g4 (struct s);
+struct s g5 (void);
+struct s g6 (void);
+struct s g7 (void);
+struct s g8 (struct s *);
+int g9 (struct s);
+int g10 (int);
+
+struct s last;
+struct s tmp;
+
+struct s
+f (int i)
+{
+ struct s ret;
+ ret.x = i + 100;
+ last = ret;
+ return ret;
+}
+
+void
+callit (void (*fn) (void))
+{
+ fn ();
+}
+
+int
+test (int last_val, int global_val, int tmp_val)
+{
+ return last.x == last_val && global.x == global_val && tmp.x == tmp_val;
+}
+
+int
+main (void)
+{
+ global.x = 200;
+ tmp.x = 300;
+ g1 ();
+ if (!test (101, 200, 300))
+__builtin_abort ();
+ g2 ();
+ if (!test (102, 102, 300))
+__builtin_abort ();
+ g3 (&tmp);
+ if (!test (103, 102, 103))
+__builtin_abort ();
+ if (g4 (tmp).x != 104 || !test (104, 102, 103))
+__builtin_abort ();
+ if (g5 ().x != 105 || !test (105, 102, 103))
+__builtin_abort ();
+ if (g6 ().x != 106 || !test (106, 102, 103))
+__builtin_abort ();
+ if (g7 ().x != 107 || !test (107, 107, 103))
+__builtin_abort ();
+ if (g8 (&tmp).x != 108 || !test (108, 107, 108))
+__builtin_abort ();
+ if (g9 (tmp) != 9 || !test (109, 107, 108))
+__builtin_abort ();
+ if (g10 (10) != 10 || !test (110, 107, 108))
+__builtin_abort ();
+ return 0;
+}
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/tailcall-8-run.c
b/gcc/testsuite/gcc.dg/tree-ssa/tailcall-8-run.c
new file mode 100644
index 000..f892909
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/tailcall-8-run.c
@@ -0,0 +1,86 @@
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+/* { dg-additional-sources "tailcall-8.c" } */
+
+struct s { int x; };
+
+int expected;
+struct s *last_ptr;
+struct s tmp;
+
+void
+start (int val, struct s *initial_last_ptr)
+{
+ expected = val;
+ tmp.x = val;
+ last_ptr = initial_last_ptr;
+}
+
+void
+f_direct (struct s param)
+{
+ if (param.x != expected)
+__builtin_abort ();
+}
+
+void
+f_indirect (struct s *ptr)
+{
+ if (ptr->x != expected)
+__builtin_abort ();
+ last_ptr = ptr;
+ ptr->x += 100;
+}
+
+void
+f_void (void)
+{
+ if (last_ptr->x != expected + 100)
+__builtin_abort ();
+}
+
+
+void g1 (struct s);
+void g2 (struct s *);
+void g3 (struct s *);
+void g4 (struct s *);
+void g5 (struct s);
+void g6 (struct s);
+void g7 (struct s);
+void g8 (struct s *);
+void g9 (struct s *);
+
+int
+main (void)
+{
+ struct s g6_s = { 106 };
+
+ start (1, 0);
+ g1 (tmp);
+
+ start (2, 0);
+ g2 (&tmp);
+
+ start (3, 0);
+ g3 (&tmp);
+
+ start (4, 0);
+ g4 (&tmp);
+
+ start (5, 0);
+ g5 (tmp);
+
+ start (6, &g6_s);
+ g6 (tmp);
+
+ start (7, 0);
+ g7 (tmp);
+
+ start (8, 0);
+ g8 (&tmp);
+
+ start (9, 0);
+ g9 (&tmp);
+
+ return 0;
+}
Re: Tighten check for whether a sibcall references local variables
On Tue, Nov 22, 2016 at 10:00 AM, Richard Sandiford
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 000..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;
>
Tighten check for whether a sibcall references local variables
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.
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 000..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;
