Re: [PATCH 1/4] Remove cgraph_node function and fixup all callers

[Martin Jambor <mjambor at suse dot cz>]

Hi,

On Fri, Mar 25, 2011 at 05:55:24PM +0100, Jan Hubicka wrote:
> > > > Index: src/gcc/passes.c
> > > > ===================================================================
> > > > --- src.orig/gcc/passes.c	2011-03-19 01:16:23.000000000 +0100
> > > > +++ src/gcc/passes.c	2011-03-19 01:54:42.000000000 +0100
> > > > @@ -1344,7 +1344,7 @@ pass_init_dump_file (struct opt_pass *pa
> > > >        if (dump_file && current_function_decl)
> > > >  	{
> > > >  	  const char *dname, *aname;
> > > > -	  struct cgraph_node *node = cgraph_node (current_function_decl);
> > > > +	  struct cgraph_node *node = cgraph_get_node (current_function_decl);
> > > 
> > > Don't you want cgraph_do_get_node on those places that will ICE anyway?
> > 
> > No, the other way round.  I only used cgraph_do_get_node when it would
> > not ICE immediately but might later, like when the result was passed
> > to a caller or stored in some data structure for later use.  However
> > using it here is certainly possible, if you like.
> 
> Hmm, OK.  The mutiplicity of cgraph accesstors is bit confusing.  For future
> development I would preffer if people don't really need to worry about those
> except for very corner cases (i.e. cgraph construction).
> 
> I would like to have one most common way to get cgraph node that is
> used thorough backend.  I think it should be cgraph_do_get_node
> since in the backend all functions that matters should be in cgraph.

My intention is really to make cgraph_get_node the one main accessor
function almost everyone should use to get their node.  Of course,
that means the return value should be checked somehow.  My gut feeling
at this moment is that now there are still too many cases where it can
return NULL legitimately (builtins, OMP builtins, mudflap stuff,
profile generation stuff, size functions, calls from front-end for
aliases...) that whoever wants to get their node will have to
differentiate in between the two cases when they can assert the result
is not NULL and when they cannot.

That will change only if we manage to create a node for each of these
problematic corner cases before someone can ask for it and not remove
it when removing unreachable nodes if folder can still put it into IL
(etc. etc.).

In fact, cgraph_do_get_node was originally a hack to help me identify
places where cgraph_get_node returns NULL.  And them I realized it
might be useful so I left it in the patch.  Actually, I anticipated a
request to remove it completely, not make it the default :-)

On the other hand I do agree that the other accessors (create,
get_create) should be used very rarely - preferably only in
cgraph(re)build and front-ends and we should continue efforts to weed
them out of everywhere else.

> > 
> > > > Index: src/gcc/tree-ssa-alias.c
> > > > ===================================================================
> > > > --- src.orig/gcc/tree-ssa-alias.c	2011-03-19 01:16:23.000000000 +0100
> > > > +++ src/gcc/tree-ssa-alias.c	2011-03-19 01:54:42.000000000 +0100
> > > > @@ -1246,14 +1246,15 @@ ref_maybe_used_by_call_p_1 (gimple call,
> > > >  
> > > >    /* Check if base is a global static variable that is not read
> > > >       by the function.  */
> > > > -  if (TREE_CODE (base) == VAR_DECL
> > > > +  if (callee != NULL_TREE
> > > > +      && TREE_CODE (base) == VAR_DECL
> > > 
> > > Why non-NULL tests are needed here?  It seems that at the time
> > > cgraph is created we should have nodes for all accessible functions.
> > 
> > Almost.  We do not have a node for __builtin_GOMP_parallel_start when
> > we examine a call from dse_possible_dead_store_p.  It would be nice in
> > general if OMP made call graph nodes for its new functions in some
> > more defined manner.  At this point it relies on cgraphbuild.c and
> > apparently even that is sometimes not enough.  Should I add a FIXME
> > here?
> > 
> > The failing tests are gcc.dg/autopar/reduc-1.c and
> > gcc.dg/autopar/reduc-2.c.
> 
> 
> Yes, please add FIXME.  It is better to fix incrementally, but I gor
> previously troubles with GOMP requiring or not requiring cgraph
> nodes to be present for interesting reasons (i.e. I have to disable
> unreachable node removal before GOMP is ready).  We need to clean
> this up.

OK

> > 
> > > 
> > > What I wonder about is that we have similar API for annotations.  Here we have
> > > var_ann_t for type name
> > > var_ann (var) for what cgraph_get_node is
> > > get_var_ann (var) for what cgraph_get_create_node is
> > > create_var_ann (var) for hwat cgraph_create_node is.
> > > 
> > > So we may want to be consistent here. I never had problem with overlaping
> > > struct and function obvoiusly, but if people disagree, what about:
> > > 
> > > cgraph_create_node = crate node and aborts if already exist
> > > cgraph_get_node = return node or create noe
> > > cgraph_maybe_get_node = return node or NULL
> > > cgraph_do_get_node = return node and abort if NULL.
> > > 
> > > We may later even want to introduce our popular
> > > cgraph_node_d/cgraph_node_t and rename back cgraph_do_get_node
> > > to cgraph_node, since it is is the most common accestor.  I
> > > would preffer to do that incrementally so we can merge
> > > ipa-inline changes.
> > 
> > Frankly, I think it is too much work for very little gain, and it
> > would cause a lot of trouble for anyone maintaining their patches or
> > branches.  Moreover, changing re-using function names of
> > cgraph_get_node and cgraph_node while changing their behavior seems
> > like a bad idea to me.
> 
> Well, how many open patches people have? (except for me and Richi's
> inliner changes).

I don't know but changing every "struct cgraph_node" in the source to
"cgraph_mode_t" (or _d?) seems quite invasive and time consuming and
boring... and did I say I don't think it is very useful?

> In general I do not like those small disprepancies in API names
> remaining in GCC forever, so I would vote to change the names to be
> consistent at some point.  With you current patch it is all matter
> of simple sed script, more or less, isn't it?

Well, not really.  My patch touches every call to "cgraph_node" there is
but it does not do the same for every currently existing call to
"cgraph_get_node" (which would also need to be examined and
potentially converted to the new _maybe_get_ variant).  Of course
changing every "struct cgraph_node" to "cgraph_node_favorite_letter"
would require even more modifications.

I also think that making cgraph_get_node to do something radically
different from what it does now (like lazily creating nodes) will lead
to confusion and bugs and it is not a good idea in general.

Having said that, I am willing to do the work necessary to make
cgraph_get_node to abort instead of returning NULL and introduce
cgraph_maybe_get_node which would not.  On the other hand, I would
very much prefer to spend my time on other issues than making cgraph
interface consistent with variable annotations interface.  If someone
else wants to do it, my patches will help them.

Is there any chance that you will be on IRC soon?  I'd just love to
get through this.

Thanks,

Martin