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

[Tom de Vries <Tom_deVries at mentor dot com>]

On 12-05-15 09:45, Richard Biener wrote:
> On Mon, 11 May 2015, Tom de Vries wrote:
>
> > On 11-05-15 09:47, Richard Biener wrote:
> > > > Bootstrapped and reg-tested on x86_64, with and without -m32.
> > > > > OK for trunk?
> > > > >
> > > > > [ FWIW, I suspect this patch will make life easier for the
> > > > reimplementation of
> > > > > the pass_stdarg optimization using ifn_va_arg. ]
> > > +  if (canon_va_type != NULL)
> > > +    {
> > > +      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);
> > >
> > > can we "simplify" this and ...
> > >
> > > -       }
> > > -
> > > +      gcc_assert (TREE_CODE (TREE_TYPE (valist)) != ARRAY_TYPE);
> > >          gimplify_expr (&valist, pre_p, post_p, is_gimple_val, fb_rvalue);
> > >
> > > this to use [!]POINTER_TYPE_P ()?
> >
> > I'm not sure.
> >
> > > Why do we arrive with non-array
> > > va_type but array canon_va_type in build_va_arg?
> >
> > grokdeclarator in c-decl.c:
> > ...
> >          /* A parameter declared as an array of T is really a pointer to T.
> >             One declared as a function is really a pointer to a function.  */
> >
> >          if (TREE_CODE (type) == ARRAY_TYPE)
> >            {
> >              /* Transfer const-ness of array into that of type pointed to.  */
> >              type = TREE_TYPE (type);
> >              if (type_quals)
> >                type = c_build_qualified_type (type, type_quals);
> >              type = c_build_pointer_type (type);
> > ...
> >
> > > I suppose the
> > > va_list argument already decayed to a pointer then
> >
> > The above means that the va_list function parameter decayed to a pointer.
> > AFAIU, the va_list argument to va_arg just uses the same type (for parsing,
> > grep for RID_VA_ARG in c-parser.c).
> >
> > > (in which case
> > > the argument should already be addressable?)?
> >
> > The argument is of pointer type. That pointer-typed-argument will only be
> > addressable if we take the address of it, which is precisely the thing we're
> > trying to avoid in this patch.
> >
> > > I think the overall idea of the patch is good - I'm just worried about
> > > special-casing of ARRAY_TYPE vs. non-pointer-type (because it's the
> > > latter that we ultimately want...).
> >
> > AFAIU, the special casing of ARRAY_TYPE in the patch is a consequence of the
> > special-casing of ARRAY_TYPE as a parameter.
> >
> > I don't see how [!]POINTER_TYPE_P () can help here. I've rewritten and
> > attached the build_va_arg bit using POINTER_TYPE_P and expanded comments
> > a bit to demonstrate.
>
> Ah, ok.
>
> The patch is ok.

Committed with comments below added.

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.

Thanks,
- Tom

diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index c2aa1ca..9ff789e 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -5925,6 +5925,12 @@ build_va_arg (location_t loc, tree expr, tree type)

   if (canon_va_type != NULL)
     {
+      /* 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
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 7ca1374..8ad32ac 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -9404,7 +9404,8 @@ gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p,
       else
        {
          /* Don't take the address.  Gimplify_va_arg_internal expects a pointer
-            to array element type, and we already have that.  */
+            to array element type, and we already have that.
+            See also comment in build_va_arg.  */
          ap = valist;
          do_deref = integer_zero_node;
        }