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

[Jan Hubicka]

> > Seems OK, however..
> 
> To what extent is this an approval? :-)

Approval only after you convince me on the questions bellow.

> 
> > > Index: src/gcc/gimplify.c
> > > ===================================================================
> > > --- src.orig/gcc/gimplify.c	2011-03-19 01:16:24.000000000 +0100
> > > +++ src/gcc/gimplify.c	2011-03-19 01:54:42.000000000 +0100
> > > @@ -959,11 +959,11 @@ copy_if_shared (tree *tp)
> > >  static void
> > >  unshare_body (tree *body_p, tree fndecl)
> > >  {
> > > -  struct cgraph_node *cgn = cgraph_node (fndecl);
> > > +  struct cgraph_node *cgn = cgraph_get_node (fndecl);
> > >  
> > >    copy_if_shared (body_p);
> > >  
> > > -  if (body_p == &DECL_SAVED_TREE (fndecl))
> > > +  if (cgn && body_p == &DECL_SAVED_TREE (fndecl))
> > 
> > The non-NULL check is here because of gimplification happening
> > before cgraph construction?
> 
> One of my notes I took during development says: "unshare_body can
> request a non-existing cgraph_node if called from
> finalize_size_functions in stor-layout.c."  And a quick test yesterday
> revealed that it can be also called when generating profile
> (create_coverage->cgraph_build_static_cdtor_1->gimplify_function_tree).
> So it's basically corner cases.

Hmm, so indeeed it is gimplificaiton before cgraph is updated. OK then.
> 
> > > 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.
> 
> > > 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.
> 
> > 
> > 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).

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?

Honza
> 
> Nevertheless, there seems to be no opposition to the interface changes
> I proposed so I am going to split the huge patch some more and seek
> approval for the individual pieces.
> 
> Thanks,
> 
> Martin