Re: [patch] Apply conditional down cast to cgraph.h et.al.

[Lawrence Crowl <crowl at googlers dot com>]

On 10/30/12, Diego Novillo <dnovillo@google.com> wrote:
> On 2012-10-29 15:01 , Lawrence Crowl wrote:
>> On 10/27/12, Marc Glisse <marc.glisse@inria.fr> wrote:
>>> On Fri, 26 Oct 2012, Lawrence Crowl wrote:
>>>> 2012-10-26  Lawrence Crowl  <crowl@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  <crowl@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