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.
Thanks,
- Tom
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);
