[PATCH] vectorizer: add _bb_vec_info::const_iterator

[Richard Sandiford]

Martin Liška <mliska@suse.cz> writes:
> > Also, one minor formatting nit, sorry: the other functions instead
> > indent the “{” block by the same amount as the function prototype,
> > which seems more consistent with the usual out-of-class formatting.
>
> Hope I fixed that.

Sorry, I meant the other functions were IMO formatted correctly, with the
“{” directly under the function name.  It looks like the new patch instead
indents all “{” by two spaces relative to the function name or “struct”
keyword.  I.e. IMO:

  struct const_iterator
  {
  };

seems better than:
  
  struct const_iterator
    {
    };

and:

  const const_iterator &
  operator++ ()
  {
  }

seems better than:

  const const_iterator &
  operator++ ()
    {
    }

This makes the formatting consistent with definitions in column 0.

> About rbiener's comment, I wrapper the iterators with bb_vinfo::region_stmts..

Sorry for dragging this on longer, but…

> @@ -5120,20 +5120,14 @@ vect_determine_precisions (vec_info *vinfo)
>    else
>      {
>        bb_vec_info bb_vinfo = as_a <bb_vec_info> (vinfo);
> -      gimple_stmt_iterator si = bb_vinfo->region_end;
> -      gimple *stmt;
> -      do
> +      for ( _bb_vec_info::reverse_iterator it = bb_vinfo->region_stmts.rbegin ();
> +	   it != bb_vinfo->region_stmts.rend (); ++it)
>  	{
> -	  if (!gsi_stmt (si))
> -	    si = gsi_last_bb (bb_vinfo->bb);
> -	  else
> -	    gsi_prev (&si);
> -	  stmt = gsi_stmt (si);
> +	  gimple *stmt = *it;
>  	  stmt_vec_info stmt_info = vinfo->lookup_stmt (stmt);
>  	  if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
>  	    vect_determine_stmt_precisions (vinfo, stmt_info);
>  	}
> -      while (stmt != gsi_stmt (bb_vinfo->region_begin));
>      }
>  }

I think this should be a similar style, i.e.

     for (gimple *stmt : bb_vinfo->reverse_region_stmts ())

rather than using iterators directly.

> @@ -5492,10 +5486,8 @@ vect_pattern_recog (vec_info *vinfo)
>    else
>      {
>        bb_vec_info bb_vinfo = as_a <bb_vec_info> (vinfo);
> -      for (si = bb_vinfo->region_begin;
> -	   gsi_stmt (si) != gsi_stmt (bb_vinfo->region_end); gsi_next (&si))
> +      for (gimple *stmt : bb_vinfo->region_stmts)
>  	{
> -	  gimple *stmt = gsi_stmt (si);
>  	  stmt_vec_info stmt_info = bb_vinfo->lookup_stmt (stmt);
>  	  if (!stmt_info || !STMT_VINFO_VECTORIZABLE (stmt_info))
>  	    continue;
> diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
> index 303410c2fc4..f4809c2207d 100644
> --- a/gcc/tree-vect-slp.c
> +++ b/gcc/tree-vect-slp.c
> @@ -2546,20 +2546,15 @@ vect_detect_hybrid_slp (loop_vec_info loop_vinfo)
>  /* Initialize a bb_vec_info struct for the statements between
>     REGION_BEGIN_IN (inclusive) and REGION_END_IN (exclusive).  */
>  
> -_bb_vec_info::_bb_vec_info (gimple_stmt_iterator region_begin_in,
> -			    gimple_stmt_iterator region_end_in,
> +_bb_vec_info::_bb_vec_info (gimple_stmt_iterator region_begin,
> +			    gimple_stmt_iterator region_end,
>  			    vec_info_shared *shared)
>    : vec_info (vec_info::bb, init_cost (NULL), shared),
> -    bb (gsi_bb (region_begin_in)),
> -    region_begin (region_begin_in),
> -    region_end (region_end_in)
> +    bb (gsi_bb (region_begin)),
> +    region_stmts (region_begin, region_end)
>  {
> -  gimple_stmt_iterator gsi;
> -
> -  for (gsi = region_begin; gsi_stmt (gsi) != gsi_stmt (region_end);
> -       gsi_next (&gsi))
> +  for (gimple *stmt : this->region_stmts)
>      {
> -      gimple *stmt = gsi_stmt (gsi);
>        gimple_set_uid (stmt, 0);
>        if (is_gimple_debug (stmt))
>  	continue;
> @@ -2575,10 +2570,9 @@ _bb_vec_info::_bb_vec_info (gimple_stmt_iterator region_begin_in,
>  
>  _bb_vec_info::~_bb_vec_info ()
>  {
> -  for (gimple_stmt_iterator si = region_begin;
> -       gsi_stmt (si) != gsi_stmt (region_end); gsi_next (&si))
> +  for (gimple *stmt : this->region_stmts)
>      /* Reset region marker.  */
> -    gimple_set_uid (gsi_stmt (si), -1);
> +    gimple_set_uid (stmt, -1);
>  
>    bb->aux = NULL;
>  }
> @@ -3012,16 +3006,13 @@ vect_bb_vectorization_profitable_p (bb_vec_info bb_vinfo)
>  static void
>  vect_slp_check_for_constructors (bb_vec_info bb_vinfo)
>  {
> -  gimple_stmt_iterator gsi;
> -
> -  for (gsi = bb_vinfo->region_begin;
> -       gsi_stmt (gsi) != gsi_stmt (bb_vinfo->region_end); gsi_next (&gsi))
> +  for (gimple *stmt : bb_vinfo->region_stmts)
>      {
> -      gassign *stmt = dyn_cast <gassign *> (gsi_stmt (gsi));
> -      if (!stmt || gimple_assign_rhs_code (stmt) != CONSTRUCTOR)
> +      gassign *assign = dyn_cast <gassign *> (stmt);
> +      if (!assign || gimple_assign_rhs_code (assign) != CONSTRUCTOR)
>  	continue;
>  
> -      tree rhs = gimple_assign_rhs1 (stmt);
> +      tree rhs = gimple_assign_rhs1 (assign);
>        if (!VECTOR_TYPE_P (TREE_TYPE (rhs))
>  	  || maybe_ne (TYPE_VECTOR_SUBPARTS (TREE_TYPE (rhs)),
>  		       CONSTRUCTOR_NELTS (rhs))
> @@ -3029,7 +3020,7 @@ vect_slp_check_for_constructors (bb_vec_info bb_vinfo)
>  	  || uniform_vector_p (rhs))
>  	continue;
>  
> -      stmt_vec_info stmt_info = bb_vinfo->lookup_stmt (stmt);
> +      stmt_vec_info stmt_info = bb_vinfo->lookup_stmt (assign);
>        BB_VINFO_GROUPED_STORES (bb_vinfo).safe_push (stmt_info);
>      }
>  }
> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
> index 6c830ad09f4..c94817e6af6 100644
> --- a/gcc/tree-vectorizer.h
> +++ b/gcc/tree-vectorizer.h
> @@ -787,12 +787,92 @@ loop_vec_info_for_loop (class loop *loop)
>  typedef class _bb_vec_info : public vec_info
>  {
>  public:
> +  struct const_iterator
> +    {

“{” should be directly under “struct”.

> +      const_iterator (gimple_stmt_iterator _gsi) : gsi (_gsi) {}
> +
> +      const const_iterator &
> +      operator++ ()
> +	{

“{” should be directly under “operator++”.  Same for the other structs
and functions.

> +	  gsi_next (&gsi); return *this;
> +	}
> +
> +      gimple *operator* () const { return gsi_stmt (gsi); }
> +
> +      bool
> +      operator== (const const_iterator& other) const
> +	{
> +	  return gsi_stmt (gsi) == gsi_stmt (other.gsi);
> +	}
> +
> +      bool
> +      operator!= (const const_iterator& other) const
> +	{
> +	  return !(*this == other);
> +	}
> +
> +      gimple_stmt_iterator gsi;
> +    };
> +
> +  struct reverse_iterator

This should be a const_reverse_iterator.

> +    {
> +      reverse_iterator (gimple_stmt_iterator _gsi) : gsi (_gsi) {}
> +
> +      const reverse_iterator &
> +      operator++ ()
> +	{
> +	  gsi_prev (&gsi); return *this;
> +	}
> +
> +      gimple *operator* () const { return gsi_stmt (gsi); }
> +
> +      bool
> +      operator== (const reverse_iterator& other) const
> +	{
> +	  return gsi_stmt (gsi) == gsi_stmt (other.gsi);
> +	}
> +
> +      bool
> +      operator!= (const reverse_iterator& other) const
> +	{
> +	  return !(*this == other);
> +	}
> +
> +      gimple_stmt_iterator gsi;
> +    };
> +
> +  struct stmt_iterator
> +    {
> +      stmt_iterator (gimple_stmt_iterator region_begin,
> +		     gimple_stmt_iterator region_end)
> +      : m_region_begin (region_begin), m_region_end (region_end) {}
> +
> +      gimple_stmt_iterator region_begin () { return m_region_begin; }
> +
> +      const_iterator begin () const { return const_iterator (m_region_begin); }
> +      const_iterator end () const { return const_iterator (m_region_end); }
> +
> +      gimple_stmt_iterator m_region_begin;
> +      gimple_stmt_iterator m_region_end;
> +
> +      reverse_iterator rbegin () const
> +	{
> +	  reverse_iterator it = reverse_iterator (m_region_end);
> +	  if (*it == NULL)
> +	    return reverse_iterator (gsi_last_bb (m_region_end.bb));
> +	  else
> +	    return ++it;
> +	}
> +
> +      reverse_iterator rend () const { return reverse_iterator (m_region_begin); }
> +    };

I think for this we should have a template class for begin/end iterator
pairs, probably in coretypes.h.  We could call it “iterator_pair” or
something.  Then “region_stmts” would return (see below) a:

   iterator_pair<const_iterator>

and “reverse_region_stmts” would return:

   iterator_pair<const_reverse_iterator>

> +
> +  basic_block bb;
> +  stmt_iterator region_stmts;

Might be wrong, but I think the suggestion was instead for region_stmts
to be a function that returns one view of the contents.  There could be
other views too, such as reverse_region_stmts above.  So I think it makes
sense to keep “bb”, “region_begin” and “region_end” in the main class.

In:

> diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
> index cdd6f6c5e5d..221ac072056 100644
> --- a/gcc/tree-vect-stmts.c
> +++ b/gcc/tree-vect-stmts.c
> @@ -1342,7 +1342,8 @@ vect_init_vector_1 (vec_info *vinfo, stmt_vec_info stmt_vinfo, gimple *new_stmt,
>        else
>         {
>            bb_vec_info bb_vinfo = dyn_cast <bb_vec_info> (vinfo);
> -	  gimple_stmt_iterator gsi_region_begin = bb_vinfo->region_begin;
> +	  gimple_stmt_iterator gsi_region_begin
> +	    = bb_vinfo->region_stmts.region_begin ();
>  	  gsi_insert_before (&gsi_region_begin, new_stmt, GSI_SAME_STMT);
>         }
>      }

IMO this should be a direct member function of bb_vinfo, rather than going
through region_stmts.

Thanks,
Richard