From patchwork Tue Apr 28 09:10:05 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tom de Vries X-Patchwork-Id: 465437 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4ECED14007D for ; Tue, 28 Apr 2015 19:10:26 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass reason="1024-bit key; unprotected key" header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=aF+PJydu; dkim-adsp=none (unprotected policy); dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=CbCE2s9PA1Qer9Wkx HPNSCdY9Y8CvAprDYfdGDkh6TPe4POuos5PrCieyytvleeJci1jdZvnEd7inZMaO 8o3Z4YJjSu2k7J5FkfuNNAM/B542KHFqsaeWZs8JfUcXbLDLbAB1LKD5G9h0U64R hesBTcs0WRoNsZhrdmWwxqSvGM= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=0mG4oXFR0kpsBpiTC+XObsh 4xs4=; b=aF+PJydujaOma6ayYSbnwgebCB13sjDWblMfTBO92uAVISEBzs1cMPX GfbyZS4QPgWw82h6/YVnumOuOKIEjqD9YFrJLyp5N9RAPPqxG6qXSVPmqWT9ahuf SrXy1vZmg7rK04SdD+hVNMKiLp7WS0u1H+fkQqTeHq/uhNR+/YBs= Received: (qmail 13780 invoked by alias); 28 Apr 2015 09:10:16 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 13757 invoked by uid 89); 28 Apr 2015 09:10:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 28 Apr 2015 09:10:13 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-FEM-02.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1Yn1Wr-0004xu-MV from Tom_deVries@mentor.com ; Tue, 28 Apr 2015 02:10:10 -0700 Received: from [127.0.0.1] (137.202.0.76) by SVR-IES-FEM-02.mgc.mentorg.com (137.202.0.106) with Microsoft SMTP Server id 14.3.224.2; Tue, 28 Apr 2015 10:10:08 +0100 Message-ID: <553F4E6D.8000304@mentor.com> Date: Tue, 28 Apr 2015 11:10:05 +0200 From: Tom de Vries User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Richard Biener CC: GCC Patches Subject: Re: [PATCH][PR65823] Fix va_arg ap_copy nop detection References: <553750B2.8010209@mentor.com> <5537A438.5030803@mentor.com> <553DE900.3040504@mentor.com> In-Reply-To: <553DE900.3040504@mentor.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 wrote: >>> On 22-04-15 10:06, Richard Biener wrote: >>>> >>>> On Wed, Apr 22, 2015 at 9:41 AM, Tom de Vries >>>> wrote: >>>>> >>>>> Hi, >>>>> >>>>> this patch fixes PR65823. >>>>> >>> >>> >>> >>>>> >>>>> 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 ; >>> e = VA_ARG_EXPR ; >>> 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 ; else goto ; >>> : >>> abort (); >>> : >>> } >>> ... >>> >>> 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 ; else goto ; >>> : >>> abort (); >>> : >>> } >>> ... >>> >>> 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 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 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