From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11706 invoked by alias); 14 Jul 2014 16:25:03 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 11388 invoked by uid 89); 14 Jul 2014 16:25:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=4.2 required=5.0 tests=AWL,BAYES_00,SPAM_BODY autolearn=no version=3.3.2 X-HELO: mx2.suse.de Received: from cantor2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Mon, 14 Jul 2014 16:24:50 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 5B9E3ABAE; Mon, 14 Jul 2014 16:24:47 +0000 (UTC) Message-ID: <53C4044E.5030106@suse.cz> Date: Mon, 14 Jul 2014 17:18:00 -0000 From: =?windows-1252?Q?Martin_Li=9Aka?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-Version: 1.0 To: Jan Hubicka CC: GCC Patches Subject: Re: [RFC, PATCH 1/n] IPA C++ refactoring References: <53BE7C94.3010909@suse.cz> <20140711100702.GA8908@atrey.karlin.mff.cuni.cz> In-Reply-To: <20140711100702.GA8908@atrey.karlin.mff.cuni.cz> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2014-07/txt/msg00990.txt.bz2 Hello On 07/11/2014 12:07 PM, Jan Hubicka wrote: >> Hi, >> this first patch continues with rafactoring of IPA infrastructure so that we will have C++ API. In the patch, I transformed many global functions to members of symtab_node and cgraph_node. >> >> Example: >> cgraph_remove_node (struct cgraph_node *node) -> cgraph_node::remove (void) >> symtab_unregister_node (symtab_node *node) -> symtab_node::unregister (void) >> >> The patch is being consulted with Honza and will iterate. We want to inform folk that we plan to do following changes. >> >> After the patch is applied, I would like to transform varpool_node and cgraph_edge in the following patch. >> >> Thank you for your comments, >> Martin > > /* Remove the node from cgraph. */ > > Perhaps Remove function from symbol table. > (similarly for varpool, perhaps few other block comments needs revisiting. > We may do that incrementally.) > > + /* Add node into symbol table. This function is not used directly, but via > + cgraph/varpool node creation routines. */ > + void register_symbol (void); > + > + /* Remove symtab node from the symbol table. */ > + void remove (void); > + > + /* Dump symtab node to F. */ > + void dump (FILE *f); > + > + /* Dump symtab node to stderr. */ > + void DEBUG_FUNCTION debug (void); > + > + /* Verify consistency of node. */ > + void DEBUG_FUNCTION verify (void); > + > + /* Return ipa reference from this symtab_node to > + REFERED_NODE or REFERED_VARPOOL_NODE. USE_TYPE specify type > + of the use and STMT the statement (if it exists). */ > + struct ipa_ref *add_reference (symtab_node *referred_node, > + enum ipa_ref_use use_type); > + > + /* Return ipa reference from this symtab_node to > + REFERED_NODE or REFERED_VARPOOL_NODE. USE_TYPE specify type > + of the use and STMT the statement (if it exists). */ > + struct ipa_ref *add_reference (symtab_node *referred_node, > + enum ipa_ref_use use_type, gimple stmt); > + > + /* If VAL is a reference to a function or a variable, add a reference from > + this symtab_node to the corresponding symbol table node. USE_TYPE specify > + type of the use and STMT the statement (if it exists). Return the new > + reference or NULL if none was created. */ > + struct ipa_ref *maybe_add_reference (tree val, enum ipa_ref_use use_type, > + gimple stmt); > + > + /* Clone all references from symtab NODE to this symtab_node. */ > + void clone_references (symtab_node *node); > + > + /* Remove all stmt references in non-speculative references. > + Those are not maintained during inlining & clonning. > + The exception are speculative references that are updated along > + with callgraph edges associated with them. */ > + void clone_referring (symtab_node *node); > + > + /* Clone reference REF to this symtab_node and set its stmt to STMT. */ > + struct ipa_ref *clone_reference (struct ipa_ref *ref, gimple stmt); > + > + /* Find the structure describing a reference to REFERRED_NODE > + and associated with statement STMT. */ > + struct ipa_ref *find_reference (symtab_node *, gimple, unsigned int); > + > + /* Remove all references that are associated with statement STMT. */ > + void remove_stmt_references (gimple stmt); > + > + /* Remove all stmt references in non-speculative references. > + Those are not maintained during inlining & clonning. > + The exception are speculative references that are updated along > + with callgraph edges associated with them. */ > + void clear_stmts_in_references (void); > + > + /* Remove all references in ref list. */ > + void remove_all_references (void); > + > + /* Remove all referring items in ref list. */ > + void remove_all_referring (void); > + > + /* Dump references in ref list to FILE. */ > + void dump_references (FILE *file); > + > + /* Dump referring in list to FILE. */ > + void dump_referring (FILE *); > + > + /* Return true if symtab node and TARGET represents > + semantically equivalent symbols. */ > + bool semantically_equivalent_p (symtab_node *target); > + > + /* Classify symbol symtab node for partitioning. */ > + enum symbol_partitioning_class get_partitioning_class (void); > + > + /* Return comdat group. */ > + tree get_comdat_group () > + { > + return x_comdat_group; > + } > + > + /* Return comdat group as identifier_node. */ > + tree get_comdat_group_id () > + { > + if (x_comdat_group && TREE_CODE (x_comdat_group) != IDENTIFIER_NODE) > + x_comdat_group = DECL_ASSEMBLER_NAME (x_comdat_group); > + return x_comdat_group; > + } > + > + /* Set comdat group. */ > + void set_comdat_group (tree group) > + { > + gcc_checking_assert (!group || TREE_CODE (group) == IDENTIFIER_NODE > + || DECL_P (group)); > + x_comdat_group = group; > + } > + > + /* Return section as string. */ > + const char * get_section () > + { > + if (!x_section) > + return NULL; > + return x_section->name; > + } > + > + /* Remove node from same comdat group. */ > + void remove_from_same_comdat_group (void); > + > + /* Add this symtab_node to the same comdat group that OLD is in. */ > + void add_to_same_comdat_group (symtab_node *old_node); > + > + /* Dissolve the same_comdat_group list in which NODE resides. */ > + void dissolve_same_comdat_group_list (void); > + > + /* Return true when symtab_node is known to be used from other (non-LTO) > + object file. Known only when doing LTO via linker plugin. */ > + bool used_from_object_file_p (void); > + > + /* Walk the alias chain to return the symbol NODE is alias of. > + If NODE is not an alias, return NODE. > + When AVAILABILITY is non-NULL, get minimal availability in the chain. */ > + symtab_node *ultimate_alias_target (enum availability *avail = NULL); > + > + /* Return next reachable static symbol with initializer after NODE. */ > + inline symtab_node *next_defined_symbol (void); > + > + /* Add reference recording that symtab node is alias of TARGET. > + The function can fail in the case of aliasing cycles; in this case > + it returns false. */ > + bool resolve_alias (symtab_node *target); > + > + /* C++ FE sometimes change linkage flags after producing same > + body aliases. */ > + void fixup_same_cpp_alias_visibility (symtab_node *target); > + > + /* Call calback on symtab node and aliases associated to this node. > + When INCLUDE_OVERWRITABLE is false, overwritable aliases and thunks are > + skipped. */ > + bool call_for_symbol_and_aliases (bool (*callback) (symtab_node *, void *), > + void *data, > + bool include_overwrite); > + > + /* If node can not be interposable by static or dynamic linker to point to > + different definition, return this symbol. Otherwise look for alias with > + such property and if none exists, introduce new one. */ > + symtab_node *noninterposable_alias (void); > + > + /* Return node that alias N is aliasing. */ > + inline symtab_node *get_alias_target (void); > + > + /* Iterates I-th reference in the list, REF is also set. */ > + struct ipa_ref *iterate_reference (unsigned i, struct ipa_ref *&ref); > + > + /* Iterates I-th referring item in the list, REF is also set. */ > + struct ipa_ref *iterate_referring (unsigned i, struct ipa_ref *&ref); > > The iterators probably should go along with other reference API methods > earlier. > + > + /* Iterates I-th referring alias item in the list, REF is also set. */ > + struct ipa_ref *iterate_direct_aliases (unsigned i, struct ipa_ref *&ref); > + > + /* Set section for symbol and its aliases. */ > + void set_section (const char *section); > + > + /* Set section, do not recurse into aliases. > + When one wants to change section of symbol and its aliases, > + use set_section. */ > + void set_section_for_node (const char *section); > + > + /* Set initialization priority to PRIORITY. */ > + void set_init_priority (priority_type priority); > + > + /* Return the initialization priority. */ > + priority_type get_init_priority (); > + > + /* Return availability of NODE. */ > + enum availability get_availability (void); > + > + /* Make DECL local. */ > + void make_decl_local (void); > + > + /* Return true if list contains an alias. */ > + bool has_aliases_p (void); > + > + /* Return true when the symbol is real symbol, i.e. it is not inline clone > + or abstract function kept for debug info purposes only. */ > + bool real_symbol_p (void); > + > + /* Return true if NODE can be discarded by linker from the binary. */ > + inline bool > + can_be_discarded_p (void) > + { > + return (DECL_EXTERNAL (decl) > + || (get_comdat_group () > + && resolution != LDPR_PREVAILING_DEF > + && resolution != LDPR_PREVAILING_DEF_IRONLY > + && resolution != LDPR_PREVAILING_DEF_IRONLY_EXP)); > + } > + > + /* Return true if NODE is local to a particular COMDAT group, and must not > + be named from outside the COMDAT. This is used for C++ decloned > + constructors. */ > + inline bool > + comdat_local_p (void) > + { > + return (same_comdat_group && !TREE_PUBLIC (decl)); > + } > + > + /* Return true if ONE and TWO are part of the same COMDAT group. */ > + inline bool in_same_comdat_group_p (symtab_node *target); > + > + /* Return true when there is a reference to node and it is not vtable. */ > + bool address_taken_from_non_vtable_p (void); > + > + /* Return symbol table node associated with DECL, if any, > + and NULL otherwise. */ > + static inline symtab_node *get (const_tree decl) > + { > +#ifdef ENABLE_CHECKING > + /* Check that we are called for sane type of object - functions > + and static or external variables. */ > + gcc_checking_assert (TREE_CODE (decl) == FUNCTION_DECL > + || (TREE_CODE (decl) == VAR_DECL > + && (TREE_STATIC (decl) || DECL_EXTERNAL (decl) > + || in_lto_p))); > + /* Check that the mapping is sane - perhaps this check can go away, > + but at the moment frontends tends to corrupt the mapping by calling > + memcpy/memset on the tree nodes. */ > + gcc_checking_assert (!decl->decl_with_vis.symtab_node > + || decl->decl_with_vis.symtab_node->decl == decl); > +#endif > + return decl->decl_with_vis.symtab_node; > + } > + > + /* Dump symbol table to F. */ > + static void dump_table (FILE *); > + > + /* Dump symbol table to stderr. */ > + static inline DEBUG_FUNCTION void debug_symtab (void) > + { > + dump_table (stderr); > + } > + > + /* Verify symbol table for internal consistency. */ > + static DEBUG_FUNCTION void verify_symtab_nodes (void); > + > + /* Return true when NODE is known to be used from other (non-LTO) > + object file. Known only when doing LTO via linker plugin. */ > + static bool used_from_object_file_p_worker (symtab_node *node); > + > /* Type of the symbol. */ > ENUM_BITFIELD (symtab_type) type : 8; > > ... > > + /* Given function symbol, walk the alias chain to return the function node > + is alias of. Do not walk through thunks. > + When AVAILABILITY is non-NULL, get minimal availability in the chain. */ > + > + cgraph_node *function_or_thunk (enum availability *availability = NULL); > > function_or_thunk_symbol I guess > + > + /* Walk the alias chain to return the function cgraph_node is alias of. > + Walk through thunk, too. > + When AVAILABILITY is non-NULL, get minimal availability in the chain. */ > + cgraph_node *function_node (enum availability *avail = NULL); > > function_symbol > + > + /* Add thunk alias into callgraph. The alias declaration is ALIAS and it > + aliases DECL with an adjustments made into the first parameter. > + See comments in thunk_adjust for detail on the parameters. */ > + cgraph_node * create_thunk (tree alias, tree, bool this_adjusting, > + HOST_WIDE_INT fixed_offset, > + HOST_WIDE_INT virtual_value, > + tree virtual_offset, > + tree real_alias); > > Here I would put basic manipulation fifrst (create/get/remove), clones next > and thunks last. > + > + /* Create node representing clone of N executed COUNT times. Decrease > + the execution counts from original node too. > + The new clone will have decl set to DECL that may or may not be the same > + as decl of N. > + > + When UPDATE_ORIGINAL is true, the counts are subtracted from the original > + function's profile to reflect the fact that part of execution is handled > + by node. > + When CALL_DUPLICATOIN_HOOK is true, the ipa passes are acknowledged about > + the new clone. Otherwise the caller is responsible for doing so later. > + > + If the new node is being inlined into another one, NEW_INLINED_TO should be > + the outline function the new one is (even indirectly) inlined to. > + All hooks will see this in node's global.inlined_to, when invoked. > + Can be NULL if the node is not inlined. */ > + cgraph_node *clone_node (tree decl, gcov_type count, int freq, > > create_clone > + bool update_original, > + vec redirect_callers, > + bool call_duplication_hook, > + struct cgraph_node *new_inlined_to, > + bitmap args_to_skip); > + > + /* Create callgraph node clone with new declaration. The actual body will > + be copied later at compilation stage. */ > + cgraph_node *create_virtual_clone (vec redirect_callers, > + vec *tree_map, > + bitmap args_to_skip, const char * suffix); > + > + /* cgraph node being removed from symbol table; see if its entry can be > + replaced by other inline clone. */ > + cgraph_node *find_replacement (void); > > Shouldn't this be protected? I tried to mark it as protected, by there's usage that blocks that: In file included from ../../gcc/symtab.c:40:0: ../../gcc/cgraph.h: In member function ‘void symtab_node::unregister()’: ../../gcc/cgraph.h:1178:16: error: ‘cgraph_node* cgraph_node::find_replacement()’ is protected cgraph_node *find_replacement (void); ^ ../../gcc/symtab.c:462:46: error: within this context replacement_node = cnode->find_replacement (); > + > + /* Create a new cgraph node which is the new version of > + callgraph node. REDIRECT_CALLERS holds the callers > + edges which should be redirected to point to > + NEW_VERSION. ALL the callees edges of the node > + are cloned to the new version node. Return the new > + version node. > + > + If non-NULL BLOCK_TO_COPY determine what basic blocks > + was copied to prevent duplications of calls that are dead > + in the clone. */ > + > + cgraph_node *copy_for_versioning (tree new_decl, > + vec redirect_callers, > + bitmap bbs_to_copy); > > Hmm, perhaps better as create_version_clone (still bad name, but it should be > merged with clone) > + > + /* Perform function versioning. > + Function versioning includes copying of the tree and > + a callgraph update (creating a new cgraph node and updating > + its callees and callers). > + > + REDIRECT_CALLERS varray includes the edges to be redirected > + to the new version. > + > + TREE_MAP is a mapping of tree nodes we want to replace with > + new ones (according to results of prior analysis). > + > + If non-NULL ARGS_TO_SKIP determine function parameters to remove > + from new version. > + If SKIP_RETURN is true, the new version will return void. > + If non-NULL BLOCK_TO_COPY determine what basic blocks to copy. > + If non_NULL NEW_ENTRY determine new entry BB of the clone. > + > + Return the new version's cgraph node. */ > + cgraph_node *function_versioning (vec redirect_callers, > + vec *tree_map, > + bitmap args_to_skip, > + bool skip_return, > + bitmap bbs_to_copy, > + basic_block new_entry_block, > + const char *clone_name); > > create_version_clone_with_body > + > + /* Insert a new cgraph_function_version_info node into cgraph_fnver_htab > + corresponding to cgraph_node. */ > + struct cgraph_function_version_info *insert_new_function_version (void); > > It such that function_version here has nothing to do with function version above, > but I guess I will need to clean up incrementally. > > Perhaps create_new_function_version as we use create for edges/symbols and others. > + > + /* Discover all functions and variables that are trivially needed, analyze > + them as well as all functions and variables referred by them */ > + void analyze (void); > + > + /* Expand thunk NODE to gimple if possible. > + When FORCE_GIMPLE_THUNK is true, gimple thunk is created and > + no assembler is produced. > + When OUTPUT_ASM_THUNK is true, also produce assembler for > + thunks that are not lowered. */ > + bool expand_thunk (bool output_asm_thunks, bool force_gimple_thunk); > + > + /* As an GCC extension we allow redefinition of the function. The > + semantics when both copies of bodies differ is not well defined. > + We replace the old body with new body so in unit at a time mode > + we always use new body, while in normal mode we may end up with > + old body inlined into some functions and new body expanded and > + inlined in others. */ > + void reset (void); > > THese three probably would better become protected if we reorgnanize cgraphunit > bit more, but incrementally. > + > + /* Creates a wrapper from cgraph_node to TARGET node. Thunk is used for this > + kind of wrapper method. */ > + void make_wrapper (cgraph_node *target); > > create_wrapper > + > + /* Verify cgraph nodes of the cgraph node. */ > + void DEBUG_FUNCTION verify_node (void); > + > + /* Remove the node from cgraph. */ > + void remove (void); > + > + /* Dump call graph node to file F. */ > + void dump (FILE *f); > + > + /* Dump call graph node to stderr. */ > + void DEBUG_FUNCTION debug (void); > + > + /* When doing LTO, read cgraph_node's body from disk if it is not already > + present. */ > + bool get_body (void); > + > + /* Release memory used to represent body of function. > + Use this only for functions that are released before being translated to > + target code (i.e. RTL). Functions that are compiled to RTL and beyond > + are free'd in final.c via free_after_compilation(). */ > + void release_body (void); > > Those probably should go to start > + > + /* Remove the node from cgraph and all inline clones inlined into it. > + Skip however removal of FORBIDDEN_NODE and return true if it needs to be > + removed. This allows to call the function from outer loop walking clone > + tree. */ > + bool remove_symbol_and_inline_clones (cgraph_node *forbidden_node = NULL); > + > + /* Record all references from cgraph_node that are taken > + in statement STMT. */ > + void record_stmt_references (gimple stmt); > + > + /* Like cgraph_set_call_stmt but walk the clone tree and update all > + clones sharing the same function body. > + When WHOLE_SPECULATIVE_EDGES is true, all three components of > + speculative edge gets updated. Otherwise we update only direct > + call. */ > + void set_call_stmt_including_clones (gimple old_stmt, gimple new_stmt, > + bool update_speculative = true); > > Those performing operations on function and its clones should be grouped together. > + > + /* cgraph_node is no longer nested function; update cgraph accordingly. */ > + void unnest (void); > + > + /* Bring cgraph node local. */ > + void make_local (void); > + > + /* Likewise indicate that a node is having address taken. */ > + void mark_address_taken (void); > + > + /* Set fialization priority to PRIORITY. */ > + void set_fini_priority (priority_type priority); > + > + /* Return the finalization priority. */ > + priority_type get_fini_priority (); > + > + /* Remove all callees from the node. */ > + void remove_callees (void); > + > + /* Create edge from a given function to CALLEE in the cgraph. */ > + struct cgraph_edge *create_edge (cgraph_node *callee, > + gimple call_stmt, gcov_type count, > + int freq); > + /* Create an indirect edge with a yet-undetermined callee where the call > + statement destination is a formal parameter of the caller with index > + PARAM_INDEX. */ > + struct cgraph_edge * > + create_indirect_edge (gimple call_stmt, int ecf_flags, gcov_type count, > + int freq); > + > + /* Like cgraph_create_edge walk the clone tree and update all clones sharing > + same function body. If clones already have edge for OLD_STMT; only > + update the edge same way as cgraph_set_call_stmt_including_clones does. */ > + void create_edge_including_clones (struct cgraph_node *callee, > + gimple old_stmt, gimple stmt, > + gcov_type count, > + int freq, > + cgraph_inline_failed_t reason); > + > + /* Return the callgraph edge representing the GIMPLE_CALL statement > + CALL_STMT. */ > + cgraph_edge *get_edge (gimple call_stmt); > + > + /* Collect all callers of cgraph_node and its aliases that are known to lead > + to NODE (i.e. are not overwritable). */ > + vec collect_callers (void); > + > + /* Return function availability. See cgraph.h for description of individual > + return values. */ > + enum availability get_body_availability (void); > > This should be get_availability and shared with symtab/varpool equivalent. > + > + /* Set TREE_NOTHROW on cgraph_node's decl and on aliases of the node > + if any to NOTHROW. */ > + void set_nothrow_flag (bool nothrow); > + > + /* Set TREE_READONLY on cgraph_node's decl and on aliases of the node > + if any to READONLY. */ > + void set_const_flag (bool readonly, bool looping); > + > + /* Set DECL_PURE_P on cgraph_node's decl and on aliases of the node > + if any to PURE. */ > + void set_pure_flag (bool pure, bool looping); > + > + /* Call all node duplication hooks. */ > + void call_duplication_hooks (cgraph_node *node2); > > Can't it be private? duplicate_thunk_for_node calls the function for thunk->call_duplication_hooks. > + > + /* Call calback on function and aliases associated to the function. > + When INCLUDE_OVERWRITABLE is false, overwritable aliases and thunks are > + skipped. */ > + > + bool call_for_symbol_and_aliases (bool (*callback) (cgraph_node *, > + void *), > + void *data, bool include_overwritable); > + > + /* Call calback on cgraph_node, thunks and aliases associated to NODE. > + When INCLUDE_OVERWRITABLE is false, overwritable aliases and thunks are > + skipped. */ > + bool call_for_symbol_thunks_and_aliases (bool (*callback) (cgraph_node *node, > + void *data), > + void *data, > + bool include_overwritable); > + > + /* Call all node insertion hooks. */ > + void call_function_insertion_hooks (void); > > Likewise create_version_clone_with_body calls: new_version_node->call_function_insertion_hooks (); > + > + /* Return true when function can be marked local. */ > + bool local_p (void); > > Probably also private? Because users should use local flag. > + > + /* Return true if cgraph_node can be made local for API change. > + Extern inline functions and C++ COMDAT functions can be made local > + at the expense of possible code size growth if function is used in multiple > + compilation units. */ > + bool can_be_local_p (void); > + > + /* Return true when cgraph_node can not return or throw and thus > + it is safe to ignore its side effects for IPA analysis. */ > + bool cannot_return_p (void); > + > + /* Return true when function cgraph_node and all its aliases are only called > + directly. > + i.e. it is not externally visible, address was not taken and > + it is not used in any other non-standard way. */ > + bool only_called_directly_p (void); > + > + /* Return true when function cgraph_node can be expected to be removed > + from program when direct calls in this compilation unit are removed. > + > + As a special case COMDAT functions are > + cgraph_can_remove_if_no_direct_calls_p while the are not > + cgraph_only_called_directly_p (it is possible they are called from other > + unit) > + > + This function behaves as cgraph_only_called_directly_p because eliminating > + all uses of COMDAT function does not make it necessarily disappear from > + the program unless we are compiling whole program or we do LTO. In this > + case we know we win since dynamic linking will not really discard the > + linkonce section. */ > + > + bool will_be_removed_from_program_if_no_direct_calls_p (void); > + > + /* Return true when function can be removed from callgraph > + if all direct calls are eliminated. */ > + bool can_remove_if_no_direct_calls_and_refs_p (void); > + > + /* Return true when function cgraph_node and its aliases can be removed from > + callgraph if all direct calls are eliminated. */ > + bool can_remove_if_no_direct_calls_p (void); > + > + /* Get the cgraph_function_version_info node corresponding to node. */ > + struct cgraph_function_version_info *function_version (void); > > Group it with the version creation. > + > + /* Dump the callgraph to file F. */ > + static void dump_cgraph (FILE *f); > + > + /* Dump the call graph to stderr. */ > + static inline void debug_cgraph (void) > + { > + dump_cgraph (stderr); > + } > > Dumping and debug should go first into the basic manipulation. > + > + /* Record that DECL1 and DECL2 are semantically identical function > + versions. */ > + static void record_function_versions (tree decl1, tree decl2); > + > + /* Remove the cgraph_function_version_info and cgraph_node for DECL. This > + DECL is a duplicate declaration. */ > + static void delete_function_version (tree decl); > > also these two > + > + /* Add the function FNDECL to the call graph. > + Unlike cgraph_finalize_function, this function is intended to be used > + by middle end and allows insertion of new function at arbitrary point > + of compilation. The function can be either in high, low or SSA form > + GIMPLE. > + > + The function is assumed to be reachable and have address taken (so no > + API breaking optimizations are performed on it). > + > + Main work done by this function is to enqueue the function for later > + processing to avoid need the passes to be re-entrant. */ > + static void add_new_function (tree fndecl, bool lowered); > + > + /* Return callgraph node for given symbol and check it is a function. */ > + static inline cgraph_node *get (const_tree decl) > + { > + gcc_checking_assert (TREE_CODE (decl) == FUNCTION_DECL); > + return dyn_cast (symtab_node::get (decl)); > + } > + > + /* Return cgraph node assigned to DECL. Create new one when needed. */ > + static cgraph_node * create (tree decl); > > So should these three > + > + /* Allocate new callgraph node and insert it into basic data structures. */ > + static cgraph_node * create_empty (void); All these functions are static and so are placed after all member functions. Is that coding style rule correct? > > private/protected? Usages from outside of cgraph_node: cgraph.c:cgraph_node::create_empty (void) cgraph.c: struct cgraph_node *node = cgraph_node::create_empty (); cgraphclones.c: struct cgraph_node *new_node = cgraph_node::create_empty (); lto-cgraph.c: node = cgraph_node::create_empty (); I will go through class members and check grouping one more. What do you think about 'static' class functions, should be placed after all member functions? Thanks, Martin > + > + /* Try to find a call graph node for declaration DECL and if it does not > + exist or if it corresponds to an inline clone, create a new one. */ > + static cgraph_node * get_create (tree); > > Also to begginig, next to get and create. > + > + /* Return the cgraph node that has ASMNAME for its DECL_ASSEMBLER_NAME. > + Return NULL if there's no such node. */ > + static cgraph_node *get_for_asmname (tree asmname); > > Likewise > + > + /* Attempt to mark ALIAS as an alias to DECL. Return alias node if > + successful and NULL otherwise. > + Same body aliases are output whenever the body of DECL is output, > + and cgraph_node::get (ALIAS) transparently > + returns cgraph_node::get (DECL). */ > + static cgraph_node * create_same_body_alias (tree alias, tree decl); > > To alias API > + > + /* Worker for cgraph_can_remove_if_no_direct_calls_p. */ > + static bool used_from_object_file_p_worker (cgraph_node *node, > + void *data ATTRIBUTE_UNUSED) > + { > + return node->used_from_object_file_p (); > + } > + > + /* Return true when cgraph_node can not be local. > + Worker for cgraph_local_node_p. */ > cgraph_local_node_p was probably renamed, but we should sanify predicates here. > Please group all functions dealing with local functions togehter, too. > + static bool non_local_p (cgraph_node *node, void *data ATTRIBUTE_UNUSED); > + > + /* Verify whole cgraph structure. */ > + static void DEBUG_FUNCTION verify_cgraph_nodes (void); > + > + /* Worker to bring NODE local. */ > + static bool make_local (cgraph_node *node, void *data ATTRIBUTE_UNUSED); > + > + /* Mark ALIAS as an alias to DECL. DECL_NODE is cgraph node representing > + the function body is associated > + with (not necessarily cgraph_node (DECL). */ > + static cgraph_node *create_alias (tree alias, tree target); > + > + static cgraph_edge * create_edge (cgraph_node *caller, cgraph_node *callee, > + gimple call_stmt, gcov_type count, > + int freq, > + bool indir_unknown_callee); > > Also edges and aliases should be grouped. > > With these changes patch looks good. Probably we will need one additional cleanup pass, but it would be better > to do it incrementally. Please write changelog that carefuly records what was renamed to what. > > Honza