On 6/16/20 4:14 PM, Richard Sandiford wrote:
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”.
All right, you provided a nice hints! There's updated version of the patch.
Patch can bootstrap on x86_64-linux-gnu and survives regression tests.
Ready to be installed?
Thanks,
Martin
Thanks,
Richard
>From dd74517410a317986856e43eca505d5817ff7146 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 iterators
gcc/ChangeLog:
* coretypes.h (struct iterator_pair): New.
* tree-vect-patterns.c (vect_determine_precisions):
Use reverse_const_iterator and new range-based loop format.
(vect_pattern_recog): Use new range-based loop.
* tree-vect-slp.c (_bb_vec_info::_bb_vec_info): Likewise.
(_bb_vec_info::~_bb_vec_info): Likewise.
(vect_slp_check_for_constructors): Use assign variable instead
of stmt.
* tree-vectorizer.h: Add const_iterator and
reverse_const_iterator for region iteration.
---
gcc/coretypes.h | 17 ++++++++
gcc/tree-vect-patterns.c | 14 +------
gcc/tree-vect-slp.c | 24 ++++-------
gcc/tree-vectorizer.h | 88 +++++++++++++++++++++++++++++++++++++++-
4 files changed, 113 insertions(+), 30 deletions(-)
diff --git a/gcc/coretypes.h b/gcc/coretypes.h
index cda22697cc3..8a7990a7dd8 100644
--- a/gcc/coretypes.h
+++ b/gcc/coretypes.h
@@ -359,6 +359,23 @@ struct kv_pair
const ValueType value; /* the value of the name */
};
+/* Iterator pair used for a collection iteration with range-based loops. */
+
+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;
+};
+
#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 303410c2fc4..b4dd93775b3 100644
--- a/gcc/tree-vect-slp.c
+++ b/gcc/tree-vect-slp.c
@@ -2554,12 +2554,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;
@@ -2575,10 +2571,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 +3007,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 +3021,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..742d693b84a 100644
--- a/gcc/tree-vectorizer.h
+++ b/gcc/tree-vectorizer.h
@@ -787,12 +787,96 @@ loop_vec_info_for_loop (class loop *loop)
typedef class _bb_vec_info : public vec_info
{
public:
- _bb_vec_info (gimple_stmt_iterator, gimple_stmt_iterator, vec_info_shared *);
- ~_bb_vec_info ();
+
+ /* 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 reverse_const_iterator
+ {
+ reverse_const_iterator (gimple_stmt_iterator _gsi) : gsi (_gsi) {}
+
+ const reverse_const_iterator &
+ operator++ ()
+ {
+ gsi_prev (&gsi); return *this;
+ }
+
+ gimple *operator* () const { return gsi_stmt (gsi); }
+
+ bool
+ operator== (const reverse_const_iterator& other) const
+ {
+ return gsi_stmt (gsi) == gsi_stmt (other.gsi);
+ }
+
+ bool
+ operator!= (const reverse_const_iterator& other) const
+ {
+ return !(*this == other);
+ }
+
+ gimple_stmt_iterator gsi;
+ };
+
+ /* Returns iterator_pair for range-based loop. */
+
+ iterator_pair<const_iterator>
+ region_stmts ()
+ {
+ return iterator_pair<const_iterator> (const_iterator (region_begin),
+ const_iterator (region_end));
+ }
+
+ /* Returns iterator_pair for range-based loop in a reverse order. */
+
+ iterator_pair<reverse_const_iterator>
+ reverse_region_stmts ()
+ {
+ reverse_const_iterator begin = reverse_const_iterator (region_end);
+ if (*begin == NULL)
+ begin = reverse_const_iterator (gsi_last_bb (region_end.bb));
+ else
+ ++begin;
+ reverse_const_iterator end = reverse_const_iterator (region_begin);
+
+ return iterator_pair<reverse_const_iterator> (begin, end);
+ }
basic_block bb;
gimple_stmt_iterator region_begin;
gimple_stmt_iterator region_end;
+
+ _bb_vec_info (gimple_stmt_iterator, gimple_stmt_iterator, vec_info_shared *);
+ ~_bb_vec_info ();
+
} *bb_vec_info;
#define BB_VINFO_BB(B) (B)->bb
--
2.27.0
