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;
}
