This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [RFC, PATCH 3/n] IPA C++ refactoring
- From: Trevor Saunders <tsaunders at mozilla dot com>
- To: Martin LiÅka <mliska at suse dot cz>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, hubicka at ucw dot cz
- Date: Thu, 31 Jul 2014 20:34:54 -0400
- Subject: Re: [RFC, PATCH 3/n] IPA C++ refactoring
- Authentication-results: sourceware.org; auth=none
- References: <53DAB28F dot 9030803 at suse dot cz>
> +++ b/gcc/cgraph.h
> @@ -27,6 +27,7 @@ along with GCC; see the file COPYING3. If not see
> #include "basic-block.h"
> #include "function.h"
> #include "ipa-ref.h"
> +#include "indexed-set.h"
you don't actually use that here so why are you including it?
> +++ b/gcc/cgraphunit.c
> @@ -211,11 +211,12 @@ along with GCC; see the file COPYING3. If not see
> #include "tree-nested.h"
> #include "gimplify.h"
> #include "dbgcnt.h"
> +#include "indexed-set.h"
afaict you don't use it here either.
> @@ -529,9 +529,9 @@ cgraph_node::add_new_function (tree fndecl, bool lowered)
> }
> if (lowered)
> node->lowered = true;
> - if (!cgraph_new_nodes)
> - cgraph_new_nodes = cgraph_node_set_new ();
> - cgraph_node_set_add (cgraph_new_nodes, node);
> + if (!cgraph_new_nodes.exists ())
> + cgraph_new_nodes.create (4);
I don't think you need this, if the vector doesn't exist it will point
at null, and safe_push knows how to handle that.
> +++ b/gcc/indexed-set.h
> +template<typename T>
> +class indexed_set
per below I don't think you actually need this class after all.
> +{
> +public:
> + class iterator
> + {
> + public:
> + iterator (const vec<T> *v, unsigned index) : m_index (index), m_vector (v) {};
> +
> + inline T operator * ()
> + {
> + gcc_assert (m_index < m_vector->length ());
> +
> + return (*m_vector)[m_index];
> + }
> +
> + inline iterator &operator ++ ()
> + {
> + m_index++;
> + return *this;
> + }
> +
> + bool operator != (const iterator &other) const
> + {
> + return m_vector != other.m_vector || m_index != other.m_index;
> + }
> +
> + inline int
> + get_index (void) const
> + {
> + return m_index;
> + }
> +
> + private:
> + unsigned m_index;
> + const vec <T> *m_vector;
> + };
> +
> + /* Iterator to the start of data structure. */
> +
> + iterator begin () const
> + {
> + gcc_assert (!is_empty ());
it seems to me it would be better to just return the iterator for end ()
if there's no elements.
> +
> + iterator iter (&m_vector, 0);
> + return iter;
> + }
> +
> + /* Iterator to the end of data structure. */
> +
> + iterator end () const
> + {
> + return iterator (&m_vector, m_vector.length ());
> + }
> +
> + /* Add item V to data structure. */
> +
> + inline void
afaik defining within the class implies inline.
> + add (T v)
You are passing by value here, which you probably don't want to do if T
is big.
> + {
> + m_map.put (v, m_vector.length ());
> + m_vector.safe_push (v);
I think this blows up if v was already in the map, so assert it isn't?
> + }
> +
> + /* Return iterator for an item V. */
> +
> + inline iterator
> + find (T v)
> + {
> + int *item = m_map.get (v);
> + gcc_unreachable ();
> +
> + return item ? iterator (&m_vector, *item) : end ();
returning an iterator here seems silly, the only information you get is
the index and if the element exists in the map, so just return an int?
> + }
> +
> + /* Return if the data structure is empty. */
> +
> + inline bool
> + is_empty (void) const
> + {
> + return m_vector.is_empty ();
> + }
> +
> + /* Return number of items stored in data structure. */
> +
> + inline unsigned
> + length (void)
> + {
> + return m_vector.length ();
> + }
> +
> +
> +private:
> + hash_map<T, int> m_map;
> + auto_vec<T> m_vector;
I know you don't use this with anything but pointers now, but this will
do "weird" things if T is some sort of object.
> +};
> +
> +#endif
> diff --git a/gcc/tree-emutls.c b/gcc/tree-emutls.c
> index 89197c7..9c06aec 100644
> --- a/gcc/tree-emutls.c
> +++ b/gcc/tree-emutls.c
> @@ -42,6 +42,7 @@ along with GCC; see the file COPYING3. If not see
> #include "target.h"
> #include "targhooks.h"
> #include "tree-iterator.h"
> +#include "indexed-set.h"
>
>
> /* Whenever a target does not support thread-local storage (TLS) natively,
> @@ -70,7 +71,7 @@ along with GCC; see the file COPYING3. If not see
> /* These two vectors, once fully populated, are kept in lock-step so that
> the index of a TLS variable equals the index of its control variable in
> the other vector. */
> -static varpool_node_set tls_vars;
> +static indexed_set<varpool_node *> tls_vars;
Note that will create a static initializer that calls malloc, and tls
emulation probably isn't needed on many targets, so I'd guess your
better off using a hash_map * with new and delete as annoying as that is.
> static vec<varpool_node *> control_vars;
so, looking at this thing more closely it looks like this is just a
weirdo implementation of a hash map from varpool_node * to (varpool_node
*, tree) so I think you could just do
struct tls_var_data
{
varpool_node *control_var;
tree access;
};
hash_map<var_pool_node *, tls_var_data>
Thanks!
Trev