On 10/30/12, Diego Novillo <dnovi...@google.com> wrote: > On 2012-10-29 15:01 , Lawrence Crowl wrote: >> On 10/27/12, Marc Glisse <marc.gli...@inria.fr> wrote: >>> On Fri, 26 Oct 2012, Lawrence Crowl wrote: >>>> 2012-10-26 Lawrence Crowl <cr...@google.com >>> >>> missing '>' >> >> Fixed. >> >>>> * is-a.h: New. >>>> (is_a <T> (U*)): New. Test for is-a relationship. >>>> (as_a <T> (U*)): New. Treat as a derived type. >>>> (dyn_cast <T> (U*)): New. Conditionally cast based on is_a. >>> >>> I can't find this file in the patch... >> >> I forgot to svn add. Updated patch here. >> >> >> This patch implements generic type query and conversion functions, >> and applies them to the use of cgraph_node, varpool_node, and >> symtab_node. >> >> The functions are: >> >> bool is_a <TYPE> (pointer) >> Tests whether the pointer actually points to a more derived TYPE. >> >> TYPE *as_a <TYPE> (pointer) >> Converts pointer to a TYPE*. >> >> TYPE *dyn_cast <TYPE> (pointer) >> Converts pointer to TYPE* if and only if "is_a <TYPE> pointer". >> Otherwise, returns NULL. >> This function is essentially a checked down cast. >> >> These functions reduce compile time and increase type safety when treating >> a >> generic item as a more specific item. In essence, the code change is >> from >> >> if (symtab_function_p (node)) >> { >> struct cgraph_node *cnode = cgraph (node); >> .... >> } >> >> to >> >> if (cgraph_node *cnode = dyn_cast <cgraph_node> (node)) >> { >> .... >> } >> >> The necessary conditional test defines a variable that holds a known good >> pointer to the specific item and avoids subsequent conversion calls and >> the assertion checks that may come with them. >> >> When, the property test is embedded within a larger condition, the >> variable >> declaration gets pulled out of the condition. (This leaves some room for >> using the variable inappropriately.) >> >> if (symtab_variable_p (node) >> && varpool (node)->finalized) >> varpool_analyze_node (varpool (node)); >> >> becomes >> >> varpool_node *vnode = dyn_cast <varpool_node> (node); >> if (vnode && vnode->finalized) >> varpool_analyze_node (vnode); >> >> Note that we have converted two sets of assertions in the calls to >> varpool >> into safe and efficient use of a variable. >> >> >> There are remaining calls to symtab_function_p and symtab_variable_p that >> do not involve a pointer to a more specific type. These have been >> converted >> to calls to a functions is_a <cgraph_node> and is_a <varpool_node>. The >> original predicate functions have been removed. >> >> The cgraph.h header defined both a struct and a function with the name >> varpool_node. This name overloading can cause some unintuitive error >> messages >> when, as is common in C++, one omits the struct keyword when using the >> type. >> I have renamed the function to varpool_node_for_decl. >> >> Tested on x86_64. >> >> >> Okay for trunk? >> Index: gcc/ChangeLog >> >> 2012-10-29 Lawrence Crowl <cr...@google.com> >> >> * is-a.h: New. >> (is_a <T> (U*)): New. Test for is-a relationship. >> (as_a <T> (U*)): New. Treat as a derived type. >> (dyn_cast <T> (U*)): New. Conditionally cast based on is_a. >> * cgraph.h (varpool_node): Rename to varpool_node_for_decl. >> Adjust callers to match. >> (is_a_helper <cgraph_node>::test (symtab_node_def *)): New. >> (is_a_helper <varpool_node>::test (symtab_node_def *)): New. >> (symtab_node_def::try_function): New. Change most calls to >> symtab_function_p with calls to dyn_cast <cgraph_node> (p). >> (symtab_node_def::try_variable): New. Change most calls to >> symtab_variable_p with calls to dyn_cast <varpool_node> (p). >> (symtab_function_p): Remove. Change callers to use >> is_a <cgraph_node> (p) instead. >> (symtab_variable_p): Remove. Change callers to use >> is_a <varpool_node> (p) instead. >> * cgraph.c (cgraph_node_for_asm): Remove redundant call to >> symtab_node_for_asm. >> * cgraphunit.c (symbol_finalized_and_needed): New. >> (symbol_finalized): New. >> (cgraph_analyze_functions): Split complicated conditionals out into >> above new functions. >> * Makefile.in (CGRAPH_H): Add is-a.h as used by cgraph.h. > > Thanks. I really like this cleanup. I have a few questions and > comments below. Honza, the patch looks OK to me but it touches a bunch > of cgraph code, could you please go over it? > > >> Index: gcc/is-a.h >> =================================================================== >> --- gcc/is-a.h (revision 0) >> +++ gcc/is-a.h (revision 0) >> @@ -0,0 +1,118 @@ >> +/* Dynamic testing for abstract is-a relationships. >> + Copyright (C) 2012 Free Software Foundation, Inc. >> + Contributed by Lawrence Crowl. >> + >> +This file is part of GCC. >> + >> +GCC is free software; you can redistribute it and/or modify it under >> +the terms of the GNU General Public License as published by the Free >> +Software Foundation; either version 3, or (at your option) any later >> +version. >> + >> +GCC is distributed in the hope that it will be useful, but WITHOUT ANY >> +WARRANTY; without even the implied warranty of MERCHANTABILITY or >> +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License >> +for more details. >> + >> +You should have received a copy of the GNU General Public License >> +along with GCC; see the file COPYING3. If not see >> +<http://www.gnu.org/licenses/>. */ >> + >> + >> +/* >> + >> +Suppose you have a symtab_node_def *ptr, AKA symtab_node ptr. You can >> +test whether it points to a 'derived' cgraph_node as follows. >> + >> + if (is_a <cgraph_node> (ptr)) >> + .... >> + >> +You can just assume that it is such a node. >> + >> + do_something_with (as_a <cgraph_node> *ptr); >> + >> +You can test and obtain a pointer to the 'derived' type in one >> +indivisible operation. >> + >> + if (cgraph_node *cptr = dyn_cast <cgraph_node> (ptr)) >> + .... >> + >> + >> +If you use these functions and get a 'inline function not defined' or >> +a 'missing symbol' error message for 'is_a_helper<....>::test', it >> +means that the connection between the types has not been made. Each >> +connection between types must be made as follows. >> + >> + template <> >> + template <> >> + inline bool >> + is_a_helper <cgraph_node>::test (symtab_node_def *p) >> + { >> + return p->symbol.type == SYMTAB_FUNCTION; >> + } >> + >> + >> +If a simple reinterpret_cast between the types is incorrect, then you >> +must specialize the cast function. Failure to do so when needed may >> +result in a crash. >> + >> + template <> >> + template <> >> + inline bool >> + is_a_helper <cgraph_node>::cast (symtab_node_def *p) >> + { >> + return &p->x_function; >> + } >> + > > So, to use these three functions, the user must define this single > 'is_a_helper' routine? Nothing else?
You need to distinguish which kind user. Someone just wanting to convert does not need to know about the is_a_helper stuff. Someone wanting to extend the set of type relationships needs to provide one or two template specializations. I've modified the in-header documentation to better reflect the distinction. > Could you add the explanation at the top of your message in the comments > here? I particularly liked this section: I've merge the patch description into the in-header comments. > > bool is_a <TYPE> (pointer) > > Tests whether the pointer actually points to a more derived TYPE. > > > > TYPE *as_a <TYPE> (pointer) > > Converts pointer to a TYPE*. > > > > TYPE *dyn_cast <TYPE> (pointer) > > Converts pointer to TYPE* if and only if "is_a <TYPE> pointer". > > Otherwise, returns NULL. > > This function is essentially a checked down cast. > > > > These functions reduce compile time and increase type safety when > > treating a > > generic item as a more specific item. In essence, the code change is > > from > > > > if (symtab_function_p (node)) > > { > > struct cgraph_node *cnode = cgraph (node); > > .... > > } > > > > to > > > > if (cgraph_node *cnode = dyn_cast <cgraph_node> (node)) > > { > > .... > > } > > > >> +*/ >> + >> +#ifndef GCC_IS_A_H >> +#define GCC_IS_A_H >> + >> + >> +template <typename T> >> +struct is_a_helper >> +{ >> + template <typename U> >> + static inline bool test (U *p); >> + template <typename U> >> + static inline T *cast (U *p); >> +}; >> + > > Those definitions you described at the top of your message would be very > useful to have as lead-in comments to each of the functions below: Updated. > >> +template <typename T> >> +template <typename U> >> +inline T * >> +is_a_helper <T>::cast (U *p) >> +{ >> + return reinterpret_cast <T *> (p); >> +} >> + >> + >> +template <typename T, typename U> >> +inline bool >> +is_a (U *p) >> +{ >> + return is_a_helper<T>::test (p); >> +} >> + >> + >> +template <typename T, typename U> >> +inline T * >> +as_a (U *p) >> +{ >> + gcc_assert (is_a <T> (p)); >> + return is_a_helper <T>::cast (p); >> +} >> + >> + >> +template <typename T, typename U> >> +inline T * >> +dyn_cast (U *p) >> +{ >> + if (is_a <T> (p)) >> + return is_a_helper <T>::cast (p); >> + else >> + return static_cast <T *> (0); >> +} >> + >> +#endif /* GCC_IS_A_H */ >> Index: gcc/lto-symtab.c >> =================================================================== >> --- gcc/lto-symtab.c (revision 192956) >> +++ gcc/lto-symtab.c (working copy) >> @@ -532,11 +532,11 @@ lto_symtab_merge_cgraph_nodes_1 (symtab_ >> >> if (!symtab_real_symbol_p (e)) >> continue; >> - if (symtab_function_p (e) >> - && !DECL_BUILT_IN (e->symbol.decl)) >> - lto_cgraph_replace_node (cgraph (e), cgraph (prevailing)); >> - if (symtab_variable_p (e)) >> - lto_varpool_replace_node (varpool (e), varpool (prevailing)); > >> + cgraph_node *ce = dyn_cast <cgraph_node> (e); >> + if (ce && !DECL_BUILT_IN (e->symbol.decl)) >> + lto_cgraph_replace_node (ce, cgraph (prevailing)); >> + if (varpool_node *ve = dyn_cast <varpool_node> (e)) >> + lto_varpool_replace_node (ve, varpool (prevailing)); > > I see that you used dyn_cast<> differently here. The first time, 'ce' > is declared outside the if(), the second time, 've' is inside the if(). > Is this because 'ce' is used after the if()? It's because in the first case it is a composite conditional. > I think I prefer the form used for 've'. I originally had if (cgraph_node *ce = dyn_cast <cgraph_node> (e)) if (!DECL_BUILT_IN (e->symbol.decl)) lto_cgraph_replace_node (ce, cgraph (prevailing)); but folks objected to increasing the nesting, and asked that I change to the pre-declare form. >> + >> +/* Determine if a symbol is finalized and needed. */ > > s/a symbol/symbol NODE/. Fixed. > >> + >> +inline static bool >> +symbol_finalized_and_needed (symtab_node node) >> +{ >> + if (cgraph_node *cnode = dyn_cast <cgraph_node> (node)) >> + return cnode->local.finalized >> + && cgraph_decide_is_function_needed (cnode, cnode->symbol.decl); >> + if (varpool_node *vnode = dyn_cast <varpool_node> (node)) >> + return vnode->finalized >> + && !DECL_EXTERNAL (vnode->symbol.decl) >> + && decide_is_variable_needed (vnode, vnode->symbol.decl); >> + return false; >> +} >> + >> +/* Determine if a symbol is finalized. */ > > Likewise. Fixed. -- Lawrence Crowl