[PATCH, PR 45934 1/6] [PR 46287] Do not generate direct calls to thunks

[Martin Jambor]

Hi,

On Wed, Dec 01, 2010 at 09:58:08PM +0100, Jan Hubicka wrote:
> > 2010-11-08  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	PR tree-optimization/46053
> > 	PR middle-end/46287
> > 	* cgraph.h (cgraph_indirect_call_info): New field.
> > 	* gimple.h (gimple_fold_obj_type_ref): Declaration removed.
> > 	(gimple_fold_call): Declare.
> > 	(gimple_adjust_this_by_delta): Likewise.
> > 	* cgraph.c (cgraph_make_edge_direct): New parameter delta.  Updated
> > 	all users.
> > 	(cgraph_clone_edge): Create a copy of indirect_info also for direct
> > 	edges.
> > 	* cgraphunit.c (cgraph_redirect_edge_call_stmt_to_callee): Adjust this
> > 	parameters.
> > 	* gimple-fold.c (gimple_fold_obj_type_ref_known_binfo): Renamed to
> > 	gimple_get_virt_mehtod_for_binfo, new parameter delta.  Do not search
> > 	through thunks, in fact bail out if we encounter one, check that
> > 	BINFO_VIRTUALS is not NULL.
> > 	(gimple_adjust_this_by_delta): New function.
> > 	(gimple_fold_obj_type_ref): Removed.
> > 	(gimple_fold_obj_type_ref_call): New function.
> > 	(fold_gimple_call): Renamed to gimple_fold_call, made external.
> > 	Updated users.  Call gimple_fold_obj_type_ref_call instead of
> > 	gimple_fold_obj_type_ref.
> > 	* ipa-cp.c (ipcp_process_devirtualization_opportunities): Process
> > 	thunk deltas.
> > 	(ipcp_discover_new_direct_edges): Likewise.
> > 	* ipa-prop.c (ipa_make_edge_direct_to_target): New parameter delta.
> > 	Updated callers.
> > 	(ipa_write_indirect_edge_info): Stream thunk_delta.
> > 	(ipa_read_indirect_edge_info): Likewise.
> > 	* tree-ssa-ccp.c (ccp_fold_stmt): Use gimple_fold_call instead of
> > 	gimple_fold_obj_type_ref.
> > 
> > 	* testsuite/g++.dg/ipa/pr46053.C: New test.
> > 	* testsuite/g++.dg/ipa/pr46287-1.C: Likewise.
> > 	* testsuite/g++.dg/ipa/pr46287-2.C: Likewise.
> > 	* testsuite/g++.dg/ipa/pr46287-3.C: Likewise.
> > 	* testsuite/g++.dg/torture/covariant-1.C: Likewise.
> > 	* testsuite/g++.dg/torture/pr46287.C: Likewise.
> > 
> 
> >  /* Make an indirect EDGE with an unknown callee an ordinary edge leading to
> > -   CALLEE.  */
> > +   CALLEE.  DELTA, if non-NULL, is an integer constant that is to be added to
> > +   the this pointer (first parameter).  */
> >  
> >  void
> > -cgraph_make_edge_direct (struct cgraph_edge *edge, struct cgraph_node *callee)
> > +cgraph_make_edge_direct (struct cgraph_edge *edge, struct cgraph_node *callee,
> > +			 tree delta)
> >  {
> >    edge->indirect_unknown_callee = 0;
> > +  edge->indirect_info->thunk_delta = delta;
> 
> I guess it is still fine as it is definite improvement over current
> situation, but won't we need to handle all of cgraph_thunk_info
> here?

Eventually we will, certainly before we start propagating constants to
zeroth arguments of OBJ_TYPE_REFs.  And not only here but also at
other places (e.g. cgraph_set_call_stmt and company... and of course
there is the weird cgraph_create_edge_including_clones I wrote you
about earlier).

> In thunk_info it is HOST_WIDE_INT, I would expect it to be here as well.

I extract it from BINFOs as tree constants and I need to use it in
POINTER_PLUSes as tree constants so there is probably no point in
striing it as H_W_I.

> 
> > +  if (e->indirect_info && e->indirect_info->thunk_delta
> > +      && integer_nonzerop (e->indirect_info->thunk_delta))
> > +    {
> > +      if (cgraph_dump_file)
> > +	{
> > +	  fprintf (cgraph_dump_file, "          Thunk delta is ");
> > +	  print_generic_expr (cgraph_dump_file,
> > +			      e->indirect_info->thunk_delta, 0);
> > +	  fprintf (cgraph_dump_file, "\n");
> > +	}
> > +      gsi = gsi_for_stmt (e->call_stmt);
> > +      gsi_computed = true;
> > +      gimple_adjust_this_by_delta (&gsi, e->indirect_info->thunk_delta);
> > +      e->indirect_info->thunk_delta = NULL_TREE;
> > +    }
> 
> Should've test here if this parameter was eliminated or not?
> (i.e. first bit of e->callee->clone.combined_args_to_skip?

It is not strictly necessary because I do the callee modification
before removing parameters but on the second thought, yes, we should
avoid creating unnecessary stuff when it is this easy.  So I will add
the check and the condition will be:

  if (e->indirect_info && e->indirect_info->thunk_delta
      && integer_nonzerop (e->indirect_info->thunk_delta)
      && (!e->callee->clone.combined_args_to_skip
	  || !bitmap_bit_p (e->callee->clone.combined_args_to_skip, 0)))

> 
> > Index: icln/gcc/ipa-prop.c
> > ===================================================================
> > --- icln.orig/gcc/ipa-prop.c
> > +++ icln/gcc/ipa-prop.c
> 
> I would expect somewhere here to be code handling updating of operand
> value for non-0 delta when doing ipa-cp propagation, but don't seem to
> be able to find it?

What do you mean?  IPA-CP stores the delta into the indirect info
structure of the corresponding edge and then the code in
gimple_adjust_this_by_delta performs the adjustment.

> 
> The rest seems OK.  I am most concerned that we implement just part of thunk
> logic, but I see that you get deltas from BINFOs and the rest of adjustments
> are not there?
> 

Correct, for simple this adjusting thunks, BINFOs contain the decl of
the real function and the this delta separately, for more complex
thunks it stores the decl of an underlying thunk + a this delta which
would otherwise be adjusted in this thunk.

And yes, we will certainly have to re-think how to represent thunks in
the cgraph in a more general way.

Anyway, is the patch OK with the above change then?

Thanks,

Martin