On Tue, Oct 30, 2012 at 1:20 PM, 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?
>
> Could you add the explanation at the top of your message in the comments
> here?
Some more comments would indeed be nice. The patch is ok with that
and the other suggestions from Diego if Honza doesn't have any further
comments on the cgraph parts.
Thanks,
Richard.
> I particularly liked this section:
>
>
>> 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:
>
>
>> +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()?
>
> I think I prefer the form used for 've'.
>
>
>> +
>> +/* Determine if a symbol is finalized and needed. */
>
>
> s/a symbol/symbol NODE/.
>
>
>> +
>> +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.
>
>
> Diego.
