[PATCH] vectorizer: add _bb_vec_info::const_iterator

[Richard Sandiford]

Martin Liška <mliska@suse.cz> writes:
> On 6/16/20 4:14 PM, Richard Sandiford wrote:
>> Martin Liška <mliska@suse.cz> writes:
>>> 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
>
> [...]
> +    gimple *operator* () const { return gsi_stmt (gsi); }
> +
> +    bool
> +    operator== (const const_iterator& other) const

I think the usual formatting in GCC (though not elsewhere) is to put the
space before rather than after the “&”, like for pointers.  Same for the
rest of the patch.

> +    {
> +      return gsi_stmt (gsi) == gsi_stmt (other.gsi);
> +    }
> +
> +    bool
> +    operator!= (const const_iterator& other) const
> +    {
> +      return !(*this == other);
> +    }
> +
> +    gimple_stmt_iterator gsi;
> +  };
> +
> +  /* GIMPLE statement iterator going from region_end to region_begin.  */
> +
> +  struct reverse_const_iterator

The standard name for this abstraction is const_reverse_iterator rather
than reverse_const_iterator.

> +  {
> +    reverse_const_iterator (gimple_stmt_iterator _gsi) : gsi (_gsi) {}
> +
> +    const reverse_const_iterator &
> +    operator++ ()
> +    {
> +      gsi_prev (&gsi); return *this;
> +    }
> +
> +    gimple *operator* () const { return gsi_stmt (gsi); }
> +
> +    bool
> +    operator== (const reverse_const_iterator& other) const
> +    {
> +      return gsi_stmt (gsi) == gsi_stmt (other.gsi);
> +    }
> +
> +    bool
> +    operator!= (const reverse_const_iterator& other) const
> +    {
> +      return !(*this == other);
> +    }
> +
> +    gimple_stmt_iterator gsi;
> +  };
> +
> +  /* Returns iterator_pair for range-based loop.  */
> +
> +  iterator_pair<const_iterator>
> +  region_stmts ()
> +  {
> +    return iterator_pair<const_iterator> (const_iterator (region_begin),
> +					  const_iterator (region_end));

The const_iterator(…)s aren't necessary here, just:

    return iterator_pair<const_iterator> (region_begin, region_end);

is IMO more readable.

> +  }
> +
> +  /* Returns iterator_pair for range-based loop in a reverse order.  */
> +
> +  iterator_pair<reverse_const_iterator>
> +  reverse_region_stmts ()
> +  {
> +    reverse_const_iterator begin = reverse_const_iterator (region_end);
> +    if (*begin == NULL)
> +      begin = reverse_const_iterator (gsi_last_bb (region_end.bb));
> +    else
> +      ++begin;

Similarly here we don't need the reverse_const_iterator(…)s
(= const_reverse_iterator(…)s after the change above).  It can just be:

    const_reverse_iterator begin = region_end;
    if (*begin == NULL)
      begin = gsi_last_bb (region_end.bb);
    else
      ++begin;

> +    reverse_const_iterator end = reverse_const_iterator (region_begin);
> +
> +    return iterator_pair<reverse_const_iterator> (begin, end);

Not sure the “end” temporary really buys much, could just be:

    return iterator_pair<const_reverse_iterator> (begin, region_begin);

> +  }
>  
>    basic_block bb;
>    gimple_stmt_iterator region_begin;
>    gimple_stmt_iterator region_end;
> +
> +  _bb_vec_info (gimple_stmt_iterator, gimple_stmt_iterator, vec_info_shared *);
> +  ~_bb_vec_info ();
> +
>  } *bb_vec_info;

I realise it just ended up this way through the various iterations,
but I think the constructor and destructor should go between the
new types and the new member functions, rather than at the end.
The coding conventions say;

- first define all public types,
- then define all non-public types,
- then declare all public constructors,
- then declare the public destructor,
- then declare all public member functions,
- then declare all public member variables,

There should be no blank line before the final “}”.

OK with those changes, and thanks for your patience :-)

Richard