From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 10035 invoked by alias); 17 Apr 2011 19:44:16 -0000 Received: (qmail 10023 invoked by uid 22791); 17 Apr 2011 19:44:14 -0000 X-SWARE-Spam-Status: No, hits=-5.8 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; Sun, 17 Apr 2011 19:43:58 +0000 Received: from relay2.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 80B959486B; Sun, 17 Apr 2011 21:43:56 +0200 (CEST) Date: Sun, 17 Apr 2011 21:27:00 -0000 From: Richard Guenther To: Jan Hubicka Cc: gcc-patches@gcc.gnu.org, mjambor@suse.cz Subject: Re: Remove vtable_method field in cgraph_node In-Reply-To: <20110417163459.GH5273@kam.mff.cuni.cz> Message-ID: References: <20110417163459.GH5273@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: 2011-04/txt/msg01340.txt.bz2 On Sun, 17 Apr 2011, Jan Hubicka wrote: > Hi, > this patch drops vtable_method filed. I never understood what it is about but reading > PR20991 I am convinced that we hit the same problem with work on new devirutalization code. > I implemented what Mark describe as "ideal solution" there - i.e. teaching cgraph that > virtual functions might become reachable until after inlining. Since we still can > devirutalize in late compilation (that is pretty much poinless but anyway), we no > ahve can_refer_decl_in_current_unit_p that tests if the function has been already thrown > away. Perhaps we might apply there the observation about vtable being output somewhere, > but I do not think it is safe: if vtable is COMDAT, we can pretty much also optimize > all references to it out in all compilation unit and up not outputting it. > When vtable is not COMDAT, the methods won't be either and this trick will not apply. > > Consequently I am dropping the flag. This is very trivial based on observation > that cp_fold_obj_type_ref, the only setter of the flag, is now dead. Plus the > varasm code is no-longer executed at the time of IPA optimizations that the original > patch was fixing. > > Martin, > can you please look into why cp_fold_obj_type_ref is no longer used and if possible > get rid of it? > > Richi, > compiling the original testcase: > // PR middle-end/20991 > // { dg-options "-O2" } > // { dg-do compile } > > struct S > { > virtual inline int foo () const; > virtual inline bool bar () const; > virtual int baz (int) const; > }; > > inline int S::foo () const > { > return 1; > } > > inline bool S::bar () const > { > return foo () == 0; > } > > void A () > { > S s; > if (s.bar ()) > s.foo (); > } > > void B () > { > S s; > if (s.bar ()) > s.foo (); > } > > we end up with bit amusing code: > void B() () > { > int D.2147; > struct S s; > > : > s._vptr.S = &_ZTV1S[2]; > D.2147_8 = OBJ_TYPE_REF(foo;&s->0) (&s); > return; > > } > > notice that OBJ_TYPE_REF is useless since foo got devitualized and it holds alive > the useless s._vptr.S = &_ZTV1S[2]. We want to drop OBJ_TYPE_REF at time we fold > first argument to constant. At some point we dropped all OBJ_TYPE_REF with direct fn but that lead to issues. I'll try to dig up what that was. Richard. > a.C.027t.fre1 dump reads: > void B() () > { > int (*__vtbl_ptr_type) () * D.2149; > int (*__vtbl_ptr_type) () D.2148; > int D.2147; > bool D.2146; > struct S s; > > : > s._vptr.S = &_ZTV1S[2]; > D.2149_6 = &_ZTV1S[2]; > D.2148_7 = foo; > D.2147_8 = OBJ_TYPE_REF(D.2148_7;&s->0) (&s); > D.2146_9 = D.2147_8 == 0; > D.2146_12 = D.2146_9; > D.2146_1 = D.2146_12; > D.2146_2 = D.2146_1; > return; > > } > > already I would expect FRE to replace D.2148_7 by foo. Later this is done by copyprop1 > > Folding statement: D.2147_8 = OBJ_TYPE_REF(D.2148_7;&s->0) (&s); > Folded into: D.2147_8 = OBJ_TYPE_REF(foo;&s->0) (&s); > > void B() () > { > int (*__vtbl_ptr_type) () * D.2149; > int (*__vtbl_ptr_type) () D.2148; > int D.2147; > bool D.2146; > struct S s; > > : > s._vptr.S = &_ZTV1S[2]; > D.2147_8 = OBJ_TYPE_REF(foo;&s->0) (&s); > return; > > } > > When copyprop chose to do constant propagation for whatever reason, it probably should > do the folding, too. Either in substitute_and_fold or via its own ccp_fold_stmt. I am sure > you know the best alternatives better than I do ;) > > Honza > > * cgraph.c (cgraph_clone_node): Do not handle vtable_method > * cgraph.h (struct cgraph_local_info): Drop vtable_method. > * cgraphunit.c (cgraph_copy_node_for_versioning): Drop vtable_method. > * lto-cgraph.c (lto_output_node, input_overwrite_node): Drop vtable method. > * gimple-fold.c (can_refer_decl_in_current_unit_p): Mention PR20991 in > gimple-fold.c > * varasm.c (mark_decl_referenced): Drop vtable_method handling code. > * cp/class.c (cp_fold_obj_type_ref): Drop vtable_method. > Index: cgraph.c > =================================================================== > *** cgraph.c (revision 172608) > --- cgraph.c (working copy) > *************** cgraph_clone_node (struct cgraph_node *n > *** 2161,2167 **** > new_node->local = n->local; > new_node->local.externally_visible = false; > new_node->local.local = true; > - new_node->local.vtable_method = false; > new_node->global = n->global; > new_node->rtl = n->rtl; > new_node->count = count; > --- 2161,2166 ---- > Index: cgraph.h > =================================================================== > *** cgraph.h (revision 172608) > --- cgraph.h (working copy) > *************** struct GTY(()) cgraph_local_info { > *** 95,104 **** > /* True when the function has been originally extern inline, but it is > redefined now. */ > unsigned redefined_extern_inline : 1; > - > - /* True if the function is going to be emitted in some other translation > - unit, referenced from vtable. */ > - unsigned vtable_method : 1; > }; > > /* Information about the function that needs to be computed globally > --- 95,100 ---- > Index: cgraphunit.c > =================================================================== > *** cgraphunit.c (revision 172608) > --- cgraphunit.c (working copy) > *************** cgraph_copy_node_for_versioning (struct > *** 1991,1997 **** > new_version->local = old_version->local; > new_version->local.externally_visible = false; > new_version->local.local = true; > - new_version->local.vtable_method = false; > new_version->global = old_version->global; > new_version->rtl = old_version->rtl; > new_version->reachable = true; > --- 1991,1996 ---- > Index: cp/class.c > =================================================================== > *** cp/class.c (revision 172608) > --- cp/class.c (working copy) > *************** cp_fold_obj_type_ref (tree ref, tree kno > *** 8402,8409 **** > DECL_VINDEX (fndecl))); > #endif > > - cgraph_get_node (fndecl)->local.vtable_method = true; > - > return build_address (fndecl); > } > > --- 8402,8407 ---- > Index: lto-cgraph.c > =================================================================== > *** lto-cgraph.c (revision 172608) > --- lto-cgraph.c (working copy) > *************** lto_output_node (struct lto_simple_outpu > *** 491,497 **** > bp_pack_value (&bp, node->local.finalized, 1); > bp_pack_value (&bp, node->local.can_change_signature, 1); > bp_pack_value (&bp, node->local.redefined_extern_inline, 1); > - bp_pack_value (&bp, node->local.vtable_method, 1); > bp_pack_value (&bp, node->needed, 1); > bp_pack_value (&bp, node->address_taken, 1); > bp_pack_value (&bp, node->abstract_and_needed, 1); > --- 491,496 ---- > *************** input_overwrite_node (struct lto_file_de > *** 927,933 **** > node->local.finalized = bp_unpack_value (bp, 1); > node->local.can_change_signature = bp_unpack_value (bp, 1); > node->local.redefined_extern_inline = bp_unpack_value (bp, 1); > - node->local.vtable_method = bp_unpack_value (bp, 1); > node->needed = bp_unpack_value (bp, 1); > node->address_taken = bp_unpack_value (bp, 1); > node->abstract_and_needed = bp_unpack_value (bp, 1); > --- 926,931 ---- > Index: gimple-fold.c > =================================================================== > *** gimple-fold.c (revision 172608) > --- gimple-fold.c (working copy) > *************** can_refer_decl_in_current_unit_p (tree d > *** 80,86 **** > return true; > /* We are not at ltrans stage; so don't worry about WHOPR. > Also when still gimplifying all referred comdat functions will be > ! produced. */ > if (!flag_ltrans && (!DECL_COMDAT (decl) || !cgraph_function_flags_ready)) > return true; > /* If we already output the function body, we are safe. */ > --- 80,89 ---- > return true; > /* We are not at ltrans stage; so don't worry about WHOPR. > Also when still gimplifying all referred comdat functions will be > ! produced. > ! ??? as observed in PR20991 for already optimized out comdat virtual functions > ! we may not neccesarily give up because the copy will be output elsewhere when > ! corresponding vtable is output. */ > if (!flag_ltrans && (!DECL_COMDAT (decl) || !cgraph_function_flags_ready)) > return true; > /* If we already output the function body, we are safe. */ > Index: varasm.c > =================================================================== > *** varasm.c (revision 172608) > --- varasm.c (working copy) > *************** mark_decl_referenced (tree decl) > *** 2201,2208 **** > definition. */ > struct cgraph_node *node = cgraph_get_create_node (decl); > if (!DECL_EXTERNAL (decl) > ! && (!node->local.vtable_method || !cgraph_global_info_ready > ! || !node->local.finalized)) > cgraph_mark_needed_node (node); > } > else if (TREE_CODE (decl) == VAR_DECL) > --- 2201,2207 ---- > definition. */ > struct cgraph_node *node = cgraph_get_create_node (decl); > if (!DECL_EXTERNAL (decl) > ! && !node->local.finalized) > cgraph_mark_needed_node (node); > } > else if (TREE_CODE (decl) == VAR_DECL) > > -- Richard Guenther Novell / SUSE Labs SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex