vectorizer: add _bb_vec_info::const_iterator

Message ID 44462fa7-0fd1-9113-9c1c-c2065f8a926f@suse.cz
State New
Headers show
Series
  • vectorizer: add _bb_vec_info::const_iterator
Related show

Commit Message

Martin Liška June 12, 2020, 8:06 a.m.
Hello.

I'm working one extension of SLP which will allow to vectorize multiple
BBs. This is a first step where I abstract _bb_vec_info::region_begin and
_bb_vec_info::region end by providing an iterator.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

gcc/ChangeLog:

	* tree-vect-patterns.c (vect_determine_precisions): Use the
	iterator.
	(vect_pattern_recog): Likewise.
	* tree-vect-slp.c (_bb_vec_info::_bb_vec_info): Likewise.
	(_bb_vec_info::~_bb_vec_info): Likewise.
	(vect_slp_check_for_constructors): Likewise.
	* tree-vect-stmts.c (vect_init_vector_1): Likewise.
	* tree-vectorizer.h: Add the new iterator and all related functions.
---
  gcc/tree-vect-patterns.c | 10 +++++-----
  gcc/tree-vect-slp.c      | 24 +++++++++---------------
  gcc/tree-vect-stmts.c    |  2 +-
  gcc/tree-vectorizer.h    | 38 ++++++++++++++++++++++++++++++++++++--
  4 files changed, 51 insertions(+), 23 deletions(-)

-- 
2.26.2

Comments

Richard Sandiford June 12, 2020, 10:46 a.m. | #1
Martin Liška <mliska@suse.cz> writes:
> Hello.

>

> I'm working one extension of SLP which will allow to vectorize multiple

> BBs. This is a first step where I abstract _bb_vec_info::region_begin and

> _bb_vec_info::region end by providing an iterator.


Nice.

> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h

> index 6c830ad09f4..542d49402d2 100644

> --- a/gcc/tree-vectorizer.h

> +++ b/gcc/tree-vectorizer.h

> @@ -787,12 +787,46 @@ loop_vec_info_for_loop (class loop *loop)

>   typedef class _bb_vec_info : public vec_info

>   {

>   public:

> +  struct const_iterator

> +  {

> +    const_iterator (gimple_stmt_iterator _gsi): gsi (_gsi)

> +    {

> +    }

> +

> +    const_iterator &

> +    operator++ (int)

> +    {

> +      gsi_next (&gsi); return *this;

> +    }


Isn't this really operator++()?  I.e. it returns a reference to the
modified iterator instead of the value at the original iterator.

With that change, is there any reason we can't use a range-based for
loop instead of:

  for (_bb_vec_info::const_iterator it = begin (); it != end (); it++)

etc.?

Thanks,
Richard
Martin Liška June 12, 2020, 11:24 a.m. | #2
On 6/12/20 12:46 PM, Richard Sandiford wrote:
> Martin Liška <mliska@suse.cz> writes:

>> Hello.

>>

>> I'm working one extension of SLP which will allow to vectorize multiple

>> BBs. This is a first step where I abstract _bb_vec_info::region_begin and

>> _bb_vec_info::region end by providing an iterator.

> 

> Nice.

> 

>> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h

>> index 6c830ad09f4..542d49402d2 100644

>> --- a/gcc/tree-vectorizer.h

>> +++ b/gcc/tree-vectorizer.h

>> @@ -787,12 +787,46 @@ loop_vec_info_for_loop (class loop *loop)

>>    typedef class _bb_vec_info : public vec_info

>>    {

>>    public:

>> +  struct const_iterator

>> +  {

>> +    const_iterator (gimple_stmt_iterator _gsi): gsi (_gsi)

>> +    {

>> +    }

>> +

>> +    const_iterator &

>> +    operator++ (int)

>> +    {

>> +      gsi_next (&gsi); return *this;

>> +    }

> 

> Isn't this really operator++()?


It is, but post-increment ++ operators have one integer argument (so that
one can distinguish them from pre-increment operators:
https://en.cppreference.com/w/cpp/language/operator_incdec)

I was also quite surprised about that.

>  I.e. it returns a reference to the

> modified iterator instead of the value at the original iterator.

> 

> With that change, is there any reason we can't use a range-based for

> loop instead of:

> 

>    for (_bb_vec_info::const_iterator it = begin (); it != end (); it++)


/home/marxin/Programming/gcc/gcc/tree-vect-patterns.c: In function ‘void vect_determine_precisions(vec_info*)’:
/home/marxin/Programming/gcc/gcc/tree-vect-patterns.c:5124:31: error: no ‘operator++(int)’ declared for postfix ‘++’ [-fpermissive]
  5124 |     it != bb_vinfo->end (); it++)
       |                             ~~^~

There's slightly modified patch (one places is rewritten to classical iterator loop).

Martin

Martin

> 

> etc.?

> 

> Thanks,

> Richard

>
From 9e1a01e2e3a2075cf5ac2b2edb3fa969b198d342 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>

Date: Thu, 11 Jun 2020 13:25:40 +0200
Subject: [PATCH] vectorizer: add _bb_vec_info::const_iterator

gcc/ChangeLog:

	* tree-vect-patterns.c (vect_determine_precisions): Use the
	iterator.
	(vect_pattern_recog): Likewise.
	* tree-vect-slp.c (_bb_vec_info::_bb_vec_info): Likewise.
	(_bb_vec_info::~_bb_vec_info): Likewise.
	(vect_slp_check_for_constructors): Likewise.
	* tree-vect-stmts.c (vect_init_vector_1): Likewise.
	* tree-vectorizer.h: Add the new iterator and all related functions.
---
 gcc/tree-vect-patterns.c | 18 ++++++------------
 gcc/tree-vect-slp.c      | 24 +++++++++---------------
 gcc/tree-vect-stmts.c    |  2 +-
 gcc/tree-vectorizer.h    | 38 ++++++++++++++++++++++++++++++++++++--
 4 files changed, 52 insertions(+), 30 deletions(-)

diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index 636ad59c001..d194826f132 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -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::const_iterator it = bb_vinfo->begin ();
+	   it != bb_vinfo->end (); 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));
     }
 }
 
@@ -5492,10 +5486,10 @@ 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 (_bb_vec_info::const_iterator it = bb_vinfo->begin ();
+	   it != bb_vinfo->end (); it++)
 	{
-	  gimple *stmt = gsi_stmt (si);
+	  gimple *stmt = *it;
 	  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..5a2dc71f0cd 100644
--- a/gcc/tree-vect-slp.c
+++ b/gcc/tree-vect-slp.c
@@ -2551,15 +2551,12 @@ _bb_vec_info::_bb_vec_info (gimple_stmt_iterator region_begin_in,
 			    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)
+    m_region_begin (region_begin_in),
+    m_region_end (region_end_in)
 {
-  gimple_stmt_iterator gsi;
-
-  for (gsi = region_begin; gsi_stmt (gsi) != gsi_stmt (region_end);
-       gsi_next (&gsi))
+  for (_bb_vec_info::const_iterator it = begin (); it != end (); it++)
     {
-      gimple *stmt = gsi_stmt (gsi);
+      gimple *stmt = *it;
       gimple_set_uid (stmt, 0);
       if (is_gimple_debug (stmt))
 	continue;
@@ -2575,10 +2572,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 (_bb_vec_info::const_iterator it = begin (); it != end (); it++)
     /* Reset region marker.  */
-    gimple_set_uid (gsi_stmt (si), -1);
+    gimple_set_uid (*it, -1);
 
   bb->aux = NULL;
 }
@@ -3012,12 +3008,10 @@ 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 (_bb_vec_info::const_iterator it = bb_vinfo->begin ();
+       it != bb_vinfo->end (); it++)
     {
-      gassign *stmt = dyn_cast <gassign *> (gsi_stmt (gsi));
+      gassign *stmt = dyn_cast <gassign *> (*it);
       if (!stmt || gimple_assign_rhs_code (stmt) != CONSTRUCTOR)
 	continue;
 
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);
        }
     }
diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
index 6c830ad09f4..542d49402d2 100644
--- a/gcc/tree-vectorizer.h
+++ b/gcc/tree-vectorizer.h
@@ -787,12 +787,46 @@ loop_vec_info_for_loop (class loop *loop)
 typedef class _bb_vec_info : public vec_info
 {
 public:
+  struct const_iterator
+  {
+    const_iterator (gimple_stmt_iterator _gsi): gsi (_gsi)
+    {
+    }
+
+    const_iterator &
+    operator++ (int)
+    {
+      gsi_next (&gsi); return *this;
+    }
+
+    gimple *operator* () { 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;
+  };
+
+  const_iterator begin () const { return const_iterator (m_region_begin); }
+  const_iterator end () const { return const_iterator (m_region_end); }
+
   _bb_vec_info (gimple_stmt_iterator, gimple_stmt_iterator, vec_info_shared *);
   ~_bb_vec_info ();
 
   basic_block bb;
-  gimple_stmt_iterator region_begin;
-  gimple_stmt_iterator region_end;
+
+private:
+  gimple_stmt_iterator m_region_begin;
+  gimple_stmt_iterator m_region_end;
 } *bb_vec_info;
 
 #define BB_VINFO_BB(B)               (B)->bb
-- 
2.26.2
Richard Sandiford June 12, 2020, 11:43 a.m. | #3
Martin Liška <mliska@suse.cz> writes:
> On 6/12/20 12:46 PM, Richard Sandiford wrote:

>> Martin Liška <mliska@suse.cz> writes:

>>> Hello.

>>>

>>> I'm working one extension of SLP which will allow to vectorize multiple

>>> BBs. This is a first step where I abstract _bb_vec_info::region_begin and

>>> _bb_vec_info::region end by providing an iterator.

>> 

>> Nice.

>> 

>>> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h

>>> index 6c830ad09f4..542d49402d2 100644

>>> --- a/gcc/tree-vectorizer.h

>>> +++ b/gcc/tree-vectorizer.h

>>> @@ -787,12 +787,46 @@ loop_vec_info_for_loop (class loop *loop)

>>>    typedef class _bb_vec_info : public vec_info

>>>    {

>>>    public:

>>> +  struct const_iterator

>>> +  {

>>> +    const_iterator (gimple_stmt_iterator _gsi): gsi (_gsi)

>>> +    {

>>> +    }

>>> +

>>> +    const_iterator &

>>> +    operator++ (int)

>>> +    {

>>> +      gsi_next (&gsi); return *this;

>>> +    }

>> 

>> Isn't this really operator++()?

>

> It is, but post-increment ++ operators have one integer argument (so that

> one can distinguish them from pre-increment operators:

> https://en.cppreference.com/w/cpp/language/operator_incdec)


Sure, but what I mean by:

>> I.e. it returns a reference to the

>> modified iterator instead of the value at the original iterator.


is that the above implements the semantics of a preincrement, not a
postincrement, so it should be operator++() rather than operator++(int).

In other words, the function (rightly) implements “++it” rather than “it++”.

With that fixed, it seems like a range-based for loop should work.

Thanks,
Richard
Martin Liška June 12, 2020, 12:55 p.m. | #4
On 6/12/20 1:43 PM, Richard Sandiford wrote:
> Martin Liška <mliska@suse.cz> writes:

>> On 6/12/20 12:46 PM, Richard Sandiford wrote:

>>> Martin Liška <mliska@suse.cz> writes:

>>>> Hello.

>>>>

>>>> I'm working one extension of SLP which will allow to vectorize multiple

>>>> BBs. This is a first step where I abstract _bb_vec_info::region_begin and

>>>> _bb_vec_info::region end by providing an iterator.

>>>

>>> Nice.

>>>

>>>> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h

>>>> index 6c830ad09f4..542d49402d2 100644

>>>> --- a/gcc/tree-vectorizer.h

>>>> +++ b/gcc/tree-vectorizer.h

>>>> @@ -787,12 +787,46 @@ loop_vec_info_for_loop (class loop *loop)

>>>>     typedef class _bb_vec_info : public vec_info

>>>>     {

>>>>     public:

>>>> +  struct const_iterator

>>>> +  {

>>>> +    const_iterator (gimple_stmt_iterator _gsi): gsi (_gsi)

>>>> +    {

>>>> +    }

>>>> +

>>>> +    const_iterator &

>>>> +    operator++ (int)

>>>> +    {

>>>> +      gsi_next (&gsi); return *this;

>>>> +    }

>>>

>>> Isn't this really operator++()?

>>

>> It is, but post-increment ++ operators have one integer argument (so that

>> one can distinguish them from pre-increment operators:

>> https://en.cppreference.com/w/cpp/language/operator_incdec)

> 

> Sure, but what I mean by:

> 

>>> I.e. it returns a reference to the

>>> modified iterator instead of the value at the original iterator.

> 

> is that the above implements the semantics of a preincrement, not a

> postincrement, so it should be operator++() rather than operator++(int).


Ah, ok, I've got it.

> 

> In other words, the function (rightly) implements “++it” rather than “it++”.

> 

> With that fixed, it seems like a range-based for loop should work.


Yes, it works. The syntax is really neat:

for (gimple *stmt: *bb_vinfo)

I've tested vect.exp and i386.exp. May I install the patch after proper testing?

Thanks,
Martin

> 

> Thanks,

> Richard

>
From 977c96f27cbb6e6423c2546837ab50371df5f9d9 Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>

Date: Thu, 11 Jun 2020 13:25:40 +0200
Subject: [PATCH] vectorizer: add _bb_vec_info::const_iterator

gcc/ChangeLog:

	* tree-vect-patterns.c (vect_determine_precisions): Use the
	iterator.
	(vect_pattern_recog): Likewise.
	* tree-vect-slp.c (_bb_vec_info::_bb_vec_info): Likewise.
	(_bb_vec_info::~_bb_vec_info): Likewise.
	(vect_slp_check_for_constructors): Likewise.
	* tree-vect-stmts.c (vect_init_vector_1): Likewise.
	* tree-vectorizer.h: Add the new iterator and all related functions.
---
 gcc/tree-vect-patterns.c | 14 ++------------
 gcc/tree-vect-slp.c      | 28 ++++++++++------------------
 gcc/tree-vect-stmts.c    |  2 +-
 gcc/tree-vectorizer.h    | 38 ++++++++++++++++++++++++++++++++++++--
 4 files changed, 49 insertions(+), 33 deletions(-)

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));
     }
 }
 
@@ -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)
 	{
-	  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..8646d7f77ab 100644
--- a/gcc/tree-vect-slp.c
+++ b/gcc/tree-vect-slp.c
@@ -2551,15 +2551,11 @@ _bb_vec_info::_bb_vec_info (gimple_stmt_iterator region_begin_in,
 			    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)
+    m_region_begin (region_begin_in),
+    m_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)
     {
-      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)
     /* 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)
     {
-      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-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);
        }
     }
diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
index 6c830ad09f4..8f80a7d1bce 100644
--- a/gcc/tree-vectorizer.h
+++ b/gcc/tree-vectorizer.h
@@ -787,12 +787,46 @@ loop_vec_info_for_loop (class loop *loop)
 typedef class _bb_vec_info : public vec_info
 {
 public:
+  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;
+  };
+
+  const_iterator begin () const { return const_iterator (m_region_begin); }
+  const_iterator end () const { return const_iterator (m_region_end); }
+
   _bb_vec_info (gimple_stmt_iterator, gimple_stmt_iterator, vec_info_shared *);
   ~_bb_vec_info ();
 
   basic_block bb;
-  gimple_stmt_iterator region_begin;
-  gimple_stmt_iterator region_end;
+
+private:
+  gimple_stmt_iterator m_region_begin;
+  gimple_stmt_iterator m_region_end;
 } *bb_vec_info;
 
 #define BB_VINFO_BB(B)               (B)->bb
-- 
2.26.2
Marek Polacek via Gcc-patches June 12, 2020, 1:02 p.m. | #5
On June 12, 2020 2:55:49 PM GMT+02:00, "Martin Liška" <mliska@suse.cz> wrote:
>On 6/12/20 1:43 PM, Richard Sandiford wrote:

>> Martin Liška <mliska@suse.cz> writes:

>>> On 6/12/20 12:46 PM, Richard Sandiford wrote:

>>>> Martin Liška <mliska@suse.cz> writes:

>>>>> Hello.

>>>>>

>>>>> I'm working one extension of SLP which will allow to vectorize

>multiple

>>>>> BBs. This is a first step where I abstract

>_bb_vec_info::region_begin and

>>>>> _bb_vec_info::region end by providing an iterator.

>>>>

>>>> Nice.

>>>>

>>>>> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h

>>>>> index 6c830ad09f4..542d49402d2 100644

>>>>> --- a/gcc/tree-vectorizer.h

>>>>> +++ b/gcc/tree-vectorizer.h

>>>>> @@ -787,12 +787,46 @@ loop_vec_info_for_loop (class loop *loop)

>>>>>     typedef class _bb_vec_info : public vec_info

>>>>>     {

>>>>>     public:

>>>>> +  struct const_iterator

>>>>> +  {

>>>>> +    const_iterator (gimple_stmt_iterator _gsi): gsi (_gsi)

>>>>> +    {

>>>>> +    }

>>>>> +

>>>>> +    const_iterator &

>>>>> +    operator++ (int)

>>>>> +    {

>>>>> +      gsi_next (&gsi); return *this;

>>>>> +    }

>>>>

>>>> Isn't this really operator++()?

>>>

>>> It is, but post-increment ++ operators have one integer argument (so

>that

>>> one can distinguish them from pre-increment operators:

>>> https://en.cppreference.com/w/cpp/language/operator_incdec)

>> 

>> Sure, but what I mean by:

>> 

>>>> I.e. it returns a reference to the

>>>> modified iterator instead of the value at the original iterator.

>> 

>> is that the above implements the semantics of a preincrement, not a

>> postincrement, so it should be operator++() rather than

>operator++(int).

>

>Ah, ok, I've got it.

>

>> 

>> In other words, the function (rightly) implements “++it” rather than

>“it++”.

>> 

>> With that fixed, it seems like a range-based for loop should work.

>

>Yes, it works. The syntax is really neat:

>

>for (gimple *stmt: *bb_vinfo)


I'd rather have what we iterate over more explicit like

For(gimple. *stmt : bb_vinfo->region_stmts()) 

Richard. 

>

>I've tested vect.exp and i386.exp. May I install the patch after proper

>testing?

>

>Thanks,

>Martin

>

>> 

>> Thanks,

>> Richard

>>
Richard Sandiford June 12, 2020, 1:22 p.m. | #6
Martin Liška <mliska@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.

> @@ -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)

>  	{

> -	  gimple *stmt = gsi_stmt (si);

>  	  stmt_vec_info stmt_info = bb_vinfo->lookup_stmt (stmt);

>  	  if (!stmt_info || !STMT_VINFO_VECTORIZABLE (stmt_info))

>  	    continue;


Very minor, but I think it's more usual to have a space both sides of
the ":".  (That's also the formatting used by libstdc++-v3, and ties in
nicely with the formatting of infix operators.)  Same for the others.

> 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?  We could then
reuse this work for BB iterators, etc.

> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h

> index 6c830ad09f4..8f80a7d1bce 100644

> --- a/gcc/tree-vectorizer.h

> +++ b/gcc/tree-vectorizer.h

> @@ -787,12 +787,46 @@ loop_vec_info_for_loop (class loop *loop)

>  typedef class _bb_vec_info : public vec_info

>  {

>  public:

> +  struct const_iterator

> +  {

> +    const_iterator (gimple_stmt_iterator _gsi): gsi (_gsi)

> +    {

> +    }


Again very minor, but I think the conventions say that this should
be defined on one line, like the later operator* is.  Space before
":" here too.

Thanks,
Richard
Martin Liška June 12, 2020, 3:28 p.m. | #7
On 6/12/20 3:22 PM, Richard Sandiford wrote:
> Martin Liška <mliska@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.

> 

>> @@ -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)

>>   	{

>> -	  gimple *stmt = gsi_stmt (si);

>>   	  stmt_vec_info stmt_info = bb_vinfo->lookup_stmt (stmt);

>>   	  if (!stmt_info || !STMT_VINFO_VECTORIZABLE (stmt_info))

>>   	    continue;

> 

> Very minor, but I think it's more usual to have a space both sides of

> the ":".  (That's also the formatting used by libstdc++-v3, and ties in

> nicely with the formatting of infix operators.)  Same for the others.


Changed.

> 

>> 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.

>  We could then

> reuse this work for BB iterators, etc.

> 

>> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h

>> index 6c830ad09f4..8f80a7d1bce 100644

>> --- a/gcc/tree-vectorizer.h

>> +++ b/gcc/tree-vectorizer.h

>> @@ -787,12 +787,46 @@ loop_vec_info_for_loop (class loop *loop)

>>   typedef class _bb_vec_info : public vec_info

>>   {

>>   public:

>> +  struct const_iterator

>> +  {

>> +    const_iterator (gimple_stmt_iterator _gsi): gsi (_gsi)

>> +    {

>> +    }

> 

> Again very minor, but I think the conventions say that this should

> be defined on one line, like the later operator* is.  Space before

> ":" here too.


Likewise changed here.

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Martin

> 

> Thanks,

> Richard

>
From 8859cb13a20d91222103577f21af2cd029344a5d Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>

Date: Thu, 11 Jun 2020 13:25:40 +0200
Subject: [PATCH] vectorizer: add _bb_vec_info::const_iterator

gcc/ChangeLog:

	* tree-vectorizer.h: Add the new const_iterator and
	reverse_iterator and all related functions.
	* tree-vect-patterns.c (vect_determine_precisions): Use the
	iterator.
	(vect_pattern_recog): Likewise.
	* tree-vect-slp.c (_bb_vec_info::_bb_vec_info): Likewise.
	(_bb_vec_info::~_bb_vec_info): Likewise.
	(vect_slp_check_for_constructors): Likewise.
	* tree-vect-stmts.c (vect_init_vector_1): Likewise.
---
 gcc/tree-vect-patterns.c | 16 +++------
 gcc/tree-vect-slp.c      | 28 ++++++---------
 gcc/tree-vect-stmts.c    |  2 +-
 gcc/tree-vectorizer.h    | 74 ++++++++++++++++++++++++++++++++++++++--
 4 files changed, 87 insertions(+), 33 deletions(-)

diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index 636ad59c001..e24dc848d7b 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -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->rbegin ();
+	   it != bb_vinfo->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));
     }
 }
 
@@ -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)
 	{
-	  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..cc4ff588a13 100644
--- a/gcc/tree-vect-slp.c
+++ b/gcc/tree-vect-slp.c
@@ -2551,15 +2551,11 @@ _bb_vec_info::_bb_vec_info (gimple_stmt_iterator region_begin_in,
 			    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)
+    m_region_begin (region_begin_in),
+    m_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)
     {
-      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)
     /* 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)
     {
-      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-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);
        }
     }
diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
index 6c830ad09f4..921bae680b2 100644
--- a/gcc/tree-vectorizer.h
+++ b/gcc/tree-vectorizer.h
@@ -787,12 +787,82 @@ loop_vec_info_for_loop (class loop *loop)
 typedef class _bb_vec_info : public vec_info
 {
 public:
+  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;
+  };
+
+  const_iterator begin () const { return const_iterator (m_region_begin); }
+  const_iterator end () const { return const_iterator (m_region_end); }
+
+  struct 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;
+  };
+
+  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); }
+
   _bb_vec_info (gimple_stmt_iterator, gimple_stmt_iterator, vec_info_shared *);
   ~_bb_vec_info ();
 
   basic_block bb;
-  gimple_stmt_iterator region_begin;
-  gimple_stmt_iterator region_end;
+
+private:
+  gimple_stmt_iterator m_region_begin;
+  gimple_stmt_iterator m_region_end;
 } *bb_vec_info;
 
 #define BB_VINFO_BB(B)               (B)->bb
-- 
2.26.2
Martin Liška June 12, 2020, 3:29 p.m. | #8
On 6/12/20 3:02 PM, Richard Biener wrote:
> For(gimple. *stmt : bb_vinfo->region_stmts())


Hm, that will require region_stmts() returning one another object
that will contain the begin/end methods.
Is it what we want or do I miss something?

Martin
Marek Polacek via Gcc-patches June 12, 2020, 3:47 p.m. | #9
On June 12, 2020 5:29:40 PM GMT+02:00, "Martin Liška" <mliska@suse.cz> wrote:
>On 6/12/20 3:02 PM, Richard Biener wrote:

>> For(gimple. *stmt : bb_vinfo->region_stmts())

>

>Hm, that will require region_stmts() returning one another object

>that will contain the begin/end methods.

>Is it what we want or do I miss something?


Yes, you usually need some kind of iterator container. There's also the vector of SLP graph entries we iterate over repeatedly for example. For BBs we want to iterate over PHIs, stmts and incoming and outgoing edges. So I expect these kind of wrappers to be the common case. 

Richard. 

>Martin
Richard Sandiford June 12, 2020, 3:49 p.m. | #10
Martin Liška <mliska@suse.cz> writes:
> On 6/12/20 3:22 PM, Richard Sandiford wrote:

>> Martin Liška <mliska@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
Marek Polacek via Gcc-patches June 12, 2020, 7:50 p.m. | #11
On 6/12/20 11:49 AM, Richard Sandiford wrote:
> Martin Liška <mliska@suse.cz> writes:

>> On 6/12/20 3:22 PM, Richard Sandiford wrote:

>>> Martin Liška <mliska@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
Martin Liška June 15, 2020, 1:06 p.m. | #12
On 6/12/20 5:49 PM, Richard Sandiford wrote:
> Martin Liška <mliska@suse.cz> writes:

>> On 6/12/20 3:22 PM, Richard Sandiford wrote:

>>> Martin Liška <mliska@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.


I decided for this 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;

>> +    }

> 

> I think the else case should return “++it” instead, since AIUI

> m_region_end is an exclusive rather than inclusive endpoint.


Ah, you are right.

> 

> 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.

About rbiener's comment, I wrapper the iterators with bb_vinfo::region_stmts..

Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

Ready to be installed?
Thanks,
Martin

> 

> Thanks,

> Richard

>
From d96153a354c60e1c2c2fcd1da5714556fcd54a1a Mon Sep 17 00:00:00 2001
From: Martin Liska <mliska@suse.cz>

Date: Thu, 11 Jun 2020 13:25:40 +0200
Subject: [PATCH] vectorizer: add _bb_vec_info::region_stmts and iterators

gcc/ChangeLog:

	* tree-vectorizer.h: Add the new const_iterator and
	reverse_iterator and all related functions.
	Wrap m_region_begin and m_region_end in region_stmts.
	* tree-vect-patterns.c (vect_determine_precisions): Use the
	iterator.
	(vect_pattern_recog): Likewise.
	* tree-vect-slp.c (_bb_vec_info::_bb_vec_info): Likewise.
	(_bb_vec_info::~_bb_vec_info): Likewise.
	(vect_slp_check_for_constructors): Likewise.
	* tree-vect-stmts.c (vect_init_vector_1): Likewise.
---
 gcc/tree-vect-patterns.c | 16 ++------
 gcc/tree-vect-slp.c      | 33 ++++++---------
 gcc/tree-vect-stmts.c    |  3 +-
 gcc/tree-vectorizer.h    | 86 ++++++++++++++++++++++++++++++++++++++--
 4 files changed, 101 insertions(+), 37 deletions(-)

diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index 636ad59c001..d2d4ce1dc8e 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -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));
     }
 }
 
@@ -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-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);
        }
     }
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
+    {
+      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;
+    };
+
+  struct 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); }
+    };
+
+  basic_block bb;
+  stmt_iterator region_stmts;
+
   _bb_vec_info (gimple_stmt_iterator, gimple_stmt_iterator, vec_info_shared *);
   ~_bb_vec_info ();
 
-  basic_block bb;
-  gimple_stmt_iterator region_begin;
-  gimple_stmt_iterator region_end;
 } *bb_vec_info;
 
 #define BB_VINFO_BB(B)               (B)->bb
-- 
2.27.0
Richard Sandiford June 16, 2020, 8:50 a.m. | #13
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); }

> +    };


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
Martin Liška June 16, 2020, 1:14 p.m. | #14
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

>
Richard Sandiford June 16, 2020, 2:14 p.m. | #15
Martin Liška <mliska@suse.cz> writes:
> 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)?


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”.

Thanks,
Richard
Martin Liška June 17, 2020, 5:05 p.m. | #16
On 6/16/20 4:14 PM, Richard Sandiford wrote:
> Martin Liška <mliska@suse.cz> writes:

>> 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)?

> 

> 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 <mliska@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
Marek Polacek via Gcc-patches June 17, 2020, 7:44 p.m. | #17
On 6/17/20 11:05 AM, Martin Liška wrote:
> On 6/16/20 4:14 PM, Richard Sandiford wrote:

>> Martin Liška <mliska@suse.cz> writes:

>>> 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)?

>>

>> 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.


Since the iterator_pair template is being added as a foundational
type I'd suggest fleshing out the design a bit more.  There is
little difference between iterator_pair and std::pair, but since
the class is being used as an iterator range, I'd consider naming
it that way: iterator_range, and modeling it on the Boost template
of the same name (unless there's something better in recent
revisions of C++).  That would suggest adding the full complement
of (non-member) equality and relational operators and perhaps a few
convenience members).

Martin


> 

> Patch can bootstrap on x86_64-linux-gnu and survives regression tests.

> 

> Ready to be installed?

> Thanks,

> Martin

> 

>>

>> Thanks,

>> Richard

>>

>
Richard Sandiford June 17, 2020, 8:05 p.m. | #18
Martin Liška <mliska@suse.cz> writes:
> On 6/16/20 4:14 PM, Richard Sandiford wrote:

>> Martin Liška <mliska@suse.cz> writes:

>>> 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

>

> [...]

> +    gimple *operator* () const { return gsi_stmt (gsi); }

> +

> +    bool

> +    operator== (const const_iterator& other) const


I think the usual formatting in GCC (though not elsewhere) is to put the
space before rather than after the “&”, like for pointers.  Same for the
rest of the patch.

> +    {

> +      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


The standard name for this abstraction is const_reverse_iterator rather
than 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));


The const_iterator(…)s aren't necessary here, just:

    return iterator_pair<const_iterator> (region_begin, region_end);

is IMO more readable.

> +  }

> +

> +  /* 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;


Similarly here we don't need the reverse_const_iterator(…)s
(= const_reverse_iterator(…)s after the change above).  It can just be:

    const_reverse_iterator begin = region_end;
    if (*begin == NULL)
      begin = 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);


Not sure the “end” temporary really buys much, could just be:

    return iterator_pair<const_reverse_iterator> (begin, region_begin);

> +  }

>  

>    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;


I realise it just ended up this way through the various iterations,
but I think the constructor and destructor should go between the
new types and the new member functions, rather than at the end.
The coding conventions say;

- first define all public types,
- then define all non-public types,
- then declare all public constructors,
- then declare the public destructor,
- then declare all public member functions,
- then declare all public member variables,

There should be no blank line before the final “}”.

OK with those changes, and thanks for your patience :-)

Richard
Richard Sandiford June 17, 2020, 8:20 p.m. | #19
(Sorry, sent my previous message before seeing this one.)

Martin Sebor <msebor@gmail.com> writes:
> Since the iterator_pair template is being added as a foundational

> type I'd suggest fleshing out the design a bit more.  There is

> little difference between iterator_pair and std::pair, but since

> the class is being used as an iterator range, I'd consider naming

> it that way: iterator_range, and modeling it on the Boost template

> of the same name (unless there's something better in recent

> revisions of C++).  That would suggest adding the full complement

> of (non-member) equality and relational operators and perhaps a few

> convenience members).


Agree s/iterator_pair/iterator_range/ is better FWIW.  MartinL,
mind making that change too before committing?

However:

(a) We're not designing a library or stable API here, so IMO we should
    only add extra member functions when we actually need them.  I think
    throwing in a bunch of other stuff now in the same of providing as
    complete an interface as possible is the kind of thing that's giving
    C++-ification a bad name in GCC. :-)

(b) IMO it would be best not to look at Boost and try to reproduce
    essentially the same types and interfaces in GCC.  The copyright
    situation for doing that is not at all clear, and AFAIK none of us
    are IP lawyers.  (c.f. Google v. Oracle.)

Thanks,
Richard
Marek Polacek via Gcc-patches June 17, 2020, 11:55 p.m. | #20
On 16/06/20 15:14 +0100, Richard Sandiford wrote:
>Martin Li?ka <mliska@suse.cz> writes:

>> On 6/16/20 10:50 AM, Richard Sandiford wrote:

>> 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;

>  };


You could also add an "object generator" for that type:

template<typename T>
iterator_range<T>
make_iterator_range(T begin, T end)
{
   return iterator_range<T>(begin, end);
}

This deduces the type T from the function arguments, so will allow you
to write:

   return make_iterator_range(x, y);

instead of:

   return iterator_range<const_iterator>(x, y);

i.e. you don't need to say <const_iterator> because the compiler can
deduce it from the arguments.

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.
Marek Polacek via Gcc-patches June 17, 2020, 11:59 p.m. | #21
On 18/06/20 00:55 +0100, Jonathan Wakely wrote:
>On 16/06/20 15:14 +0100, Richard Sandiford wrote:

>>Martin Li?ka <mliska@suse.cz> writes:

>>>On 6/16/20 10:50 AM, Richard Sandiford wrote:

>>>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;

>> };

>

>You could also add an "object generator" for that type:

>

>template<typename T>

>iterator_range<T>

>make_iterator_range(T begin, T end)

>{

>  return iterator_range<T>(begin, end);

>}

>

>This deduces the type T from the function arguments, so will allow you

>to write:

>

>  return make_iterator_range(x, y);

>

>instead of:

>

>  return iterator_range<const_iterator>(x, y);

>

>i.e. you don't need to say <const_iterator> because the compiler can

>deduce it from the arguments.

>

>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.


One more thing I noticed: is there a reason your operator++ returns a
const reference? Conventionally it should be a non-const reference.
i.e.

   const_iterator& operator++();

And if you want to make it bidirectional:

   const_iterator& operator--();
Marek Polacek via Gcc-patches June 18, 2020, 12:10 a.m. | #22
On 18/06/20 00:59 +0100, Jonathan Wakely wrote:
>On 18/06/20 00:55 +0100, Jonathan Wakely wrote:

>>On 16/06/20 15:14 +0100, Richard Sandiford wrote:

>>>Martin Li?ka <mliska@suse.cz> writes:

>>>>On 6/16/20 10:50 AM, Richard Sandiford wrote:

>>>>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;

>>>};

>>

>>You could also add an "object generator" for that type:

>>

>>template<typename T>

>>iterator_range<T>

>>make_iterator_range(T begin, T end)

>>{

>> return iterator_range<T>(begin, end);

>>}

>>

>>This deduces the type T from the function arguments, so will allow you

>>to write:

>>

>> return make_iterator_range(x, y);

>>

>>instead of:

>>

>> return iterator_range<const_iterator>(x, y);

>>

>>i.e. you don't need to say <const_iterator> because the compiler can

>>deduce it from the arguments.

>>

>>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.

>

>One more thing I noticed: is there a reason your operator++ returns a

>const reference? Conventionally it should be a non-const reference.

>i.e.

>

>  const_iterator& operator++();

>

>And if you want to make it bidirectional:

>

>  const_iterator& operator--();

>


And another thing...

Why const_iterator and not just iterator? It returns a gimple* not a
const gimple* which means that it can be used to change the contents
of the range. And even if it can't, if you don't plan to add a
(non-const) iterator later that is for mutating things, then it's just
noise to make everybody type const_iterator.

It's fine for an 'iterator' type to disallow modifications, e.g. see
std::set<T>::iterator which is the same type as
std::set<T>::const_iterator, because modifying std::set elements is
not permitted (it would invalidate the ordering).

And why reverse_const_iterator not const_reverse_iterator like every
container in the standard library? (Although that question doesn't
need to be answered if you rename them as iterator and
reverse_iterator).
Richard Sandiford June 18, 2020, 6:34 a.m. | #23
Thanks for the comments, just had a question about one of them…

Jonathan Wakely <jwakely@redhat.com> writes:
> On 16/06/20 15:14 +0100, Richard Sandiford wrote:

>>Martin Li?ka <mliska@suse.cz> writes:

>>> On 6/16/20 10:50 AM, Richard Sandiford wrote:

>>> 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;

>>  };

>

> You could also add an "object generator" for that type:

>

> template<typename T>

> iterator_range<T>

> make_iterator_range(T begin, T end)

> {

>    return iterator_range<T>(begin, end);

> }

>

> This deduces the type T from the function arguments, so will allow you

> to write:

>

>    return make_iterator_range(x, y);

>

> instead of:

>

>    return iterator_range<const_iterator>(x, y);

>

> i.e. you don't need to say <const_iterator> because the compiler can

> deduce it from the arguments.


In Martin's case the x and y arguments don't naturally have type
const_iterator, so I guess this would become:

  return make_iterator_range (const_iterator (x), const_iterator (y));

Is that more idiomatic than:

  return iterator_range<const_iterator> (x, y);

?

Richard
Marek Polacek via Gcc-patches June 18, 2020, 9:03 a.m. | #24
On 18/06/20 07:34 +0100, Richard Sandiford wrote:
>Thanks for the comments, just had a question about one of them…

>

>Jonathan Wakely <jwakely@redhat.com> writes:

>> On 16/06/20 15:14 +0100, Richard Sandiford wrote:

>>>Martin Li?ka <mliska@suse.cz> writes:

>>>> On 6/16/20 10:50 AM, Richard Sandiford wrote:

>>>> 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;

>>>  };

>>

>> You could also add an "object generator" for that type:

>>

>> template<typename T>

>> iterator_range<T>

>> make_iterator_range(T begin, T end)

>> {

>>    return iterator_range<T>(begin, end);

>> }

>>

>> This deduces the type T from the function arguments, so will allow you

>> to write:

>>

>>    return make_iterator_range(x, y);

>>

>> instead of:

>>

>>    return iterator_range<const_iterator>(x, y);

>>

>> i.e. you don't need to say <const_iterator> because the compiler can

>> deduce it from the arguments.

>

>In Martin's case the x and y arguments don't naturally have type

>const_iterator, so I guess this would become:

>

>  return make_iterator_range (const_iterator (x), const_iterator (y));


Ah yes, of course..

>Is that more idiomatic than:

>

>  return iterator_range<const_iterator> (x, y);

>

>?


Idiomatic or not, the make_iterator_range function is more verbose, so
defeats the purpose of using it.
Marek Polacek via Gcc-patches June 18, 2020, 9:43 a.m. | #25
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.

N.B. the fact that the bug in reverse iteration wasn't noticed
suggests missing tests :-)
Martin Liška June 18, 2020, 2:25 p.m. | #26
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 <mliska@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
Marek Polacek via Gcc-patches June 18, 2020, 2:55 p.m. | #27
On 18/06/20 16:25 +0200, Martin Liška wrote:
>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).


Yes, the std::reverse_iterator idea is a dead end, and I agree that
make_iterator_range doesn't seem like an improvement.

Patch

diff --git a/gcc/tree-vect-patterns.c b/gcc/tree-vect-patterns.c
index 636ad59c001..4c5a791522c 100644
--- a/gcc/tree-vect-patterns.c
+++ b/gcc/tree-vect-patterns.c
@@ -5120,7 +5120,7 @@  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_iterator si = bb_vinfo->end ().gsi;
        gimple *stmt;
        do
  	{
@@ -5133,7 +5133,7 @@  vect_determine_precisions (vec_info *vinfo)
  	  if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
  	    vect_determine_stmt_precisions (vinfo, stmt_info);
  	}
-      while (stmt != gsi_stmt (bb_vinfo->region_begin));
+      while (stmt != *bb_vinfo->begin ());
      }
  }
  
@@ -5492,10 +5492,10 @@  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 (_bb_vec_info::const_iterator it = bb_vinfo->begin ();
+	   it != bb_vinfo->end (); it++)
  	{
-	  gimple *stmt = gsi_stmt (si);
+	  gimple *stmt = *it;
  	  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..5a2dc71f0cd 100644
--- a/gcc/tree-vect-slp.c
+++ b/gcc/tree-vect-slp.c
@@ -2551,15 +2551,12 @@  _bb_vec_info::_bb_vec_info (gimple_stmt_iterator region_begin_in,
  			    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)
+    m_region_begin (region_begin_in),
+    m_region_end (region_end_in)
  {
-  gimple_stmt_iterator gsi;
-
-  for (gsi = region_begin; gsi_stmt (gsi) != gsi_stmt (region_end);
-       gsi_next (&gsi))
+  for (_bb_vec_info::const_iterator it = begin (); it != end (); it++)
      {
-      gimple *stmt = gsi_stmt (gsi);
+      gimple *stmt = *it;
        gimple_set_uid (stmt, 0);
        if (is_gimple_debug (stmt))
  	continue;
@@ -2575,10 +2572,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 (_bb_vec_info::const_iterator it = begin (); it != end (); it++)
      /* Reset region marker.  */
-    gimple_set_uid (gsi_stmt (si), -1);
+    gimple_set_uid (*it, -1);
  
    bb->aux = NULL;
  }
@@ -3012,12 +3008,10 @@  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 (_bb_vec_info::const_iterator it = bb_vinfo->begin ();
+       it != bb_vinfo->end (); it++)
      {
-      gassign *stmt = dyn_cast <gassign *> (gsi_stmt (gsi));
+      gassign *stmt = dyn_cast <gassign *> (*it);
        if (!stmt || gimple_assign_rhs_code (stmt) != CONSTRUCTOR)
  	continue;
  
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);
         }
      }
diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
index 6c830ad09f4..542d49402d2 100644
--- a/gcc/tree-vectorizer.h
+++ b/gcc/tree-vectorizer.h
@@ -787,12 +787,46 @@  loop_vec_info_for_loop (class loop *loop)
  typedef class _bb_vec_info : public vec_info
  {
  public:
+  struct const_iterator
+  {
+    const_iterator (gimple_stmt_iterator _gsi): gsi (_gsi)
+    {
+    }
+
+    const_iterator &
+    operator++ (int)
+    {
+      gsi_next (&gsi); return *this;
+    }
+
+    gimple *operator* () { 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;
+  };
+
+  const_iterator begin () const { return const_iterator (m_region_begin); }
+  const_iterator end () const { return const_iterator (m_region_end); }
+
    _bb_vec_info (gimple_stmt_iterator, gimple_stmt_iterator, vec_info_shared *);
    ~_bb_vec_info ();
  
    basic_block bb;
-  gimple_stmt_iterator region_begin;
-  gimple_stmt_iterator region_end;
+
+private:
+  gimple_stmt_iterator m_region_begin;
+  gimple_stmt_iterator m_region_end;
  } *bb_vec_info;
  
  #define BB_VINFO_BB(B)               (B)->bb