[PATCH] vectorizer: add _bb_vec_info::const_iterator

Martin Liška mliska@suse.cz
Wed Jun 17 17:05:52 GMT 2020


On 6/16/20 4:14 PM, Richard Sandiford wrote:
> Martin Liška <mliska@suse.cz> writes:
>> On 6/16/20 10:50 AM, Richard Sandiford wrote:
>>> 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); }
>>>> +    };
>>
>> Thank you for the review. I'm skipping for now the renaming and formatting changes which
>> I'll do later.
>>
>>>
>>> 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>
>>
>> Hmm, sounds like a nice abstraction but I see 2 problems with that:
>>
>> 1) How can I define a constructor of the iterator_pair when I need something like:
>> iterator_pair<const_iterator> (region_begin, region_end)?
> 
> Not sure if I'm answering the right question, sorry, but I meant
> something along the lines of:
> 
>    template<typename T>
>    struct iterator_pair
>    {
>    public:
>      iterator_pair (const T &begin, const T &end)
>        : m_begin (begin), m_end (end) {}
> 
>      T begin () const { return m_begin; }
>      T end () const { return m_end; }
> 
>    private:
>      T m_begin;
>      T m_end;
>    };
> 
>> 2) rbegin function is more complex than begin function:
>>
>>         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;
>> 	}
>>
>>         const_iterator begin () const { return const_iterator (m_region_begin); }
>>
>> How can we abstract that?
> 
> With the change above, we'd replace “rbegin” and “rend”
> with a “reverse_region_stmts” function that returns an
> “iterator_pair<const_reverse_iterator>”.  Its “begin” iterator
> would have the value that “rbegin” calculates above and its “end”
> iterator would have the same value as the current patch's “rend”.

All right, you provided a nice hints! There's updated version of the patch.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

> 
> Thanks,
> Richard
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-vectorizer-add-_bb_vec_info-region_stmts-and-iterato.patch
Type: text/x-patch
Size: 7705 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20200617/d3468b0b/attachment-0001.bin>


More information about the Gcc-patches mailing list