On 6/12/20 11:49 AM, Richard Sandiford wrote:
Martin Liška <mli...@suse.cz> writes:
On 6/12/20 3:22 PM, Richard Sandiford wrote:
Martin Liška <mli...@suse.cz> writes:
diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index 636ad59c001..eac372e6abc 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -5120,20 +5120,12 @@ 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 (gimple *stmt: *bb_vinfo)
{
- if (!gsi_stmt (si))
- si = gsi_last_bb (bb_vinfo->bb);
- else
- gsi_prev (&si);
- stmt = gsi_stmt (si);
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));
}
}
This loop wants a reverse iterator: it starts at the end and walks
backwards. That's important because vect_determine_stmt_precisions
acts based on information recorded about later uses.
I thought that it may be a problematic change. Note that we don't a have
test coverage for the situation in testsuite (on x86_64). So I'm also
introducing reverse_iterator for this.
There's definitely coverage on aarch64. Thought there would be on x86
too for the PAVG stuff, but obviously not.
diff --git a/gcc/tree-vect-stmts.c b/gcc/tree-vect-stmts.c
index cdd6f6c5e5d..766598862d4 100644
--- a/gcc/tree-vect-stmts.c
+++ b/gcc/tree-vect-stmts.c
@@ -1342,7 +1342,7 @@ 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->begin ().gsi;
gsi_insert_before (&gsi_region_begin, new_stmt, GSI_SAME_STMT);
}
}
Feels like this kind-of breaks the abstraction a bit.
Would it make sense instead to add the operators to gimple_stmt_iterator
itself and just make const_iterator a typedef of that?
Well, I'm planning to use the _bb_vec_info::const_iterator to jump in between
BBs, so it can't be a simple typedef.
But what happens to this code when that happens? Is inserting at
begin().gsi meaningful? I.e. is the stmt at *begin() guaranteed
to dominate all the SLP code even with multple BBs?
Personally I'd prefer either:
(a) const_iterator starts out as a typedef of gimple_stmt_iterator,
and with your later changes becomes a derived class of it; or
(b) we just provide a region_begin() function that returns the gsi
directly.
+ 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;
+ }
I think the else case should return “++it” instead, since AIUI
m_region_end is an exclusive rather than inclusive endpoint.
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.
Thanks,
Richard
Hi Martin,
I was curious do we ever plan to use r-values here or in code that
can iterator? There was a new version in C++11 for r values that
is basically the same, but the deference operator * is overloaded
to do std::move(value) rather than what you have currently. Otherwise
the other comments about having a reverse iterator are all I have.
Doesn't seem useful here but just wanted to point it out with the
move to C++11,
Nick
--
Fundamentally an organism has conscious mental states if and only if
there is something that it is like to be that organism--something it is
like for the organism. - Thomas Nagel
