From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 16162 invoked by alias); 5 Jul 2010 09:43:45 -0000 Received: (qmail 16153 invoked by uid 22791); 5 Jul 2010 09:43:44 -0000 X-SWARE-Spam-Status: No, hits=-5.9 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from cantor.suse.de (HELO mx1.suse.de) (195.135.220.2) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 05 Jul 2010 09:43:37 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.221.2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.suse.de (Postfix) with ESMTP id 41A9894608; Mon, 5 Jul 2010 11:43:35 +0200 (CEST) Date: Mon, 05 Jul 2010 09:43:00 -0000 From: Richard Guenther To: Jan Hubicka Cc: gcc-patches@gcc.gnu.org Subject: Re: PR middle-end/44813 (ICE in Mozilla build in ptr_deref_may_alias_decl_p) In-Reply-To: <20100704224222.GB29130@kam.mff.cuni.cz> Message-ID: References: <20100704224222.GB29130@kam.mff.cuni.cz> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2010-07/txt/msg00358.txt.bz2 On Mon, 5 Jul 2010, Jan Hubicka wrote: > Hi, > the testcase in PR (Richard is running minimizing on it) produce ICE because > ipa-split produce invalid gimple > * = funcall.part (); > where is not gimple register. It is function returning by reference > so we actually must write to retval directly for correctness. is not > gimple register because needs_to_live_in_memory returns true after calling > aggregate_value_p. > > This is confused. For functions returning by reference retval is pointer and > as such it does not need to live in memory. aggregate_value_p is about return > value itself, that is * that lives in memory. > > So this patch changes behaviour of needs_to_live_in_memory that has some consequences. > > 1) We get confused wanrings since tree-ssa-uninit.c considers retvals not initilized > at function entry. This is not valid for DECL_BY_REFERENCE> > > 2) tree-inline produces bogus &tmp_1=&tmp_1 for return statement, since it no longer > recognizes return value wrapped in SSA name. > > 3) Alias analysis don't know that .filed stores escape and thus must > be preserved > > 4) Statement verifier does type checking when return statement is not retval. > It should check DECL_BY_REFERENCE, but Richard plans to submit patch for it. > So I just patched around strengthening the check for SSA name of DECL_BY_REFERENCE> > > Bootstrapped/regtested x86_64 and tested on Mozilla build. OK? > > Honza > > PR middle-end/44813 > * tree-ssa-uninit.c (ssa_undefined_value_p): Retval_expr is initialized > for functions returning by reference. > * tree.c (needs_to_live_in_memory): Retval for functions returning by > reference does not need to live in memory. > * tree-inline.c (remap_gimple_stmt): Do not produce assignment > for functions returning by reference. > * tree-ssa-structalias.c (find_what_p_points_to): Look into result decls > same was as into parm decls. > * tree-cfg.c (verify_gimple_return): Expect RESULT_DECL to have SSA name. > Index: tree-ssa-uninit.c > =================================================================== > --- tree-ssa-uninit.c (revision 161774) > +++ tree-ssa-uninit.c (working copy) > @@ -92,6 +92,12 @@ ssa_undefined_value_p (tree t) > if (TREE_CODE (var) == PARM_DECL) > return false; > > + /* When returning by reference the return address is actually a hidden > + parameter. */ > + if (TREE_CODE (SSA_NAME_VAR (t)) == RESULT_DECL > + && DECL_BY_REFERENCE (SSA_NAME_VAR (t))) > + return false; > + > /* Hard register variables get their initial value from the ether. */ > if (TREE_CODE (var) == VAR_DECL && DECL_HARD_REGISTER (var)) > return false; This change is ok. > Index: tree.c > =================================================================== > --- tree.c (revision 161774) > +++ tree.c (working copy) > @@ -9750,6 +9750,7 @@ needs_to_live_in_memory (const_tree t) > return (TREE_ADDRESSABLE (t) > || is_global_var (t) > || (TREE_CODE (t) == RESULT_DECL > + && !DECL_BY_REFERENCE (t) > && aggregate_value_p (t, current_function_decl))); > } Likewise. > Index: tree-inline.c > =================================================================== > --- tree-inline.c (revision 161774) > +++ tree-inline.c (working copy) > @@ -1236,7 +1237,11 @@ remap_gimple_stmt (gimple stmt, copy_bod > If RETVAL is just the result decl, the result decl has > already been set (e.g. a recent "foo (&result_decl, ...)"); > just toss the entire GIMPLE_RETURN. */ > - if (retval && TREE_CODE (retval) != RESULT_DECL) > + if (retval > + && (TREE_CODE (retval) != RESULT_DECL > + && (TREE_CODE (retval) != SSA_NAME > + || !SSA_NAME_IS_DEFAULT_DEF (retval) > + || TREE_CODE (SSA_NAME_VAR (retval)) != RESULT_DECL))) We should never see a defintion of a RESULT_DECL SSA name for DECL_BY_REFERENCE RESULT_DECLs (that would be a bug - we should add verification to the SSA verifier, can you do add that?). So the || !SSA_NAME_IS_DEFAULT_DEF (retval) can be dropped here. This patch is ok with that change together with an addition to the SSA verifier. > { > copy = gimple_build_assign (id->retvar, retval); > /* id->retvar is already substituted. Skip it on later remapping. */ > Index: tree-ssa-structalias.c > =================================================================== > --- tree-ssa-structalias.c (revision 161774) > +++ tree-ssa-structalias.c (working copy) > @@ -5751,7 +5751,8 @@ find_what_p_points_to (tree p) > /* For parameters, get at the points-to set for the actual parm > decl. */ > if (TREE_CODE (p) == SSA_NAME > - && TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL > + && (TREE_CODE (SSA_NAME_VAR (p)) == PARM_DECL > + || TREE_CODE (SSA_NAME_VAR (p)) == RESULT_DECL) > && SSA_NAME_IS_DEFAULT_DEF (p)) > lookup_p = SSA_NAME_VAR (p); That doesn't look correct on its own. In fact get_constraint_for_ssa_var needs a similar adjustment, likewise for consistency (probably doesn't happen here though) get_fi_for_callee. The patch is ok with those changes. > Index: tree-cfg.c > =================================================================== > --- tree-cfg.c (revision 161774) > +++ tree-cfg.c (working copy) > @@ -3818,8 +3818,7 @@ verify_gimple_return (gimple stmt) > if (op == NULL) > return false; > > - if (!is_gimple_val (op) > - && TREE_CODE (op) != RESULT_DECL) > + if (!is_gimple_val (op) && TREE_CODE (op) != RESULT_DECL) > { > error ("invalid operand in return statement"); > debug_generic_stmt (op); > @@ -3829,7 +3828,10 @@ verify_gimple_return (gimple stmt) > if (!useless_type_conversion_p (restype, TREE_TYPE (op)) > /* ??? With C++ we can have the situation that the result > decl is a reference type while the return type is an aggregate. */ > - && !(TREE_CODE (op) == RESULT_DECL > + && !((TREE_CODE (op) == RESULT_DECL > + || (TREE_CODE (op) == SSA_NAME > + && SSA_NAME_IS_DEFAULT_DEF (op) > + && TREE_CODE (SSA_NAME_VAR (op)) == RESULT_DECL)) > && TREE_CODE (TREE_TYPE (op)) == REFERENCE_TYPE > && useless_type_conversion_p (restype, TREE_TYPE (TREE_TYPE (op))))) > { I have committed a change similar to this already. Thanks, Richard.