| Message ID | 5551D014.3040205@mentor.com |
|---|---|
| State | New |
| Headers | show |
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; > } > > >
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; }