This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [RFC, PATCH 3/n] IPA C++ refactoring


> +++ 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]