Martin Liška <mli...@suse.cz> writes:
> On 6/16/20 10:50 AM, Richard Sandiford wrote:
>> Martin Liška <mli...@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”.
Thanks,
Richard
