From patchwork Wed Apr 29 09:30:09 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 465956 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 483591402BD for ; Wed, 29 Apr 2015 19:30:28 +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=t/Hiw3zW; 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 :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=VDeJmZEHJPyT/VAGgf GEjkHBk43qhl8T290rxwB1j7vqyxs1bD5f7D8OIEZDHlnLzbv2aNN+FtJMizYG8a lqkcr4BuusrJt5r7ea2wbPYgQV88ejB6/0uGWmNLVPmSw/6lJ1gVzh4DrpDrgVvd VuNB3K+7dBmRqnMRNZac1jExo= 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 :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=N2ae4VJs6Vk9stM6XESxXtTS XDg=; b=t/Hiw3zWqnEfRfORKbPQRQYI3yP4HBxXS7fENw9L9bkx4nM0Q9LGcsIe VaMr6y8vsm7Pjq0g9lglXTgkOnTRN6VYlhUeBjQhU2lExf3qFgMma6w09p6rwd5Q XCQx81MtnEFEA8OuYZA6nbD4iGrySdzNzFQs2bzczzmIEdVY81E= Received: (qmail 76117 invoked by alias); 29 Apr 2015 09:30:20 -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 76075 invoked by uid 89); 29 Apr 2015 09:30:13 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-ob0-f169.google.com Received: from mail-ob0-f169.google.com (HELO mail-ob0-f169.google.com) (209.85.214.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Wed, 29 Apr 2015 09:30:11 +0000 Received: by obfe9 with SMTP id e9so15711409obf.1 for ; Wed, 29 Apr 2015 02:30:09 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.182.87.8 with SMTP id t8mr18123124obz.35.1430299809603; Wed, 29 Apr 2015 02:30:09 -0700 (PDT) Received: by 10.76.115.167 with HTTP; Wed, 29 Apr 2015 02:30:09 -0700 (PDT) In-Reply-To: <553FEDB6.20509@mentor.com> References: <553B89FB.7000009@mentor.com> <553E3451.5010300@mentor.com> <553E500B.2060505@mentor.com> <553FEDB6.20509@mentor.com> Date: Wed, 29 Apr 2015 11:30:09 +0200 Message-ID: Subject: Re: [PATCH][PR65818][bootstrap, hppa] Return side-effect free result in gimplify_va_arg_internal From: Richard Biener To: Tom de Vries Cc: GCC Patches , John David Anglin X-IsSubscribed: yes On Tue, Apr 28, 2015 at 10:29 PM, Tom de Vries wrote: > On 28-04-15 12:34, Richard Biener wrote: >> >> On Mon, Apr 27, 2015 at 5:04 PM, Tom de Vries >> wrote: >>> >>> On 27-04-15 15:40, Richard Biener wrote: >>>> >>>> >>>> On Mon, Apr 27, 2015 at 3:06 PM, Tom de Vries >>>> wrote: >>>>> >>>>> >>>>> On 27-04-15 10:17, Richard Biener wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> This patch fixes that by gimplifying the address expression of the >>>>>>> mem-ref >>>>>>>> >>>>>>>> >>>>>>>> returned by the target hook (borrowing code from gimplify_expr, case >>>>>>>> MEM_REF). >>>>>>>> >>>>>>>> Bootstrapped and reg-tested on x86_64. >>>>>>>> >>>>>>>> Bootstrapped and reg-tested on hppa2.0w-hp-hpux11.11. >>>>>>>> >>>>>>>> OK for trunk? >>>>>> >>>>>> >>>>>> >>>>>> Hmm, that assert looks suspicious... >>>>>> >>>>>> Can't you simply always do >>>>>> >>>>>> gimplify_expr (expr, pre_p, post_p, is_gimple_lvalue, fb_lvalue); >>>>> >>>>> >>>>> >>>>> >>>>> It's a bit counter-intuitive for me, using is_gimple_lvalue for >>>>> something >>>>> (the result of va_arg) we use as rvalue. >>>> >>>> >>>> >>>> Yeah, choosing that was done because you could assert it's a MEM_REF >>>> and tree-stdarg eventually builds a WITH_SIZE_EXPR around it. >>>> >>>> It would possibly be easier to simply "inline" gimplify_va_arg_internal >>>> at its use and only gimplify the assignment. Though in that case the >>>> original code probably worked - just in the lhs == NULL case it didn't, >>>> which hints at a better place for the fix - in expand_ifn_va_arg_1 do >>>> >>>> if (lhs != NULL_TREE) >>>> { >>>> ... >>>> } >>>> else >>>> gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either); >>>> >>>> So ... if you can re-try that one it's pre-approved. >>>> >>> >>> Using that proposed patch, we run into the following problem. >>> >>> Consider this testcase with ignored-result-va_arg: >>> ... >>> #include >>> >>> void >>> f2 (int i, ...) >>> { >>> va_list ap; >>> va_start (ap, i); >>> va_arg (ap, long); >>> va_end (ap); >>> } >>> ... >>> >>> after gimplify_va_arg_internal we have: >>> ... >>> (gdb) call debug_generic_expr (expr) >>> *(long int * {ref-all}) addr.2 >>> ... >>> >>> In a bit more detail: >>> ... >>> (gdb) call debug_tree (expr) >>> >> type >> size >>> unit size >>> align 64 symtab 0 alias set -1 canonical type 0x7ffff64a77e0 >>> precision 64 min max >>> >>> pointer_to_this > >>> >>> arg 0 >> type >> 0x7ffff64a77e0 >>> long int> >>> public static unsigned DI size >> 64> >>> unit size >>> align 64 symtab 0 alias set -1 canonical type >>> 0x7ffff65e0498> >>> >>> arg 0 >> 0x7ffff64c2150> >>> used unsigned ignored DI file stdarg-1.c line 4 col 1 size >>> unit size >>> align 64 context >> >>> arg 1 >>> constant 0>> >>> ... >>> >>> During 'gimplify_expr (&expr, &pre, &post, is_gimple_val, fb_either)' we >>> ICE >>> here: >>> ... >>> 8886 gcc_assert ((*gimple_test_f) (*expr_p)); >>> ... >>> >>> At this point expr is: >>> ... >>> (gdb) call debug_generic_expr (*expr_p) >>> *addr.2 >>> ... >>> >>> In more detail: >>> ... >>> (gdb) call debug_tree (*expr_p) >>> >> type >> align 8 symtab 0 alias set -1 canonical type 0x7ffff64c2000 >>> pointer_to_this > >>> >>> arg 0 >> type >> void> >>> sizes-gimplified public unsigned DI >>> size >>> unit size >>> align 64 symtab 0 alias set -1 canonical type 0x7ffff64c2150 >>> pointer_to_this > >>> used unsigned ignored DI file stdarg-1.c line 4 col 1 size >>> unit size >>> align 64 context > >>> arg 1 >>> constant 0>> >>> ... >>> >>> The memref is now VOID type. And that's not a gimple_val: >>> ... >>> (gdb) p is_gimple_val (*expr_p) >>> $1 = false >>> ... >> >> >> On which target? I can't seem to reproduce on x86_64 or i?86. I also >> can't see how this >> could happen. >> > > Reproduced using attached patch on top of r222537, for target x86_64, with > and without -m32. Ok, I see now. The gimplifier seems to be confused with fb_lvalue. all is_gimple_addressable ()s are already is_gimple_lvalue so should be enough for that case. OTOH is_gimple_val will not be true after this, so we'd still ICE later. Which means fb_lvalue doesn't make sense for is_gimple_val. So your patch looks ok. Richard. > Thanks, > - Tom > > >> Richard. >> >>> Attached patch uses is_gimple_lvalue, but in expand_ifn_va_arg_1. >>> >>> I'll bootstrap and reg-test on x86_64 and commit, unless further comments >>> of >>> course. >>> >>> Thanks, >>> - Tom > > Index: gcc/gimplify.c =================================================================== --- gcc/gimplify.c (revision 222537) +++ gcc/gimplify.c (working copy) @@ -8844,15 +8844,9 @@ gimplify_expr (tree *expr_p, gimple_seq rvalue. */ if ((fallback & fb_lvalue) && gimple_seq_empty_p (internal_post) - && is_gimple_addressable (*expr_p)) - { - /* An lvalue will do. Take the address of the expression, store it - in a temporary, and replace the expression with an INDIRECT_REF of - that temporary. */ - tmp = build_fold_addr_expr_loc (input_location, *expr_p); - gimplify_expr (&tmp, pre_p, post_p, is_gimple_reg, fb_rvalue); - *expr_p = build_simple_mem_ref (tmp); - } + && is_gimple_lvalue (*expr_p)) + /* An lvalue will do and we already have one. */ + ; else if ((fallback & fb_rvalue) && is_gimple_reg_rhs_or_call (*expr_p)) { /* An rvalue will do. Assign the gimplified expression into a