public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* Remove vtable_method field in cgraph_node
@ 2011-04-17 17:26 Jan Hubicka
  2011-04-17 21:27 ` Richard Guenther
  2011-04-20 13:43 ` Martin Jambor
  0 siblings, 2 replies; 8+ messages in thread
From: Jan Hubicka @ 2011-04-17 17:26 UTC (permalink / raw)
  To: gcc-patches, rguenther, mjambor

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;

<bb 2>:
  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.

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;

<bb 2>:
  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;

<bb 2>:
  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)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Remove vtable_method field in cgraph_node
  2011-04-17 17:26 Remove vtable_method field in cgraph_node Jan Hubicka
@ 2011-04-17 21:27 ` Richard Guenther
  2011-04-18  0:51   ` Jan Hubicka
  2011-04-20 13:43 ` Martin Jambor
  1 sibling, 1 reply; 8+ messages in thread
From: Richard Guenther @ 2011-04-17 21:27 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: gcc-patches, mjambor

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;
> 
> <bb 2>:
>   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;
> 
> <bb 2>:
>   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;
> 
> <bb 2>:
>   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 <rguenther@suse.de>
Novell / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746 - GF: Markus Rex

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Remove vtable_method field in cgraph_node
  2011-04-17 21:27 ` Richard Guenther
@ 2011-04-18  0:51   ` Jan Hubicka
  2011-04-18  9:27     ` Richard Guenther
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Hubicka @ 2011-04-18  0:51 UTC (permalink / raw)
  To: Richard Guenther; +Cc: Jan Hubicka, gcc-patches, mjambor

> > 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.

Hah, that sounds really scary, given that OBJ_TYPE_REF should be semantically no-op.
We definitely drop them in tree-ssa-ccp, so if we need to preserve them somehow, that is
probably case where we should.

Honza

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Remove vtable_method field in cgraph_node
  2011-04-18  0:51   ` Jan Hubicka
@ 2011-04-18  9:27     ` Richard Guenther
  2011-04-18 15:40       ` Jan Hubicka
  0 siblings, 1 reply; 8+ messages in thread
From: Richard Guenther @ 2011-04-18  9:27 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: gcc-patches, mjambor

On Mon, 18 Apr 2011, Jan Hubicka wrote:

> > > 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.
> 
> Hah, that sounds really scary, given that OBJ_TYPE_REF should be semantically no-op.
> We definitely drop them in tree-ssa-ccp, so if we need to preserve them somehow, that is
> probably case where we should.

Ah, no.  We _did_ that in CCP but now we only adjust the OBJ_TYPE_REF
expr in CCP and defer to fold_stmt to eventually "devirtualize" it.
See PR45878.  Then rev.165435 was necessary, as we dropped OBJ_TYPE_REF
for the non-devirtualized call as well.  As both cases were because
of type conversion issues this should be fixed with separating the
call function type as we do now.

I'll look into handling copyprop and FRE similarly.

Richard.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Remove vtable_method field in cgraph_node
  2011-04-18  9:27     ` Richard Guenther
@ 2011-04-18 15:40       ` Jan Hubicka
  2011-04-18 15:41         ` Richard Guenther
  0 siblings, 1 reply; 8+ messages in thread
From: Jan Hubicka @ 2011-04-18 15:40 UTC (permalink / raw)
  To: Richard Guenther; +Cc: Jan Hubicka, gcc-patches, mjambor

> Ah, no.  We _did_ that in CCP but now we only adjust the OBJ_TYPE_REF
> expr in CCP and defer to fold_stmt to eventually "devirtualize" it.
> See PR45878.  Then rev.165435 was necessary, as we dropped OBJ_TYPE_REF
> for the non-devirtualized call as well.  As both cases were because

Hmm, sounds slipperly.
In any case OBJ_TYPE_REF of constant argument should be banned, or
we would need to extend gimple_call_fndecl to return contained fndecl.
Devirtualization without enabling an inlining is essentially useless transform
and it is precisely what we do now on this testcase...
> of type conversion issues this should be fixed with separating the
> call function type as we do now.
> 
> I'll look into handling copyprop and FRE similarly.

