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
