diff mbox

[PR66010] Don't take address of ap unless necessary

Message ID 5554BC07.7040609@mentor.com
State New
Headers show

Commit Message

Tom de Vries May 14, 2015, 3:15 p.m. UTC
On 12-05-15 12:04, Richard Biener wrote:
>> The fact that we have to handle this specially in both build_va_arg and
>> >gimplify_va_arg makes me wonder whether we should be dealing with all this in
>> >build_va_arg already.
>> >
>> >That is, determine whether we take the address, and add the address operator
>> >if so in build_va_arg already. Likewise, determine do_deref and pass that as
>> >extra operand.
> That would certainly be a nice cleanup.

I've implemented this cleanup.

Furthermore, I realized that in expand_ifn_va_arg_1, 'do_deref == 0' disables code:
...
	if (do_deref == integer_one_node)
	  ap = build_fold_indirect_ref (ap);
...

that has a copy in the target hook:
...
    gpr = build3 (COMPONENT_REF, TREE_TYPE (f_gpr),
		build_va_arg_indirect_ref (valist), f_gpr, NULL_TREE);
   valist = build_va_arg_indirect_ref (valist);
...

It's easier to remove that code from the few target hooks that do that, execute 
the 'do_deref == 1' code unconditionally in expand_ifn_va_arg_1 and simply 
remove all do_deref propagation (that also means we don't have to add an operand 
to VA_ARG_EXPR).


Bootstrapped and reg-tested on x86_64, with and without -m32.

Did partial build and compiled a few va_arg test-cases on the other targets.

OK for trunk?

Thanks,
- Tom

Comments

Richard Biener May 18, 2015, 7:31 a.m. UTC | #1
On Thu, 14 May 2015, Tom de Vries wrote:

> On 12-05-15 12:04, Richard Biener wrote:
> > > The fact that we have to handle this specially in both build_va_arg and
> > > >gimplify_va_arg makes me wonder whether we should be dealing with all
> > > this in
> > > >build_va_arg already.
> > > >
> > > >That is, determine whether we take the address, and add the address
> > > operator
> > > >if so in build_va_arg already. Likewise, determine do_deref and pass that
> > > as
> > > >extra operand.
> > That would certainly be a nice cleanup.
> 
> I've implemented this cleanup.
> 
> Furthermore, I realized that in expand_ifn_va_arg_1, 'do_deref == 0' disables
> code:
> ...
> 	if (do_deref == integer_one_node)
> 	  ap = build_fold_indirect_ref (ap);
> ...
> 
> that has a copy in the target hook:
> ...
>    gpr = build3 (COMPONENT_REF, TREE_TYPE (f_gpr),
> 		build_va_arg_indirect_ref (valist), f_gpr, NULL_TREE);
>   valist = build_va_arg_indirect_ref (valist);
> ...
> 
> It's easier to remove that code from the few target hooks that do that,
> execute the 'do_deref == 1' code unconditionally in expand_ifn_va_arg_1 and
> simply remove all do_deref propagation (that also means we don't have to add
> an operand to VA_ARG_EXPR).
> 
> 
> Bootstrapped and reg-tested on x86_64, with and without -m32.
> 
> Did partial build and compiled a few va_arg test-cases on the other targets.
> 
> OK for trunk?

Ok.

Thanks,
Richard.

> Thanks,
> - Tom
> 
>
diff mbox

Patch

Move array-type va_list handling to build_va_arg

2015-05-13  Tom de Vries  <tom@codesourcery.com>

	* gimplify.c (gimplify_modify_expr): Remove do_deref handling.
	(gimplify_va_arg_expr): Remove do_deref handling.  Remove adding of
	address operator to va_list operand.
	* tree-stdarg.c (expand_ifn_va_arg_1): Do deref of va_list operand
	unconditionally.
	* config/i386/i386.c (ix86_gimplify_va_arg): Remove deref on va_list
	operand.
	* config/rs6000/rs6000.c (rs6000_gimplify_va_arg): Same.
	* config/s390/s390.c (s390_gimplify_va_arg): Same.
	* config/spu/spu.c (spu_gimplify_va_arg_expr): Same.

	* c-common.c (build_va_arg_1): New function.
	(build_va_arg): Add address operator to va_list operand if necessary.
---
 gcc/c-family/c-common.c    | 127 +++++++++++++++++++++++++++++++++++++++------
 gcc/config/i386/i386.c     |   6 +--
 gcc/config/rs6000/rs6000.c |   1 -
 gcc/config/s390/s390.c     |   1 -
 gcc/config/spu/spu.c       |   1 -
 gcc/gimplify.c             |  37 ++-----------
 gcc/tree-stdarg.c          |  14 ++---
 7 files changed, 125 insertions(+), 62 deletions(-)

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 7e5ac72..8f9657d 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -5915,6 +5915,20 @@  set_compound_literal_name (tree decl)
   DECL_NAME (decl) = get_identifier (name);
 }
 
