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); }
> + };
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
