> > 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
