On Tue, 12 May 2015, Tom de Vries wrote:
> 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.
That would certainly be a nice cleanup.
Richard.
> 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;
> }
>
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham
Norton, HRB 21284 (AG Nuernberg)