Thanks,  I would not even mind having some automatic check that the constant
OBJ_TYPE_REFs don't leak at cgraph edge construction time...
Honza
> 
> Richard.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Remove vtable_method field in cgraph_node
  2011-04-18 15:40       ` Jan Hubicka
@ 2011-04-18 15:41         ` Richard Guenther
  2011-04-18 16:27           ` Jan Hubicka
  0 siblings, 1 reply; 8+ messages in thread
From: Richard Guenther @ 2011-04-18 15:41 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: gcc-patches, mjambor

On Mon, 18 Apr 2011, Jan Hubicka wrote:

> > Ah, no.  We _did_ that in CCP but now we only adjust the OBJ_TYPE_REF
> > expr in CCP and defer to fold_stmt to eventually "devirtualize" it.
> > See PR45878.  Then rev.165435 was necessary, as we dropped OBJ_TYPE_REF
> > for the non-devirtualized call as well.  As both cases were because
> 
> Hmm, sounds slipperly.
> In any case OBJ_TYPE_REF of constant argument should be banned, or
> we would need to extend gimple_call_fndecl to return contained fndecl.

Yeah, I thought about this as well ...

> Devirtualization without enabling an inlining is essentially useless transform
> and it is precisely what we do now on this testcase...

Well, it transforms an indirect into a direct call (unless we handle
direct OBJ_TYPE_REF calls in gimple_call_fndecl of course).

> > of type conversion issues this should be fixed with separating the
> > call function type as we do now.
> > 
> > I'll look into handling copyprop and FRE similarly.
> 
> Thanks,  I would not even mind having some automatic check that the constant
> OBJ_TYPE_REFs don't leak at cgraph edge construction time...
> Honza

I still have no idea what this OBJ_TYPE_REF is for and why we need to
preserve it ... (apart from for the weirt type-based devirtualization).

Richard.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Remove vtable_method field in cgraph_node
  2011-04-18 15:41         ` Richard Guenther
@ 2011-04-18 16:27           ` Jan Hubicka
  0 siblings, 0 replies; 8+ messages in thread
From: Jan Hubicka @ 2011-04-18 16:27 UTC (permalink / raw)
  To: Richard Guenther; +Cc: Jan Hubicka, gcc-patches, mjambor

> On Mon, 18 Apr 2011, Jan Hubicka wrote:
> 
> > > Ah, no.  We _did_ that in CCP but now we only adjust the OBJ_TYPE_REF
> > > expr in CCP and defer to fold_stmt to eventually "devirtualize" it.
> > > See PR45878.  Then rev.165435 was necessary, as we dropped OBJ_TYPE_REF
> > > for the non-devirtualized call as well.  As both cases were because
> > 
> > Hmm, sounds slipperly.
> > In any case OBJ_TYPE_REF of constant argument should be banned, or
> > we would need to extend gimple_call_fndecl to return contained fndecl.
> 
> Yeah, I thought about this as well ...
> 
> > Devirtualization without enabling an inlining is essentially useless transform
> > and it is precisely what we do now on this testcase...
> 
> Well, it transforms an indirect into a direct call (unless we handle
> direct OBJ_TYPE_REF calls in gimple_call_fndecl of course).

Well, I meant that producing OBJ_TYPE_REF(foo,XY) is useless as long as
callgraph will not notice as a direct call.  So either we want
gimple_call_fndecl to look into OBJ_TYPE_REF or we want to sanity check that no
indirect edges in callgraph have this form.  In general it does not matter much
if we emit direct or indirect call on modern hardware, but we are better to be
sure that devirtualization mahcinery leads to calls we can inline.
> 
> > > of type conversion issues this should be fixed with separating the
> > > call function type as we do now.
> > > 
> > > I'll look into handling copyprop and FRE similarly.
> > 
> > Thanks,  I would not even mind having some automatic check that the constant
> > OBJ_TYPE_REFs don't leak at cgraph edge construction time...
> > Honza
> 
> I still have no idea what this OBJ_TYPE_REF is for and why we need to
> preserve it ... (apart from for the weirt type-based devirtualization).

Weirdness seems to be nature of devirtualization in C++ :(, but we should find
way how to be effective on it...
I would be surprised if OBJ_TYPE_REF of an constant paid any difference
for it.

Honza
> 
> Richard.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: Remove vtable_method field in cgraph_node
  2011-04-17 17:26 Remove vtable_method field in cgraph_node Jan Hubicka
  2011-04-17 21:27 ` Richard Guenther
@ 2011-04-20 13:43 ` Martin Jambor
  1 sibling, 0 replies; 8+ messages in thread
From: Martin Jambor @ 2011-04-20 13:43 UTC (permalink / raw)
  To: Jan Hubicka; +Cc: gcc-patches, rguenther

Hi,

On Sun, Apr 17, 2011 at 06:34:59PM +0200, 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?

It was needed because of LANG_HOOKS_FOLD_OBJ_TYPE_REF which I removed
last May (revision 159393) and I somehow left the implementation in
place.  I'll take care of removing it.

Thanks,

Martin

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2011-04-20 13:24 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-17 17:26 Remove vtable_method field in cgraph_node Jan Hubicka
2011-04-17 21:27 ` Richard Guenther
2011-04-18  0:51   ` Jan Hubicka
2011-04-18  9:27     ` Richard Guenther
2011-04-18 15:40       ` Jan Hubicka
2011-04-18 15:41         ` Richard Guenther
2011-04-18 16:27           ` Jan Hubicka
2011-04-20 13:43 ` Martin Jambor

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).