[PATCH 1/4] Remove cgraph_node function and fixup all callers
Martin Jambor
mjambor@suse.cz
Fri Mar 25 10:25:00 GMT 2011
Hi,
On Wed, Mar 23, 2011 at 10:08:53AM +0100, Jan Hubicka wrote:
> > 2011-03-18 Martin Jambor <mjambor@suse.cz>
> >
> > * cgraph.h (cgraph_node): Remove function declaration.
> > (cgraph_create_node): Declare.
> > (cgraph_get_create_node): Likewise.
> > (cgraph_do_get_node): New function.
> >
> > * cgraph.c (cgraph_create_node): Renamed to cgraph_create_node_1.
> > Updated all callers.
> > (cgraph_node): Renamed to cgraph_create_node, assert that a node for
> > the decl does not already exist. Call cgraph_get_create_node instead
> > of cgraph_node.
> > (cgraph_get_create_node): New function.
> > (cgraph_same_body_alias): Update comment.
> > (cgraph_set_call_stmt): Call cgraph_do_get_node instead of cgraph_node.
> > (cgraph_update_edges_for_call_stmt): Likewise.
> > (cgraph_clone_edge): Likewise.
> > (cgraph_create_virtual_clone): Likewise.
> > (cgraph_set_call_stmt): Call cgraph_get_create_node instead of
> > cgraph_node.
> > (cgraph_local_info): Call cgraph_get_node instead of cgraph_node,
> > handle NULL return value.
> > (cgraph_global_info): Likewise.
> > (cgraph_rtl_info): Likewise.
> > (cgraph_add_new_function): Call cgraph_create_node or
> > cgraph_get_create_node instead of cgraph_node.
> >
> > * cgraphbuild.c (record_reference): Call cgraph_get_create_node
> > instead of cgraph_node.
> > (record_eh_tables): Likewise.
> > (mark_address): Likewise.
> > (mark_load): Likewise.
> > (build_cgraph_edges): Call cgraph_get_node and cgraph_get_create_node
> > instead of cgraph_node.
> > (rebuild_cgraph_edges): Likewise.
> > (cgraph_rebuild_references): Call cgraph_get_node instead of
> > cgraph_node.
> > (remove_cgraph_callee_edges): Likewise.
> >
> > * cgraphunit.c (cgraph_finalize_function): Call cgraph_get_create_node
> > instead of cgraph_node.
> > (cgraph_mark_if_needed): Call cgraph_get_node instead of cgraph_node.
> > (verify_cgraph_node): Likewise.
> > (cgraph_analyze_functions): Likewise.
> > (cgraph_preserve_function_body_p): Likewise.
> > (save_inline_function_body): Likewise.
> > (save_inline_function_body): Likewise.
> > (cgraph_copy_node_for_versioning): Call cgraph_create_node instead of
> > cgraph_node.
> >
> > * tree-inline.c (copy_bb): Call cgraph_get_node instead of cgraph_node.
> > (estimate_num_insns): Likewise and handle returned NULL.
> > (optimize_inline_calls): Call cgraph_get_node instead of cgraph_node.
> > (tree_function_versioning): Call cgraph_do_get_node instead of
> > cgraph_node.
> >
> > * lto-symtab.c (lto_symtab_merge_cgraph_nodes_1): Call
> > cgraph_create_node instead of cgraph_node.
> >
> > * c-decl.c (finish_function): Call cgraph_get_create_node instead
> > of cgraph_node.
> > * c-family/c-gimplify.c (c_genericize): Likewise.
> > * lto-cgraph.c (input_node): Likewise.
> > * lto-streamer-in.c (input_function): Likewise.
> >
> > * except.c (set_nothrow_function_flags): Call cgraph_get_node instead
> > of cgraph_node.
> > * final.c (rest_of_clean_state): Likewise.
> > * gimple-iterator.c (update_call_edge_frequencies): Likewise.
> > * passes.c (pass_init_dump_file): Likewise.
> > (execute_all_ipa_transforms): Likewise.
> > (function_called_by_processed_nodes_p): Likewise.
> > * predict.c (maybe_hot_frequency_p): Likewise.
> > (probably_never_executed_bb_p): Likewise.
> > (compute_function_frequency): Likewise.
> > * tree-nested.c (check_for_nested_with_variably_modified): Likewise.
> > (unnest_nesting_tree_1): Likewise.
> > (lower_nested_functions): Likewise.
> > * tree-optimize.c (execute_fixup_cfg): Likewise.
> > (tree_rest_of_compilation): Likewise.
> > * tree-optimize.c (execute_fixup_cfg): Likewise.
> > (tree_rest_of_compilation): Likewise.
> > * tree-profile.c (gimple_gen_ic_func_profiler): Likewise.
> > * tree-sra.c (ipa_early_sra): Likewise.
> > * tree-ssa-loop-ivopts.c (computation_cost): Likewise.
> > * config/i386/i386.c (ix86_compute_frame_layout): Likewise.
> > * ipa.c (record_cdtor_fn): Likewise.
> > * ipa-inline.c (cgraph_early_inlining): Likewise.
> > (compute_inline_parameters_for_current): Likewise.
> > * ipa-prop.c (ipa_make_edge_direct_to_target): Likewise.
> > * ipa-pure-const.c (local_pure_const): Likewise.
> > * ipa-split.c (split_function): Likewise.
> > (split_function): Likewise.
> > (execute_split_functions): Likewise.
> >
> > * lto-streamer-in.c (lto_read_body): Call cgraph_do_get_node instead
> > of cgraph_node.
> > * omp-low.c (new_omp_context): Likewise.
> > (create_task_copyfn): Likewise.
> > * tree-emutls.c (lower_emutls_function_body): Likewise.
> > * ipa-struct-reorg.c (update_cgraph_with_malloc_call): Likewise.
> > * ipa-type-escape.c (check_call): Likewise.
> > * matrix-reorg.c (transform_allocation_sites): Likewise.
> >
> > * gimplify.c (unshare_body): Call cgraph_get_node instead of
> > cgraph_node, handle NULL return value.
> > (unvisit_body): Likewise.
> > (gimplify_body): Likewise.
> > * predict.c (optimize_function_for_size_p): Likewise.
> > * tree-ssa-alias.c (ref_maybe_used_by_call_p_1): Likewise.
> > (call_may_clobber_ref_p_1): Likewise.
> >
> > * varasm.c (function_section_1): Call cgraph_get_node instead of
> > cgraph_node and handle returned NULL.
> > (assemble_start_function): Likewise.
> > (mark_decl_referenced): Call cgraph_get_create_node instead of
> > cgraph_node.
> > (assemble_alias): Likewise.
> >
> > gcc/c-family/
> > * c-gimplify.c (c_genericize): Call cgraph_get_create_node instead
> >
> > gcc/cp/
> > * cp/class.c (cp_fold_obj_type_ref): Call cgraph_get_create_node
> > instead of cgraph_node.
> > * cp/decl2.c (cxx_callgraph_analyze_expr): Likewise.
> > (cp_write_global_declarations): Likewise.
> > * cp/optimize.c (maybe_clone_body): Likewise.
> > (maybe_clone_body): Likewise.
> > * cp/semantics.c (maybe_add_lambda_conv_op): Likewise.
> > * cp/mangle.c (mangle_decl): Likewise.
> > * cp/method.c (make_alias_for_thunk): Likewise.
> > (use_thunk): Likewise.
> >
> > gcc/ada/
> > * gcc-interface/utils.c (end_subprog_body): Call
> > cgraph_get_create_node instead of cgraph_node.
> >
> > gcc/fortran/
> > * trans-decl.c (gfc_generate_function_code): Call
> > cgraph_get_create_node instead of cgraph_node.
> >
> > gcc/java/
> > * decl.c (java_mark_decl_local): Call cgraph_get_node instead of
> > cgraph_node and handle returned NULL.
> >
> > gcc/objc/
> > * objc-act.c (mark_referenced_methods): Call cgraph_get_create_node
> > instead of cgraph_node.
>
> Seems OK, however..
To what extent is this an approval? :-)
> > 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.
> > 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.
> > 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.
>
> 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.
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
More information about the Gcc-patches
mailing list