On 6/16/20 10:50 AM, Richard Sandiford wrote:
Martin Liška <mli...@suse.cz> writes:
Also, one minor formatting nit, sorry: the other functions instead
indent the “{” block by the same amount as the function prototype,
which seems more consistent with the usual out-of-class formatting.

Hope I fixed that.

Sorry, I meant the other functions were IMO formatted correctly, with the
“{” directly under the function name.  It looks like the new patch instead
indents all “{” by two spaces relative to the function name or “struct”
keyword.  I.e. IMO:

   struct const_iterator
   {
   };

seems better than:
struct const_iterator
     {
     };

and:

   const const_iterator &
   operator++ ()
   {
   }

seems better than:

   const const_iterator &
   operator++ ()
     {
     }

This makes the formatting consistent with definitions in column 0.

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

Sorry for dragging this on longer, but…

@@ -5120,20 +5120,14 @@ vect_determine_precisions (vec_info *vinfo)
    else
      {
        bb_vec_info bb_vinfo = as_a <bb_vec_info> (vinfo);
-      gimple_stmt_iterator si = bb_vinfo->region_end;
-      gimple *stmt;
-      do
+      for ( _bb_vec_info::reverse_iterator it = bb_vinfo->region_stmts.rbegin 
();
+          it != bb_vinfo->region_stmts.rend (); ++it)
        {
-         if (!gsi_stmt (si))
-           si = gsi_last_bb (bb_vinfo->bb);
-         else
-           gsi_prev (&si);
-         stmt = gsi_stmt (si);
+         gimple *stmt = *it;
          stmt_vec_info stmt_info = vinfo->lookup_stmt (stmt);
          if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
            vect_determine_stmt_precisions (vinfo, stmt_info);
        }
-      while (stmt != gsi_stmt (bb_vinfo->region_begin));
      }
  }

I think this should be a similar style, i.e.

      for (gimple *stmt : bb_vinfo->reverse_region_stmts ())

rather than using iterators directly.

@@ -5492,10 +5486,8 @@ vect_pattern_recog (vec_info *vinfo)
    else
      {
        bb_vec_info bb_vinfo = as_a <bb_vec_info> (vinfo);
-      for (si = bb_vinfo->region_begin;
-          gsi_stmt (si) != gsi_stmt (bb_vinfo->region_end); gsi_next (&si))
+      for (gimple *stmt : bb_vinfo->region_stmts)
        {
-         gimple *stmt = gsi_stmt (si);
          stmt_vec_info stmt_info = bb_vinfo->lookup_stmt (stmt);
          if (!stmt_info || !STMT_VINFO_VECTORIZABLE (stmt_info))
            continue;
diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c
index 303410c2fc4..f4809c2207d 100644
--- a/gcc/tree-vect-slp.c
+++ b/gcc/tree-vect-slp.c
@@ -2546,20 +2546,15 @@ vect_detect_hybrid_slp (loop_vec_info loop_vinfo)
  /* Initialize a bb_vec_info struct for the statements between
     REGION_BEGIN_IN (inclusive) and REGION_END_IN (exclusive).  */
-_bb_vec_info::_bb_vec_info (gimple_stmt_iterator region_begin_in,
-                           gimple_stmt_iterator region_end_in,
+_bb_vec_info::_bb_vec_info (gimple_stmt_iterator region_begin,
+                           gimple_stmt_iterator region_end,
                            vec_info_shared *shared)
    : vec_info (vec_info::bb, init_cost (NULL), shared),
-    bb (gsi_bb (region_begin_in)),
-    region_begin (region_begin_in),
-    region_end (region_end_in)
+    bb (gsi_bb (region_begin)),
+    region_stmts (region_begin, region_end)
  {
-  gimple_stmt_iterator gsi;
-
-  for (gsi = region_begin; gsi_stmt (gsi) != gsi_stmt (region_end);
-       gsi_next (&gsi))
+  for (gimple *stmt : this->region_stmts)
      {
-      gimple *stmt = gsi_stmt (gsi);
        gimple_set_uid (stmt, 0);
        if (is_gimple_debug (stmt))
        continue;
@@ -2575,10 +2570,9 @@ _bb_vec_info::_bb_vec_info (gimple_stmt_iterator 
region_begin_in,
_bb_vec_info::~_bb_vec_info ()
  {
-  for (gimple_stmt_iterator si = region_begin;
-       gsi_stmt (si) != gsi_stmt (region_end); gsi_next (&si))
+  for (gimple *stmt : this->region_stmts)
      /* Reset region marker.  */
-    gimple_set_uid (gsi_stmt (si), -1);
+    gimple_set_uid (stmt, -1);
bb->aux = NULL;
  }
@@ -3012,16 +3006,13 @@ vect_bb_vectorization_profitable_p (bb_vec_info 
bb_vinfo)
  static void
  vect_slp_check_for_constructors (bb_vec_info bb_vinfo)
  {
-  gimple_stmt_iterator gsi;
-
-  for (gsi = bb_vinfo->region_begin;
-       gsi_stmt (gsi) != gsi_stmt (bb_vinfo->region_end); gsi_next (&gsi))
+  for (gimple *stmt : bb_vinfo->region_stmts)
      {
-      gassign *stmt = dyn_cast <gassign *> (gsi_stmt (gsi));
-      if (!stmt || gimple_assign_rhs_code (stmt) != CONSTRUCTOR)
+      gassign *assign = dyn_cast <gassign *> (stmt);
+      if (!assign || gimple_assign_rhs_code (assign) != CONSTRUCTOR)
        continue;
- tree rhs = gimple_assign_rhs1 (stmt);
+      tree rhs = gimple_assign_rhs1 (assign);
        if (!VECTOR_TYPE_P (TREE_TYPE (rhs))
          || maybe_ne (TYPE_VECTOR_SUBPARTS (TREE_TYPE (rhs)),
                       CONSTRUCTOR_NELTS (rhs))
@@ -3029,7 +3020,7 @@ vect_slp_check_for_constructors (bb_vec_info bb_vinfo)
          || uniform_vector_p (rhs))
        continue;
- stmt_vec_info stmt_info = bb_vinfo->lookup_stmt (stmt);
+      stmt_vec_info stmt_info = bb_vinfo->lookup_stmt (assign);
        BB_VINFO_GROUPED_STORES (bb_vinfo).safe_push (stmt_info);
      }
  }
diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
index 6c830ad09f4..c94817e6af6 100644
--- a/gcc/tree-vectorizer.h
+++ b/gcc/tree-vectorizer.h
@@ -787,12 +787,92 @@ loop_vec_info_for_loop (class loop *loop)
  typedef class _bb_vec_info : public vec_info
  {
  public:
+  struct const_iterator
+    {

“{” should be directly under “struct”.

+      const_iterator (gimple_stmt_iterator _gsi) : gsi (_gsi) {}
+
+      const const_iterator &
+      operator++ ()
+       {

“{” should be directly under “operator++”.  Same for the other structs
and functions.

+         gsi_next (&gsi); return *this;
+       }
+
+      gimple *operator* () const { return gsi_stmt (gsi); }
+
+      bool
+      operator== (const const_iterator& other) const
+       {
+         return gsi_stmt (gsi) == gsi_stmt (other.gsi);
+       }
+
+      bool
+      operator!= (const const_iterator& other) const
+       {
+         return !(*this == other);
+       }
+
+      gimple_stmt_iterator gsi;
+    };
+
+  struct reverse_iterator

This should be a const_reverse_iterator.

+    {
+      reverse_iterator (gimple_stmt_iterator _gsi) : gsi (_gsi) {}
+
+      const reverse_iterator &
+      operator++ ()
+       {
+         gsi_prev (&gsi); return *this;
+       }
+
+      gimple *operator* () const { return gsi_stmt (gsi); }
+
+      bool
+      operator== (const reverse_iterator& other) const
+       {
+         return gsi_stmt (gsi) == gsi_stmt (other.gsi);
+       }
+
+      bool
+      operator!= (const reverse_iterator& other) const
+       {
+         return !(*this == other);
+       }
+
+      gimple_stmt_iterator gsi;
+    };
+
+  struct stmt_iterator
+    {
+      stmt_iterator (gimple_stmt_iterator region_begin,
+                    gimple_stmt_iterator region_end)
+      : m_region_begin (region_begin), m_region_end (region_end) {}
+
+      gimple_stmt_iterator region_begin () { return m_region_begin; }
+
+      const_iterator begin () const { return const_iterator (m_region_begin); }
+      const_iterator end () const { return const_iterator (m_region_end); }
+
+      gimple_stmt_iterator m_region_begin;
+      gimple_stmt_iterator m_region_end;
+
+      reverse_iterator rbegin () const
+       {
+         reverse_iterator it = reverse_iterator (m_region_end);
+         if (*it == NULL)
+           return reverse_iterator (gsi_last_bb (m_region_end.bb));
+         else
+           return ++it;
+       }
+
+      reverse_iterator rend () const { return reverse_iterator 
(m_region_begin); }
+    };

Thank you for the review. I'm skipping for now the renaming and formatting 
changes which
I'll do later.


I think for this we should have a template class for begin/end iterator
pairs, probably in coretypes.h.  We could call it “iterator_pair” or
something.  Then “region_stmts” would return (see below) a:

    iterator_pair<const_iterator>

and “reverse_region_stmts” would return:

    iterator_pair<const_reverse_iterator>

Hmm, sounds like a nice abstraction but I see 2 problems with that:

1) How can I define a constructor of the iterator_pair when I need something 
like:
iterator_pair<const_iterator> (region_begin, region_end)?

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


Reply via email to