This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][PR65823] Fix va_arg ap_copy nop detection
- From: Tom de Vries <Tom_deVries at mentor dot com>
- To: Richard Biener <richard dot guenther at gmail dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Tue, 28 Apr 2015 11:10:05 +0200
- Subject: Re: [PATCH][PR65823] Fix va_arg ap_copy nop detection
- Authentication-results: sourceware.org; auth=none
- References: <553750B2 dot 8010209 at mentor dot com> <CAFiYyc3vuDJh0sgjy7qX0sBQ-Eqpq4LJoutMMkxBa3-v5PNvag at mail dot gmail dot com> <5537A438 dot 5030803 at mentor dot com> <CAFiYyc1bJ51ynbWH_=M25wt_5pw7LGO_gys9D5s7pvqXt7XL=A at mail dot gmail dot com> <553DE900 dot 3040504 at mentor dot com>
On 27-04-15 09:45, Tom de Vries wrote:
On 22-04-15 15:50, Richard Biener wrote:
On Wed, Apr 22, 2015 at 3:38 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
On 22-04-15 10:06, Richard Biener wrote:
On Wed, Apr 22, 2015 at 9:41 AM, Tom de Vries <Tom_deVries@mentor.com>
wrote:
Hi,
this patch fixes PR65823.
<SNIP>
The patches fixes the problem by using operand_equal_p to do the equality
test.
Bootstrapped and reg-tested on x86_64.
Did minimal non-bootstrap build on arm and reg-tested.
OK for trunk?
Hmm, ok for now.
Committed.
But I wonder if we can't fix things to not require that
odd extra copy.
Agreed, that would be good.
In fact that we introduce ap.1 looks completely bogus to me
AFAICT, it's introduced by gimplify_arg ('&argp') because argp (a PARM_DECL)
is not addressable.
(and we don't in this case for arm). Note that the pointer compare
obviously
fails because we unshare the expression.
So ... what breaks if we simply remove this odd "fixup"?
[ Originally mentioned at https://gcc.gnu.org/ml/gcc/2015-02/msg00011.html .
]
I've committed gcc.target/x86_64/abi/callabi/vaarg-6.c specifically as a
minimal version of this problem.
If we remove the ap_copy fixup, at original we have:
...
;; Function do_cpy2 (null)
{
char * e;
char * e;
e = VA_ARG_EXPR <argp>;
e = VA_ARG_EXPR <argp>;
if (e != b)
{
abort ();
}
}
...
and after gimplify we have:
...
do_cpy2 (char * argp)
{
char * argp.1;
char * argp.2;
char * b.3;
char * e;
argp.1 = argp;
e = VA_ARG (&argp.1, 0B);
argp.2 = argp;
e = VA_ARG (&argp.2, 0B);
b.3 = b;
if (e != b.3) goto <D.1373>; else goto <D.1374>;
<D.1373>:
abort ();
<D.1374>:
}
...
The second VA_ARG uses &argp.2, which is a copy of argp, which is unmodified
by the first VA_ARG.
Using attached _proof-of-concept_ patch, I get callabi.exp working without
the ap_copy, still at the cost of one 'argp.1 = argp' copy though:
...
do_cpy2 (char * argp)
{
char * argp.1;
char * b.2;
char * e;
argp.1 = argp;
e = VA_ARG (&argp.1, 0B);
e = VA_ARG (&argp.1, 0B);
b.2 = b;
if (e != b.2) goto <D.1372>; else goto <D.1373>;
<D.1372>:
abort ();
<D.1373>:
}
...
But perhaps there's an easier way?
Hum, simply
Index: gcc/gimplify.c
===================================================================
--- gcc/gimplify.c (revision 222320)
+++ gcc/gimplify.c (working copy)
@@ -9419,6 +9419,7 @@ gimplify_va_arg_expr (tree *expr_p, gimp
}
/* Transform a VA_ARG_EXPR into an VA_ARG internal function. */
+ mark_addressable (valist);
ap = build_fold_addr_expr_loc (loc, valist);
tag = build_int_cst (build_pointer_type (type), 0);
*expr_p = build_call_expr_internal_loc (loc, IFN_VA_ARG, type, 2, ap, tag);
That sort of works, but causes other problems. Filed PR65887 - 'remove va_arg ap
copies' to track this issue.
this patch marks the va_arg ap argument in the frontend as addressable, and
removes the fixup.
Bootstrapped and reg-tested on x86_64.
OK for trunk?
Thanks,
- Tom
Remove ifn_va_arg ap fixup
2015-04-28 Tom de Vries <tom@codesourcery.com>
PR tree-optimization/65887
* gimplify.c (gimplify_modify_expr): Remove ifn_va_arg ap fixup.
* c-common.c (build_va_arg): Mark va_arg ap argument as addressable.
---
gcc/c-family/c-common.c | 1 +
gcc/gimplify.c | 16 ----------------
2 files changed, 1 insertion(+), 16 deletions(-)
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index 9797e17..d6a93d9 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -5910,6 +5910,7 @@ set_compound_literal_name (tree decl)
tree
build_va_arg (location_t loc, tree expr, tree type)
{
+ mark_addressable (expr);
expr = build1 (VA_ARG_EXPR, type, expr);
SET_EXPR_LOCATION (expr, loc);
return expr;
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index c68bd47..1d5341e 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -4569,7 +4569,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
gimple assign;
location_t loc = EXPR_LOCATION (*expr_p);
gimple_stmt_iterator gsi;
- tree ap = NULL_TREE, ap_copy = NULL_TREE;
gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR
|| TREE_CODE (*expr_p) == INIT_EXPR);
@@ -4730,16 +4729,12 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
enum internal_fn ifn = CALL_EXPR_IFN (*from_p);
auto_vec<tree> vargs (nargs);
- if (ifn == IFN_VA_ARG)
- ap = unshare_expr (CALL_EXPR_ARG (*from_p, 0));
for (i = 0; i < nargs; i++)
{
gimplify_arg (&CALL_EXPR_ARG (*from_p, i), pre_p,
EXPR_LOCATION (*from_p));
vargs.quick_push (CALL_EXPR_ARG (*from_p, i));
}
- if (ifn == IFN_VA_ARG)
- ap_copy = CALL_EXPR_ARG (*from_p, 0);
call_stmt = gimple_build_call_internal_vec (ifn, vargs);
gimple_set_location (call_stmt, EXPR_LOCATION (*expr_p));
}
@@ -4784,17 +4779,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
gsi = gsi_last (*pre_p);
maybe_fold_stmt (&gsi);
- /* When gimplifying the &ap argument of va_arg, we might end up with
- ap.1 = ap
- va_arg (&ap.1, 0B)
- We need to assign ap.1 back to ap, otherwise va_arg has no effect on
- ap. */
- if (ap != NULL_TREE
- && TREE_CODE (ap) == ADDR_EXPR
- && TREE_CODE (ap_copy) == ADDR_EXPR
- && !operand_equal_p (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), 0))
- gimplify_assign (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), pre_p);
-
if (want_value)
{
*expr_p = TREE_THIS_VOLATILE (*to_p) ? *from_p : unshare_expr (*to_p);
--
1.9.1