+/* build_va_arg helper function.  Return a VA_ARG_EXPR with location LOC, type
+   TYPE and operand OP.  */
+
+static tree
+build_va_arg_1 (location_t loc, tree type, tree op)
+{
+  tree expr = build1 (VA_ARG_EXPR, type, op);
+  SET_EXPR_LOCATION (expr, loc);
+  return expr;
+}
+
+/* Return a VA_ARG_EXPR corresponding to a source-level expression
+   va_arg (EXPR, TYPE) at source location LOC.  */
+
 tree
 build_va_arg (location_t loc, tree expr, tree type)
 {
@@ -5923,24 +5937,107 @@  build_va_arg (location_t loc, tree expr, tree type)
 			? NULL_TREE
 			: targetm.canonical_va_list_type (va_type));
 
-  if (canon_va_type != NULL)
+  if (va_type == error_mark_node
+      || canon_va_type == NULL_TREE)
     {
-      /* When the va_arg ap argument is a parm decl with declared type va_list,
-	 and the va_list type is an array, then grokdeclarator changes the type
-	 of the parm decl to the corresponding pointer type.  We know that that
-	 pointer is constant, so there's no need to modify it, so there's no
-	 need to pass it around using an address operator, so there's no need to
-	 mark it addressable.  */
-      if (!(TREE_CODE (canon_va_type) == ARRAY_TYPE
-	    && TREE_CODE (va_type) != ARRAY_TYPE))
-	/* In gimplify_va_arg_expr we take the address of the ap argument, mark
-	   it addressable now.  */
-	mark_addressable (expr);
+      /* Let's handle things neutrallly, if expr:
+	 - has undeclared type, or
+	 - is not an va_list type.  */
+      return build_va_arg_1 (loc, type, expr);
     }
 
-  expr = build1 (VA_ARG_EXPR, type, expr);
-  SET_EXPR_LOCATION (expr, loc);
-  return expr;
+  if (TREE_CODE (canon_va_type) != ARRAY_TYPE)
+    {
+      /* Case 1: Not an array type.  */
+
+      /* Take the address, to get '&ap'.  */
+      mark_addressable (expr);
+      expr = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (expr)), expr);
+
+      /* Verify that &ap is still recognized as having va_list type.  */
+      tree canon_expr_type
+	= targetm.canonical_va_list_type (TREE_TYPE (expr));
+      gcc_assert (canon_expr_type != NULL_TREE);
+
+      return build_va_arg_1 (loc, type, expr);
+    }
+
+  /* Case 2: Array type.
+
+     Background:
+
+     For contrast, let's start with the simple case (case 1).  If
+     canon_va_type is not an array type, but say a char *, then when
+     passing-by-value a va_list, the type of the va_list param decl is
+     the same as for another va_list decl (all ap's are char *):
+
+     f2_1 (char * ap)
+       D.1815 = VA_ARG (&ap, 0B, 1);
+       return D.1815;
+
+     f2 (int i)
+       char * ap.0;
+       char * ap;
+       __builtin_va_start (&ap, 0);
+       ap.0 = ap;
+       res = f2_1 (ap.0);
+       __builtin_va_end (&ap);
+       D.1812 = res;
+       return D.1812;
+
+     However, if canon_va_type is ARRAY_TYPE, then when passing-by-value a
+     va_list the type of the va_list param decl (case 2b, struct * ap) is not
+     the same as for another va_list decl (case 2a, struct ap[1]).
+
+     f2_1 (struct  * ap)
+       D.1844 = VA_ARG (ap, 0B, 0);
+       return D.1844;
+
+     f2 (int i)
+       struct  ap[1];
+       __builtin_va_start (&ap, 0);
+       res = f2_1 (&ap);
+       __builtin_va_end (&ap);
+       D.1841 = res;
+       return D.1841;
+
+     Case 2b is different because:
+     - on the callee side, the parm decl has declared type va_list, but
+       grokdeclarator changes the type of the parm decl to a pointer to the
+       array elem type.
+     - on the caller side, the pass-by-value uses &ap.
+
+     We unify these two cases (case 2a: va_list is array type,
+     case 2b: va_list is pointer to array elem type), by adding '&' for the
+     array type case, such that we have a pointer to array elem in both
+     cases.  */
+
+  if (TREE_CODE (va_type) == ARRAY_TYPE)
+    {
+      /* Case 2a: va_list is array type.  */
+
+      /* Take the address, to get '&ap'.  Make sure it's a pointer to array
+	 elem type.  */
+      mark_addressable (expr);
+      expr = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (canon_va_type)),
+		     expr);
+
+      /* Verify that &ap is still recognized as having va_list type.  */
+      tree canon_expr_type
+	= targetm.canonical_va_list_type (TREE_TYPE (expr));
+      gcc_assert (canon_expr_type != NULL_TREE);
+    }
+  else
+    {
+      /* Case 2b: va_list is pointer to array elem type.  */
+      gcc_assert (POINTER_TYPE_P (va_type));
+      gcc_assert (TREE_TYPE (va_type) == TREE_TYPE (canon_va_type));
+
+      /* Don't take the address.  We've already got '&ap'.  */
+      ;
+    }
+
+  return build_va_arg_1 (loc, type, expr);
 }
 
 
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index de333d8..4ec469b 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -9094,8 +9094,8 @@  ix86_gimplify_va_arg (tree valist, tree type, gimple_seq *pre_p,
   f_sav = DECL_CHAIN (f_ovf);
 
   gpr = build3 (COMPONENT_REF, TREE_TYPE (f_gpr),
-		build_va_arg_indirect_ref (valist), f_gpr, NULL_TREE);
-  valist = build_va_arg_indirect_ref (valist);
+		valist, f_gpr, NULL_TREE);
+
   fpr = build3 (COMPONENT_REF, TREE_TYPE (f_fpr), valist, f_fpr, NULL_TREE);
   ovf = build3 (COMPONENT_REF, TREE_TYPE (f_ovf), valist, f_ovf, NULL_TREE);
   sav = build3 (COMPONENT_REF, TREE_TYPE (f_sav), valist, f_sav, NULL_TREE);
