On 6/18/20 11:43 AM, Jonathan Wakely wrote:
On 18/06/20 00:55 +0100, Jonathan Wakely wrote:
Does your reverse iterator work correctly? It looks to me like it will
fail to visit the region_begin statement, because this loop will
terminate when the reverse iterator reaches the end() value, and will
not actually process that gsi:
- gimple_stmt_iterator si = bb_vinfo->region_end;
- gimple *stmt;
- do
+ for (gimple *stmt : bb_vinfo->reverse_region_stmts ())
Adding const_iterator::operator--() and using
std::reverse_iterator<const_iterator> would fix that problem. The
std::reverse_iterator utility already solves the problem of getting
the begin and end of a reversed range correct.
You might need to add a couple of things to your const_iterator to
make it more like a conventional STL iterator in order to use
std::reverse_iterator, but IMHO it would be worth it. I can look at
that in the morning.
Using std::reverse_iterator won't work, because this iterator is an
"input iterator" i.e. single pass. Incrementing it modifies the
underlying data structure (the gimple_statement_iterator) so it can't
meet the requirements for a bidirectional iterator.
But fixing the reverse iterator to visit the last statement isn't
hard.
Nice catch from Jonathan!
I'm leaving the idea of std::reverse_iterator and make_iterator_range and I'm
testing
a patch version that I'll install (if there are no objections).
N.B. the fact that the bug in reverse iteration wasn't noticed
suggests missing tests :-)
Yes, so far we don't have any selftests for _bb_vec_info and friends.
Martin
>From 15ac28bcb05bfbdebb958bef0b43e8e60d3cc2cd Mon Sep 17 00:00:00 2001
From: Martin Liska <mli...@suse.cz>
Date: Thu, 11 Jun 2020 13:25:40 +0200
Subject: [PATCH] vectorizer: add _bb_vec_info::region_stmts and
reverse_region_stmts
gcc/ChangeLog:
* coretypes.h (struct iterator_range): New type.
* tree-vect-patterns.c (vect_determine_precisions): Use
range-based iterator.
(vect_pattern_recog): Likewise.
* tree-vect-slp.c (_bb_vec_info): Likewise.
(_bb_vec_info::~_bb_vec_info): Likewise.
(vect_slp_check_for_constructors): Likewise.
* tree-vectorizer.h:Add new iterators
and functions that use it.
---
gcc/coretypes.h | 17 +++++++++
gcc/tree-vect-patterns.c | 14 +------
gcc/tree-vect-slp.c | 24 ++++--------
gcc/tree-vectorizer.h | 82 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 109 insertions(+), 28 deletions(-)
diff --git a/gcc/coretypes.h b/gcc/coretypes.h
index 01ec2e23ce2..720f9f9c63f 100644
--- a/gcc/coretypes.h
+++ b/gcc/coretypes.h
@@ -363,6 +363,23 @@ struct kv_pair
template<typename T1, typename T2>
using first_type = T1;
+/* Iterator pair used for a collection iteration with range-based loops. */
+
+template<typename T>
+struct iterator_range
+{
+public:
+ iterator_range (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;
+};
+
#else
struct _dont_use_rtx_here_;
diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index 636ad59c001..03d50ec5c90 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->reverse_region_stmts ())
{
- 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));
}
}
@@ -5492,10 +5484,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 fe946738a97..09cdc353a43 100644
--- a/gcc/tree-vect-slp.c
+++ b/gcc/tree-vect-slp.c
@@ -2561,12 +2561,8 @@ _bb_vec_info::_bb_vec_info (gimple_stmt_iterator region_begin_in,
region_begin (region_begin_in),
region_end (region_end_in)
{
- 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;
@@ -2582,10 +2578,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;
}
@@ -3019,16 +3014,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))
@@ -3036,7 +3028,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..9755cfb9c0e 100644
--- a/gcc/tree-vectorizer.h
+++ b/gcc/tree-vectorizer.h
@@ -787,9 +787,91 @@ loop_vec_info_for_loop (class loop *loop)
typedef class _bb_vec_info : public vec_info
{
public:
+
+ /* GIMPLE statement iterator going from region_begin to region_end. */
+
+ struct const_iterator
+ {
+ const_iterator (gimple_stmt_iterator _gsi) : gsi (_gsi) {}
+
+ const const_iterator &
+ operator++ ()
+ {
+ 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;
+ };
+
+ /* GIMPLE statement iterator going from region_end to region_begin. */
+
+ struct const_reverse_iterator
+ {
+ const_reverse_iterator (gimple_stmt_iterator _gsi) : gsi (_gsi) {}
+
+ const const_reverse_iterator &
+ operator++ ()
+ {
+ gsi_prev (&gsi); return *this;
+ }
+
+ gimple *operator* () const { return gsi_stmt (gsi); }
+
+ bool
+ operator== (const const_reverse_iterator &other) const
+ {
+ return gsi_stmt (gsi) == gsi_stmt (other.gsi);
+ }
+
+ bool
+ operator!= (const const_reverse_iterator &other) const
+ {
+ return !(*this == other);
+ }
+
+ gimple_stmt_iterator gsi;
+ };
+
_bb_vec_info (gimple_stmt_iterator, gimple_stmt_iterator, vec_info_shared *);
~_bb_vec_info ();
+ /* Returns iterator_range for range-based loop. */
+
+ iterator_range<const_iterator>
+ region_stmts ()
+ {
+ return iterator_range<const_iterator> (region_begin, region_end);
+ }
+
+ /* Returns iterator_range for range-based loop in a reverse order. */
+
+ iterator_range<const_reverse_iterator>
+ reverse_region_stmts ()
+ {
+ const_reverse_iterator begin = region_end;
+ if (*begin == NULL)
+ begin = const_reverse_iterator (gsi_last_bb (region_end.bb));
+ else
+ ++begin;
+
+ const_reverse_iterator end = region_begin;
+ return iterator_range<const_reverse_iterator> (begin, ++end);
+ }
+
basic_block bb;
gimple_stmt_iterator region_begin;
gimple_stmt_iterator region_end;
--
2.27.0
