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? 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.
