Re: [PATCH] New IPA-CP with real function cloning

[Martin Jambor <mjambor@suse.cz>]

Hi,

I'll send a new version of IPA-CP incorporating most of the feedback
in a new thread but let me also comment on some of the points here:

On Fri, Jul 08, 2011 at 08:24:31PM +0200, Jan Hubicka wrote:
> > > > {
> > > >   /* Pointer to an array of structures describing individual formal
> > > >      parameters.  */
> > > >   struct ipcp_lattice *lattices;
> > > 
> > > Hmm, how we get here around the need to mark this GTY(). I.e are we sure that all the known_vals
> > > must be referneced from elsewhere at ggc time?
> > 
> > (Scalar) constants that are results of arithmetic jump functions may
> > not be referenced from elsewhere, everything else is referenced from
> > the jump functions.  If it is a problem it is already present in the
> > current IPA-CP.  ipa_node_params and lattices are not GTYed there
> > either.
> 
> Hmm, I guess it is not really problem only because the lattices are used
> only in ipa-cp so the values do not really live across GGC call.

Well, technically they survive until after inlining (because of
indirect inlining which also derives information from the lattices
corresponding to node->inlined_to node.  Results of arithmetic
functions are not going to be accessed during inlining when compiling
any reasonable program but...

> > > > 
> > > > /* ipa_edge_args stores information related to a callsite and particularly its
> > > >    arguments.  It can be accessed by the IPA_EDGE_REF macro.  */
> > > > typedef struct GTY(()) ipa_edge_args
> > > 
> > > probably edge_summary would be my preferred name.
> > 
> > Ugh, this is the current name, we may change it later.  In any event
> > the name should probably tell that the summary is about parameters.
> 
> Hmm, OK, it is not bad name after all.

:-)

> > > 
> > > >       || !inline_summary (node)->versionable
> > > >       || TYPE_ATTRIBUTES (TREE_TYPE (node->decl))
> > > 
> > > This is getting an issue for Fortran that attach the function arguments quite commonly,
> > > perhaps we should start moving ahead on this and ruling out only the arguments that
> > > gets can't be preserved.
> > 
> > Yes, for example in 433.milc basically all the functions are
> > considered not versionable because of this.  I also thought of not
> > removing parameters for such functions.
> > 
> > > Also you need to test attributes of DECL itself.
> > 
> > Really?  We are not doing it now, nether for IPA-CP nor for IPA-SRA
> > (which is good at triggering problems with these).
> 
> Hmm, all the ipa-inline code checks DECL_ATTRIBUTES only.  I believe the type
> attributes ends up being copied to decl attributes but not vice versa.
> I guess the testcase should be
> 
> __attribute__ ((really_bad_attribute))
> function (int param)
> {
>   use param
> }
> 
> and then let ipa-cp to invent param is a constant.

what would be such a "really_bad_attribute" ? 

> > 
> > > 
> > > I think this all should be part of can_change_signature_p code, since we can version
> > > with all attributes I can thunk of when we don't change signature.
> > > 
> > > >       || cgraph_function_body_availability (node) <= AVAIL_OVERWRITABLE)
> > > >     res =  false;
> > > >   else
> > > >     /* Removing arguments doesn't work if the function takes varargs
> > > >        or use __builtin_apply_args.
> > > >        FIXME: handle this together with can_change_signature flag.  */
> > > >     for (edge = node->callees; edge; edge = edge->next_callee)
> > > >       {
> > > > 	tree t = edge->callee->decl;
> > > > 	if (DECL_BUILT_IN_CLASS (t) == BUILT_IN_NORMAL
> > > > 	    && (DECL_FUNCTION_CODE (t) == BUILT_IN_APPLY_ARGS
> > > > 		|| DECL_FUNCTION_CODE (t) == BUILT_IN_VA_START))
> > > 
> > > can_change_sigunature will also handle apply_args.
> > > Add VA_START code there, too.  For the other use of this flag (in i386) VA_START

The last one already is VA_START... or do you mean a different one?

> > > rules out the change, too, but so far we explicitely tested that in the backend.
> > > 
> > > Iguess with these changes, inline_summary (node)->versionable should coincide
> > > with IPA_NODE_REF (node)->node_versionable making this whole code unnecesary
> > > (it was supposed to work this way, I am not sure why we now have to versionable
> > > flags).
> > 
> > That would be nice.  I was wondering why the difference between the
> > two.  Again, I am yet to decide whether this should be done as a
> > followup or within this change.
> 
> OK.  

BTW, it will be a followup change.

> > > If you use for_node_thunk_and_aliases you can remove the recursion you do above
> > > and it will work for aliases of thunk correctly, too ;)
> > 
> > But I wouldn't be able to check the edge for hotness.
> 
> BTW currently the edges from thunks miss any profile info.
> (i.e. it will be 0). I guess we ought to make ipa-profile pass to estimate those
> (it is difficult ot measure count of an alias).
> 

I'm not really looking at the edges from thunks to the actual
function.  OTOH, I assume that edges to a thunk do have a count and
look at that.

> If you walk only hot edges, then you need to make your function descent into
> both alias refs and thunks calls, or the aliases of thunks will not be seen
> then.

Well, looking at bits of the patch again now, aliases to thunks might
indeed be a problem for a few pieces of it.  I'll send the patch
nevertheless and ponder about this problem later.

Thanks,

Martin