[PATCH] vectorizer: add _bb_vec_info::const_iterator
Martin Liška
mliska@suse.cz
Tue Jun 16 13:14:11 GMT 2020
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)?
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?
>
>> +
>> + 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.
I'm fine with that.
>
> 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.
Likewise here.
Martin
>
> Thanks,
> Richard
>
More information about the Gcc-patches
mailing list