diff mbox

[tree-tailcall] Check if function returns it's argument

Message ID CAAgBjMn6EL+RZWgbAkd5OB3L1r_Mns1P_qT1YA-6Eeg+bLY24g@mail.gmail.com
State New
Headers show

Commit Message

Prathamesh Kulkarni Dec. 2, 2016, 6:03 a.m. UTC 
On 2 December 2016 at 03:57, Jeff Law <law@redhat.com> wrote:
> On 12/01/2016 06:22 AM, Richard Biener wrote:

>>>

>>> Well after removing DECL_BY_REFERENCE, verify_gimple still fails but

>>> differently:

>>>

>>> tail-padding1.C:13:1: error: RESULT_DECL should be read only when

>>> DECL_BY_REFERENCE is set

>>>  }

>>>  ^

>>> while verifying SSA_NAME nrvo_4 in statement

>>> # .MEM_3 = VDEF <.MEM_1(D)>

>>>     nrvo_4 = __builtin_memset (nrvo_2(D), 0, 8);

>>> tail-padding1.C:13:1: internal compiler error: verify_ssa failed

>>

>>

>> Hmm, ok.  Not sure why we enforce this.

>

> I don't know either.  But I would start by looking at tree-nrv.c since it

> looks (based on the variable names) that the named-value-return optimization

> kicked in.

Um, the name nrv0 was in the test-case itself. The transform takes
place in tailr1 pass,
which appears to be before nrv, so possibly this is not related to nrv ?

The verify check seems to be added in r161898 by Honza to fix PR 44813
based on Richard's following suggestion from
https://gcc.gnu.org/ml/gcc-patches/2010-07/msg00358.html:

"We should never see a defintion of a RESULT_DECL SSA name for
DECL_BY_REFERENCE RESULT_DECLs (that would
be a bug - we should add verification to the SSA verifier, can you
do add that?)."

The attached patch moves && ret_stmt together with !ass_var,
and keeps the !DECL_BY_REFERENCE (DECL_RESULT (cfun->decl))
check, and adjusts tailcall-9.c testcase to scan _\[0-9\]* = __builtin_memcpy
in tailr1 dump since that's where the transform takes place.
Is this version OK ?

Thanks,
Prathamesh
>

>>

>> Note that in the end this patch looks fishy -- iff we really need

>> the LHS on the assignment for correctness if we have the tailcall

>> flag set then what guarantees that later passes do not remove it

>> again?  So anybody removing a LHS would need to unset the tailcall flag?

>>

>> Saying again that I don't know enough about the RTL part of tailcall

>> expansion.

>

> The LHS on the assignment makes it easier to identify when a tail call is

> possible.  It's not needed for correctness.  Not having the LHS on the

> assignment just means we won't get an optimized tail call.

>

> Under what circumstances would the LHS possibly be removed?  We know the

> return statement references the LHS, so it's not going to be something that

> DCE will do.

>

> jeff
diff mbox

Patch

diff --git a/gcc/testsuite/gcc.dg/tree-ssa/tailcall-9.c b/gcc/testsuite/gcc.dg/tree-ssa/tailcall-9.c
new file mode 100644
index 0000000..9c482f4
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/tailcall-9.c
@@ -0,0 +1,10 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-tailr-details" } */
+
+void *f(void *a1, void *a2, __SIZE_TYPE__ a3)
+{
+  __builtin_memcpy (a1, a2, a3);
+  return a1;
+}
+
+/* { dg-final { scan-tree-dump "_\[0-9\]* = __builtin_memcpy" "tailr1" } } */ 
diff --git a/gcc/tree-tailcall.c b/gcc/tree-tailcall.c
index 66a0a4c..64f624f 100644
--- a/gcc/tree-tailcall.c
+++ b/gcc/tree-tailcall.c
@@ -401,6 +401,7 @@  find_tail_calls (basic_block bb, struct tailcall **ret)
   basic_block abb;
   size_t idx;
   tree var;
+  greturn *ret_stmt = NULL;
 
   if (!single_succ_p (bb))
     return;
@@ -408,6 +409,8 @@  find_tail_calls (basic_block bb, struct tailcall **ret)
   for (gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
     {
       stmt = gsi_stmt (gsi);
+      if (!ret_stmt)
+	ret_stmt = dyn_cast<greturn *> (stmt);
 
       /* Ignore labels, returns, nops, clobbers and debug stmts.  */
       if (gimple_code (stmt) == GIMPLE_LABEL
@@ -422,6 +425,35 @@  find_tail_calls (basic_block bb, struct tailcall **ret)
 	{
 	  call = as_a <gcall *> (stmt);
 	  ass_var = gimple_call_lhs (call);
+	  if (!ass_var && ret_stmt)
+	    {
+	      /* Check if function returns one if it's arguments
+		 and that argument is used as return value.
+		 In that case create an artificial lhs to call_stmt,
+		 and set it as the return value.  */
+
+	      unsigned rf = gimple_call_return_flags (call);
+	      if (rf & ERF_RETURNS_ARG)
+		{
+		  unsigned argnum = rf & ERF_RETURN_ARG_MASK;
+		  if (argnum < gimple_call_num_args (call))
+		    {
+		      tree arg = gimple_call_arg (call, argnum);
+		      tree retval = gimple_return_retval (ret_stmt);
+		      if (retval
+			  && TREE_CODE (retval) == SSA_NAME
+			  && operand_equal_p (retval, arg, 0)
+			  && !DECL_BY_REFERENCE (DECL_RESULT (cfun->decl)))
+			{
+			  ass_var = copy_ssa_name (arg);
+			  gimple_call_set_lhs (call, ass_var);
+			  update_stmt (call);
+			  gimple_return_set_retval (ret_stmt, ass_var);
+			  update_stmt (ret_stmt);
+			}
+		    }
+		}
+	    }
 	  break;
 	}