| Message ID | 5550C7DA.6070406@mentor.com |
|---|---|
| State | New |
| Headers | show |
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. Thanks, Richard.
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 378f237..95cf69b 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -5918,9 +5918,45 @@ set_compound_literal_name (tree decl) tree build_va_arg (location_t loc, tree expr, tree type) { - /* In gimplify_va_arg_expr we take the address of the ap argument, mark it - addressable now. */ - mark_addressable (expr); + tree va_type = TREE_TYPE (expr); + tree canon_va_type = (va_type == error_mark_node + ? NULL_TREE + : targetm.canonical_va_list_type (va_type)); + + if (va_type == error_mark_node + || canon_va_type == NULL_TREE) + /* Let's not bother about addressable or not, if expr: + - has parse errors, or + - is not an va_list type. */ + ; + else + { + if (POINTER_TYPE_P (va_type)) + { + /* If it's a pointer type, there are two possibilities: + - expr is a va_list param decl that is declared as an array type + but was turned into a pointer type by grok_declarator. + - expr is a va_list decl declared as pointer type. + Detect the former case by looking at the canonical type. */ + if (TREE_CODE (canon_va_type) == ARRAY_TYPE) + /* We know that the 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. */ + ; + else + /* The type is an actual va_list. It might be modified by va_arg, so + we need to pass it around using an address operator, so we need + to mark it addressable. */ + mark_addressable (expr); + } + else + { + /* The type is an actual va_list. It might be modified by va_arg, so + we need to pass it around using an address operator, so we need to + mark it addressable. */ + mark_addressable (expr); + } + } expr = build1 (VA_ARG_EXPR, type, expr); SET_EXPR_LOCATION (expr, loc);