| Message ID | 553F4E6D.8000304@mentor.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 28, 2015 at 11:10 AM, Tom de Vries <Tom_deVries@mentor.com> wrote: > 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? Ok with adding a comment to build_va_arg that gimplification later wants to take the address of expr. Thanks, Richard. > 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