@@ -9315,7 +9315,7 @@  ix86_gimplify_va_arg (tree valist, tree type, gimple_seq *pre_p,
 	{
 	  t = build2 (PLUS_EXPR, TREE_TYPE (fpr), fpr,
 		      build_int_cst (TREE_TYPE (fpr), needed_sseregs * 16));
-	  gimplify_assign (fpr, t, pre_p);
+	  gimplify_assign (unshare_expr (fpr), t, pre_p);
 	}
 
       gimple_seq_add_stmt (pre_p, gimple_build_goto (lab_over));
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index ec7332c..077e55a 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -11470,7 +11470,6 @@  rs6000_gimplify_va_arg (tree valist, tree type, gimple_seq *pre_p,
   f_ovf = DECL_CHAIN (f_res);
   f_sav = DECL_CHAIN (f_ovf);
 
-  valist = build_va_arg_indirect_ref (valist);
   gpr = build3 (COMPONENT_REF, TREE_TYPE (f_gpr), valist, f_gpr, NULL_TREE);
   fpr = build3 (COMPONENT_REF, TREE_TYPE (f_fpr), unshare_expr (valist),
 		f_fpr, NULL_TREE);
diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c
index cbe8e1f..e9a4e70 100644
--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -9747,7 +9747,6 @@  s390_gimplify_va_arg (tree valist, tree type, gimple_seq *pre_p,
   f_ovf = DECL_CHAIN (f_fpr);
   f_sav = DECL_CHAIN (f_ovf);
 
-  valist = build_va_arg_indirect_ref (valist);
   gpr = build3 (COMPONENT_REF, TREE_TYPE (f_gpr), valist, f_gpr, NULL_TREE);
   fpr = build3 (COMPONENT_REF, TREE_TYPE (f_fpr), valist, f_fpr, NULL_TREE);
   sav = build3 (COMPONENT_REF, TREE_TYPE (f_sav), valist, f_sav, NULL_TREE);
diff --git a/gcc/config/spu/spu.c b/gcc/config/spu/spu.c
index a03b87e..7a29df8 100644
--- a/gcc/config/spu/spu.c
+++ b/gcc/config/spu/spu.c
@@ -4065,7 +4065,6 @@  spu_gimplify_va_arg_expr (tree valist, tree type, gimple_seq * pre_p,
   f_args = TYPE_FIELDS (TREE_TYPE (va_list_type_node));
   f_skip = DECL_CHAIN (f_args);
 
-  valist = build_simple_mem_ref (valist);
   args =
     build3 (COMPONENT_REF, TREE_TYPE (f_args), valist, f_args, NULL_TREE);
   skip =
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 4846478..7d53aa2 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -4655,13 +4655,13 @@  gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
       if (TREE_CODE (call) == CALL_EXPR
 	  && CALL_EXPR_IFN (call) == IFN_VA_ARG)
 	{
+	  int nargs = call_expr_nargs (call);
 	  tree type = TREE_TYPE (call);
 	  tree ap = CALL_EXPR_ARG (call, 0);
 	  tree tag = CALL_EXPR_ARG (call, 1);
-	  tree do_deref = CALL_EXPR_ARG (call, 2);
 	  tree newcall = build_call_expr_internal_loc (EXPR_LOCATION (call),
-						       IFN_VA_ARG, type, 4, ap,
-						       tag, do_deref,
+						       IFN_VA_ARG, type,
+						       nargs + 1, ap, tag,
 						       vlasize);
 	  tree *call_p = &(TREE_OPERAND (*from_p, 0));
 	  *call_p = newcall;
@@ -9312,7 +9312,7 @@  gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p,
   tree promoted_type, have_va_type;
   tree valist = TREE_OPERAND (*expr_p, 0);
   tree type = TREE_TYPE (*expr_p);
-  tree t, tag, ap, do_deref;
+  tree t, tag;
   location_t loc = EXPR_LOCATION (*expr_p);
 
   /* Verify that valist is of the proper type.  */
@@ -9365,35 +9365,8 @@  gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p,
       return GS_ALL_DONE;
     }
 
-  /* Transform a VA_ARG_EXPR into an VA_ARG internal function.  */
-  if (TREE_CODE (have_va_type) == ARRAY_TYPE)
-    {
-      if (TREE_CODE (TREE_TYPE (valist)) == ARRAY_TYPE)
-	{
-	  /* Take the address, but don't strip it.  Gimplify_va_arg_internal
-	     expects a pointer to array element type.  */
-	  ap = build_fold_addr_expr_loc (loc, valist);
-	  do_deref = integer_zero_node;
-	}
-      else
-	{
-	  /* Don't take the address.  Gimplify_va_arg_internal expects a pointer
-	     to array element type, and we already have that.
-	     See also comment in build_va_arg.  */
-	  ap = valist;
-	  do_deref = integer_zero_node;
-	}
-    }
-  else
-    {
-      /* No special handling.  Take the address here, note that it needs to be
-	 stripped before calling gimplify_va_arg_internal. */
-      ap = build_fold_addr_expr_loc (loc, valist);
-      do_deref = integer_one_node;
-    }
   tag = build_int_cst (build_pointer_type (type), 0);
-  *expr_p = build_call_expr_internal_loc (loc, IFN_VA_ARG, type, 3, ap, tag,
-					  do_deref);
+  *expr_p = build_call_expr_internal_loc (loc, IFN_VA_ARG, type, 2, valist, tag);
 
   /* Clear the tentatively set PROP_gimple_lva, to indicate that IFN_VA_ARG
      needs to be expanded.  */
diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c
index f8ff70a..794b94a 100644
--- a/gcc/tree-stdarg.c
+++ b/gcc/tree-stdarg.c
@@ -1042,7 +1042,7 @@  expand_ifn_va_arg_1 (function *fun)
     for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i))
       {
 	gimple stmt = gsi_stmt (i);
-	tree ap, expr, lhs, type, do_deref;
+	tree ap, expr, lhs, type;
 	gimple_seq pre = NULL, post = NULL;
 
 	if (!gimple_call_ifn_va_arg_p (stmt))
@@ -1052,19 +1052,15 @@  expand_ifn_va_arg_1 (function *fun)
 
 	type = TREE_TYPE (TREE_TYPE (gimple_call_arg (stmt, 1)));
 	ap = gimple_call_arg (stmt, 0);
-	do_deref = gimple_call_arg (stmt, 2);
 
-	if (do_deref == integer_one_node)
-	  ap = build_fold_indirect_ref (ap);
+	/* Balanced out the &ap, usually added by build_va_arg.  */
+	ap = build_fold_indirect_ref (ap);
 
 	push_gimplify_context (false);
 
 	/* Make it easier for the backends by protecting the valist argument
 	   from multiple evaluations.  */
-	if (do_deref == integer_one_node)
-	  gimplify_expr (&ap, &pre, &post, is_gimple_min_lval, fb_lvalue);
-	else
-	  gimplify_expr (&ap, &pre, &post, is_gimple_val, fb_rvalue);
+	gimplify_expr (&ap, &pre, &post, is_gimple_min_lval, fb_lvalue);
 
 	expr = targetm.gimplify_va_arg_expr (ap, type, &pre, &post);
 
@@ -1074,7 +1070,7 @@  expand_ifn_va_arg_1 (function *fun)
 	    unsigned int nargs = gimple_call_num_args (stmt);
 	    gcc_assert (useless_type_conversion_p (TREE_TYPE (lhs), type));
 
-	    if (nargs == 4)
+	    if (nargs == 3)
 	      {
 		/* We've transported the size of with WITH_SIZE_EXPR here as
 		   the last argument of the internal fn call.  Now reinstate
-- 
1.9.1