public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Richard Biener <richard.guenther@gmail.com>
To: Feng Xue OS <fxue@os.amperecomputing.com>
Cc: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] vect: Unify bbs in loop_vec_info and bb_vec_info
Date: Wed, 29 May 2024 09:14:32 +0200	[thread overview]
Message-ID: <CAFiYyc3OnhvecbeZVrL7zJGPnDnw5BNkfO0WbQekZ6cKenopQA@mail.gmail.com> (raw)
In-Reply-To: <LV2PR01MB7839542FCAC840D9209D047AF7F12@LV2PR01MB7839.prod.exchangelabs.com>

On Tue, May 28, 2024 at 6:11 PM Feng Xue OS <fxue@os.amperecomputing.com> wrote:
>
> Because bbs of loop_vec_info need to be allocated via old-fashion
> XCNEWVEC, in order to receive result from dfs_enumerate_from(),
> so have to make bb_vec_info align with loop_vec_info, use
> basic_block * instead of vec<basic_block>. Another reason is that
> some loop vect related codes assume that bbs is a pointer, such
> as using LOOP_VINFO_BBS() to directly free the bbs area.

I think dfs_enumerate_from is fine with receiving bbs.address ()
(if you first grow the vector, of course).  There might be other code
that needs changing, sure.

> While encapsulating bbs into array_slice might make changed code
> more wordy. So still choose basic_block * as its type. Updated the
> patch by removing bbs_as_vector.

The updated patch looks good to me.  Lifetime management of
the base class bbs done differently by _loop_vec_info and _bb_vec_info
is a bit ugly but it's a well isolated fact.

Thus, OK.

I do think we can turn the basic_block * back to a vec<> but this
can be done as followup if anybody has spare cycles.

Thanks,
Richard.

> Feng.
> ----
> gcc/
>         * tree-vect-loop.cc (_loop_vec_info::_loop_vec_info): Move
>         initialization of bbs to explicit construction code.  Adjust the
>         definition of nbbs.
>         (update_epilogue_loop_vinfo): Update nbbs for epilog vinfo.
>         * tree-vect-pattern.cc (vect_determine_precisions): Make
>         loop_vec_info and bb_vec_info share same code.
>         (vect_pattern_recog): Remove duplicated vect_pattern_recog_1 loop.
>         * tree-vect-slp.cc (vect_get_and_check_slp_defs): Access to bbs[0]
>         via base vec_info class.
>         (_bb_vec_info::_bb_vec_info): Initialize bbs and nbbs using data
>         fields of input auto_vec<> bbs.
>         (vect_slp_region): Use access to nbbs to replace original
>         bbs.length().
>         (vect_schedule_slp_node): Access to bbs[0] via base vec_info class.
>         * tree-vectorizer.cc (vec_info::vec_info): Add initialization of
>         bbs and nbbs.
>         (vec_info::insert_seq_on_entry): Access to bbs[0] via base vec_info
>         class.
>         * tree-vectorizer.h (vec_info): Add new fields bbs and nbbs.
>         (LOOP_VINFO_NBBS): New macro.
>         (BB_VINFO_BBS): Rename BB_VINFO_BB to BB_VINFO_BBS.
>         (BB_VINFO_NBBS): New macro.
>         (_loop_vec_info): Remove field bbs.
>         (_bb_vec_info): Rename field bbs.
> ---
>  gcc/tree-vect-loop.cc     |   7 +-
>  gcc/tree-vect-patterns.cc | 142 +++++++++++---------------------------
>  gcc/tree-vect-slp.cc      |  23 +++---
>  gcc/tree-vectorizer.cc    |   7 +-
>  gcc/tree-vectorizer.h     |  19 +++--
>  5 files changed, 70 insertions(+), 128 deletions(-)
>
> diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc
> index 3b94bb13a8b..04a9ac64df7 100644
> --- a/gcc/tree-vect-loop.cc
> +++ b/gcc/tree-vect-loop.cc
> @@ -1028,7 +1028,6 @@ bb_in_loop_p (const_basic_block bb, const void *data)
>  _loop_vec_info::_loop_vec_info (class loop *loop_in, vec_info_shared *shared)
>    : vec_info (vec_info::loop, shared),
>      loop (loop_in),
> -    bbs (XCNEWVEC (basic_block, loop->num_nodes)),
>      num_itersm1 (NULL_TREE),
>      num_iters (NULL_TREE),
>      num_iters_unchanged (NULL_TREE),
> @@ -1079,8 +1078,9 @@ _loop_vec_info::_loop_vec_info (class loop *loop_in, vec_info_shared *shared)
>       case of the loop forms we allow, a dfs order of the BBs would the same
>       as reversed postorder traversal, so we are safe.  */
>
> -  unsigned int nbbs = dfs_enumerate_from (loop->header, 0, bb_in_loop_p,
> -                                         bbs, loop->num_nodes, loop);
> +  bbs = XCNEWVEC (basic_block, loop->num_nodes);
> +  nbbs = dfs_enumerate_from (loop->header, 0, bb_in_loop_p, bbs,
> +                            loop->num_nodes, loop);
>    gcc_assert (nbbs == loop->num_nodes);
>
>    for (unsigned int i = 0; i < nbbs; i++)
> @@ -11667,6 +11667,7 @@ update_epilogue_loop_vinfo (class loop *epilogue, tree advance)
>
>    free (LOOP_VINFO_BBS (epilogue_vinfo));
>    LOOP_VINFO_BBS (epilogue_vinfo) = epilogue_bbs;
> +  LOOP_VINFO_NBBS (epilogue_vinfo) = epilogue->num_nodes;
>
>    /* Advance data_reference's with the number of iterations of the previous
>       loop and its prologue.  */
> diff --git a/gcc/tree-vect-patterns.cc b/gcc/tree-vect-patterns.cc
> index 8929e5aa7f3..88e7e34d78d 100644
> --- a/gcc/tree-vect-patterns.cc
> +++ b/gcc/tree-vect-patterns.cc
> @@ -6925,81 +6925,41 @@ vect_determine_stmt_precisions (vec_info *vinfo, stmt_vec_info stmt_info)
>  void
>  vect_determine_precisions (vec_info *vinfo)
>  {
> +  basic_block *bbs = vinfo->bbs;
> +  unsigned int nbbs = vinfo->nbbs;
> +
>    DUMP_VECT_SCOPE ("vect_determine_precisions");
>
> -  if (loop_vec_info loop_vinfo = dyn_cast <loop_vec_info> (vinfo))
> +  for (unsigned int i = 0; i < nbbs; i++)
>      {
> -      class loop *loop = LOOP_VINFO_LOOP (loop_vinfo);
> -      basic_block *bbs = LOOP_VINFO_BBS (loop_vinfo);
> -      unsigned int nbbs = loop->num_nodes;
> -
> -      for (unsigned int i = 0; i < nbbs; i++)
> +      basic_block bb = bbs[i];
> +      for (auto gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>         {
> -         basic_block bb = bbs[i];
> -         for (auto gsi = gsi_start_phis (bb);
> -              !gsi_end_p (gsi); gsi_next (&gsi))
> -           {
> -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi.phi ());
> -             if (stmt_info)
> -               vect_determine_mask_precision (vinfo, stmt_info);
> -           }
> -         for (auto si = gsi_start_bb (bb); !gsi_end_p (si); gsi_next (&si))
> -           if (!is_gimple_debug (gsi_stmt (si)))
> -             vect_determine_mask_precision
> -               (vinfo, vinfo->lookup_stmt (gsi_stmt (si)));
> +         stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi.phi ());
> +         if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> +           vect_determine_mask_precision (vinfo, stmt_info);
>         }
> -      for (unsigned int i = 0; i < nbbs; i++)
> +      for (auto gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>         {
> -         basic_block bb = bbs[nbbs - i - 1];
> -         for (gimple_stmt_iterator si = gsi_last_bb (bb);
> -              !gsi_end_p (si); gsi_prev (&si))
> -           if (!is_gimple_debug (gsi_stmt (si)))
> -             vect_determine_stmt_precisions
> -               (vinfo, vinfo->lookup_stmt (gsi_stmt (si)));
> -         for (auto gsi = gsi_start_phis (bb);
> -              !gsi_end_p (gsi); gsi_next (&gsi))
> -           {
> -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi.phi ());
> -             if (stmt_info)
> -               vect_determine_stmt_precisions (vinfo, stmt_info);
> -           }
> +         stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi_stmt (gsi));
> +         if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> +           vect_determine_mask_precision (vinfo, stmt_info);
>         }
>      }
> -  else
> +  for (unsigned int i = 0; i < nbbs; i++)
>      {
> -      bb_vec_info bb_vinfo = as_a <bb_vec_info> (vinfo);
> -      for (unsigned i = 0; i < bb_vinfo->bbs.length (); ++i)
> +      basic_block bb = bbs[nbbs - i - 1];
> +      for (auto gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
>         {
> -         basic_block bb = bb_vinfo->bbs[i];
> -         for (auto gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> -           {
> -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi.phi ());
> -             if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> -               vect_determine_mask_precision (vinfo, stmt_info);
> -           }
> -         for (auto gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> -           {
> -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi_stmt (gsi));
> -             if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> -               vect_determine_mask_precision (vinfo, stmt_info);
> -           }
> +         stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi_stmt (gsi));
> +         if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> +           vect_determine_stmt_precisions (vinfo, stmt_info);
>         }
> -      for (int i = bb_vinfo->bbs.length () - 1; i != -1; --i)
> +      for (auto gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>         {
> -         for (gimple_stmt_iterator gsi = gsi_last_bb (bb_vinfo->bbs[i]);
> -              !gsi_end_p (gsi); gsi_prev (&gsi))
> -           {
> -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi_stmt (gsi));
> -             if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> -               vect_determine_stmt_precisions (vinfo, stmt_info);
> -           }
> -         for (auto gsi = gsi_start_phis (bb_vinfo->bbs[i]);
> -              !gsi_end_p (gsi); gsi_next (&gsi))
> -           {
> -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi.phi ());
> -             if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> -               vect_determine_stmt_precisions (vinfo, stmt_info);
> -           }
> +         stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi.phi ());
> +         if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> +           vect_determine_stmt_precisions (vinfo, stmt_info);
>         }
>      }
>  }
> @@ -7328,56 +7288,32 @@ vect_pattern_recog_1 (vec_info *vinfo,
>  void
>  vect_pattern_recog (vec_info *vinfo)
>  {
> -  class loop *loop;
> -  basic_block *bbs;
> -  unsigned int nbbs;
> -  gimple_stmt_iterator si;
> -  unsigned int i, j;
> +  basic_block *bbs = vinfo->bbs;
> +  unsigned int nbbs = vinfo->nbbs;
>
>    vect_determine_precisions (vinfo);
>
>    DUMP_VECT_SCOPE ("vect_pattern_recog");
>
> -  if (loop_vec_info loop_vinfo = dyn_cast <loop_vec_info> (vinfo))
> +  /* Scan through the stmts in the region, applying the pattern recognition
> +     functions starting at each stmt visited.  */
> +  for (unsigned i = 0; i < nbbs; i++)
>      {
> -      loop = LOOP_VINFO_LOOP (loop_vinfo);
> -      bbs = LOOP_VINFO_BBS (loop_vinfo);
> -      nbbs = loop->num_nodes;
> +      basic_block bb = bbs[i];
>
> -      /* Scan through the loop stmts, applying the pattern recognition
> -        functions starting at each stmt visited:  */
> -      for (i = 0; i < nbbs; i++)
> +      for (auto si = gsi_start_bb (bb); !gsi_end_p (si); gsi_next (&si))
>         {
> -         basic_block bb = bbs[i];
> -         for (si = gsi_start_bb (bb); !gsi_end_p (si); gsi_next (&si))
> -           {
> -             if (is_gimple_debug (gsi_stmt (si)))
> -               continue;
> -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi_stmt (si));
> -             /* Scan over all generic vect_recog_xxx_pattern functions.  */
> -             for (j = 0; j < NUM_PATTERNS; j++)
> -               vect_pattern_recog_1 (vinfo, &vect_vect_recog_func_ptrs[j],
> -                                     stmt_info);
> -           }
> +         stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi_stmt (si));
> +
> +         if (!stmt_info || !STMT_VINFO_VECTORIZABLE (stmt_info))
> +           continue;
> +
> +         /* Scan over all generic vect_recog_xxx_pattern functions.  */
> +         for (unsigned j = 0; j < NUM_PATTERNS; j++)
> +           vect_pattern_recog_1 (vinfo, &vect_vect_recog_func_ptrs[j],
> +                                 stmt_info);
>         }
>      }
> -  else
> -    {
> -      bb_vec_info bb_vinfo = as_a <bb_vec_info> (vinfo);
> -      for (unsigned i = 0; i < bb_vinfo->bbs.length (); ++i)
> -       for (gimple_stmt_iterator gsi = gsi_start_bb (bb_vinfo->bbs[i]);
> -            !gsi_end_p (gsi); gsi_next (&gsi))
> -         {
> -           stmt_vec_info stmt_info = bb_vinfo->lookup_stmt (gsi_stmt (gsi));
> -           if (!stmt_info || !STMT_VINFO_VECTORIZABLE (stmt_info))
> -             continue;
> -
> -           /* Scan over all generic vect_recog_xxx_pattern functions.  */
> -           for (j = 0; j < NUM_PATTERNS; j++)
> -             vect_pattern_recog_1 (vinfo,
> -                                   &vect_vect_recog_func_ptrs[j], stmt_info);
> -         }
> -    }
>
>    /* After this no more add_stmt calls are allowed.  */
>    vinfo->stmt_vec_info_ro = true;
> diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc
> index 7a963e28063..bc7a85d6bfc 100644
> --- a/gcc/tree-vect-slp.cc
> +++ b/gcc/tree-vect-slp.cc
> @@ -748,8 +748,7 @@ vect_get_and_check_slp_defs (vec_info *vinfo, unsigned char swap,
>           && is_a <bb_vec_info> (vinfo)
>           && TREE_CODE (oprnd) == SSA_NAME
>           && !SSA_NAME_IS_DEFAULT_DEF (oprnd)
> -         && !dominated_by_p (CDI_DOMINATORS,
> -                             as_a <bb_vec_info> (vinfo)->bbs[0],
> +         && !dominated_by_p (CDI_DOMINATORS, vinfo->bbs[0],
>                               gimple_bb (SSA_NAME_DEF_STMT (oprnd))))
>         {
>           if (dump_enabled_p ())
> @@ -6293,10 +6292,16 @@ vect_detect_hybrid_slp (loop_vec_info loop_vinfo)
>
>  _bb_vec_info::_bb_vec_info (vec<basic_block> _bbs, vec_info_shared *shared)
>    : vec_info (vec_info::bb, shared),
> -    bbs (_bbs),
>      roots (vNULL)
>  {
> -  for (unsigned i = 0; i < bbs.length (); ++i)
> +  /* The region we are operating on.  bbs[0] is the entry, excluding
> +     its PHI nodes.  In the future we might want to track an explicit
> +     entry edge to cover bbs[0] PHI nodes and have a region entry
> +     insert location.  */
> +  bbs = _bbs.address ();
> +  nbbs = _bbs.length ();
> +
> +  for (unsigned i = 0; i < nbbs; ++i)
>      {
>        if (i != 0)
>         for (gphi_iterator si = gsi_start_phis (bbs[i]); !gsi_end_p (si);
> @@ -6325,7 +6330,7 @@ _bb_vec_info::_bb_vec_info (vec<basic_block> _bbs, vec_info_shared *shared)
>  _bb_vec_info::~_bb_vec_info ()
>  {
>    /* Reset region marker.  */
> -  for (unsigned i = 0; i < bbs.length (); ++i)
> +  for (unsigned i = 0; i < nbbs; ++i)
>      {
>        if (i != 0)
>         for (gphi_iterator si = gsi_start_phis (bbs[i]); !gsi_end_p (si);
> @@ -7630,7 +7635,7 @@ vect_slp_is_lane_insert (gimple *use_stmt, tree vec, unsigned *this_lane)
>  static void
>  vect_slp_check_for_roots (bb_vec_info bb_vinfo)
>  {
> -  for (unsigned i = 0; i < bb_vinfo->bbs.length (); ++i)
> +  for (unsigned i = 0; i < bb_vinfo->nbbs; ++i)
>      for (gimple_stmt_iterator gsi = gsi_start_bb (bb_vinfo->bbs[i]);
>          !gsi_end_p (gsi); gsi_next (&gsi))
>      {
> @@ -8114,7 +8119,7 @@ vect_slp_region (vec<basic_block> bbs, vec<data_reference_p> datarefs,
>              we vectorized all if-converted code.  */
>           if ((!profitable_subgraphs.is_empty () || force_clear) && orig_loop)
>             {
> -             gcc_assert (bb_vinfo->bbs.length () == 1);
> +             gcc_assert (bb_vinfo->nbbs == 1);
>               for (gimple_stmt_iterator gsi = gsi_start_bb (bb_vinfo->bbs[0]);
>                    !gsi_end_p (gsi); gsi_next (&gsi))
>                 {
> @@ -9618,14 +9623,14 @@ vect_schedule_slp_node (vec_info *vinfo,
>        if (!last_stmt)
>         {
>           gcc_assert (seen_vector_def);
> -         si = gsi_after_labels (as_a <bb_vec_info> (vinfo)->bbs[0]);
> +         si = gsi_after_labels (vinfo->bbs[0]);
>         }
>        else if (is_ctrl_altering_stmt (last_stmt))
>         {
>           /* We split regions to vectorize at control altering stmts
>              with a definition so this must be an external which
>              we can insert at the start of the region.  */
> -         si = gsi_after_labels (as_a <bb_vec_info> (vinfo)->bbs[0]);
> +         si = gsi_after_labels (vinfo->bbs[0]);
>         }
>        else if (is_a <bb_vec_info> (vinfo)
>                && gimple_bb (last_stmt) != gimple_bb (stmt_info->stmt)
> diff --git a/gcc/tree-vectorizer.cc b/gcc/tree-vectorizer.cc
> index 9001b738bf3..1fb4fb36ed4 100644
> --- a/gcc/tree-vectorizer.cc
> +++ b/gcc/tree-vectorizer.cc
> @@ -463,7 +463,9 @@ shrink_simd_arrays
>  vec_info::vec_info (vec_info::vec_kind kind_in, vec_info_shared *shared_)
>    : kind (kind_in),
>      shared (shared_),
> -    stmt_vec_info_ro (false)
> +    stmt_vec_info_ro (false),
> +    bbs (NULL),
> +    nbbs (0)
>  {
>    stmt_vec_infos.create (50);
>  }
> @@ -660,9 +662,8 @@ vec_info::insert_seq_on_entry (stmt_vec_info context, gimple_seq seq)
>      }
>    else
>      {
> -      bb_vec_info bb_vinfo = as_a <bb_vec_info> (this);
>        gimple_stmt_iterator gsi_region_begin
> -       = gsi_after_labels (bb_vinfo->bbs[0]);
> +       = gsi_after_labels (bbs[0]);
>        gsi_insert_seq_before (&gsi_region_begin, seq, GSI_SAME_STMT);
>      }
>  }
> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
> index 93bc30ef660..bd4f5952f4b 100644
> --- a/gcc/tree-vectorizer.h
> +++ b/gcc/tree-vectorizer.h
> @@ -499,6 +499,12 @@ public:
>       made any decisions about which vector modes to use.  */
>    machine_mode vector_mode;
>
> +  /* The basic blocks in the vectorization region.  */
> +  basic_block *bbs;
> +
> +  /* The count of the basic blocks in the vectorization region.  */
> +  unsigned int nbbs;
> +
>  private:
>    stmt_vec_info new_stmt_vec_info (gimple *stmt);
>    void set_vinfo_for_stmt (gimple *, stmt_vec_info, bool = true);
> @@ -679,9 +685,6 @@ public:
>    /* The loop to which this info struct refers to.  */
>    class loop *loop;
>
> -  /* The loop basic blocks.  */
> -  basic_block *bbs;
> -
>    /* Number of latch executions.  */
>    tree num_itersm1;
>    /* Number of iterations.  */
> @@ -969,6 +972,7 @@ public:
>  #define LOOP_VINFO_EPILOGUE_IV_EXIT(L)     (L)->vec_epilogue_loop_iv_exit
>  #define LOOP_VINFO_SCALAR_IV_EXIT(L)       (L)->scalar_loop_iv_exit
>  #define LOOP_VINFO_BBS(L)                  (L)->bbs
> +#define LOOP_VINFO_NBBS(L)                 (L)->nbbs
>  #define LOOP_VINFO_NITERSM1(L)             (L)->num_itersm1
>  #define LOOP_VINFO_NITERS(L)               (L)->num_iters
>  /* Since LOOP_VINFO_NITERS and LOOP_VINFO_NITERSM1 can change after
> @@ -1094,16 +1098,11 @@ public:
>    _bb_vec_info (vec<basic_block> bbs, vec_info_shared *);
>    ~_bb_vec_info ();
>
> -  /* The region we are operating on.  bbs[0] is the entry, excluding
> -     its PHI nodes.  In the future we might want to track an explicit
> -     entry edge to cover bbs[0] PHI nodes and have a region entry
> -     insert location.  */
> -  vec<basic_block> bbs;
> -
>    vec<slp_root> roots;
>  } *bb_vec_info;
>
> -#define BB_VINFO_BB(B)               (B)->bb
> +#define BB_VINFO_BBS(B)              (B)->bbs
> +#define BB_VINFO_NBBS(B)             (B)->nbbs
>  #define BB_VINFO_GROUPED_STORES(B)   (B)->grouped_stores
>  #define BB_VINFO_SLP_INSTANCES(B)    (B)->slp_instances
>  #define BB_VINFO_DATAREFS(B)         (B)->shared->datarefs
> --
> 2.17.1
>
> ________________________________________
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: Tuesday, May 28, 2024 5:43 PM
> To: Feng Xue OS
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] vect: Unify bbs in loop_vec_info and bb_vec_info
>
> On Sat, May 25, 2024 at 4:54 PM Feng Xue OS <fxue@os.amperecomputing.com> wrote:
> >
> > Both derived classes ( loop_vec_info/bb_vec_info) have their own "bbs"
> > field, which have exactly same purpose of recording all basic blocks
> > inside the corresponding vect region, while the fields are composed by
> > different data type, one is normal array, the other is auto_vec. This
> > difference causes some duplicated code even handling the same stuff,
> > almost in tree-vect-patterns. One refinement is lifting this field into the
> > base class "vec_info", and reset its value to the continuous memory area
> > pointed by two old "bbs" in each constructor of derived classes.
>
> Nice.  But.  bbs_as_vector - why is that necessary?  Why is vinfo->bbs
> not a vec<basic_block>?  Having bbs and nbbs feels like a step back.
>
> Note the code duplications can probably be removed by "indirecting"
> through an array_slice.
>
> I'm a bit torn to approve this as-is given the above.  Can you explain what
> made you not choose vec<> for bbs?  I bet you tried.
>
> Richard.
>
> > Regression test on x86-64 and aarch64.
> >
> > Feng
> > --
> > gcc/
> >         * tree-vect-loop.cc (_loop_vec_info::_loop_vec_info): Move
> >         initialization of bbs to explicit construction code.  Adjust the
> >         definition of nbbs.
> >         * tree-vect-pattern.cc (vect_determine_precisions): Make
> >         loop_vec_info and bb_vec_info share same code.
> >         (vect_pattern_recog): Remove duplicated vect_pattern_recog_1 loop.
> >         * tree-vect-slp.cc (vect_get_and_check_slp_defs): Access to bbs[0]
> >         via base vec_info class.
> >         (_bb_vec_info::_bb_vec_info): Initialize bbs and nbbs using data
> >         fields of input auto_vec<> bbs.
> >         (_bb_vec_info::_bb_vec_info): Add assertions on bbs and nbbs to ensure
> >         they are not changed externally.
> >         (vect_slp_region): Use access to nbbs to replace original
> >         bbs.length().
> >         (vect_schedule_slp_node): Access to bbs[0] via base vec_info class.
> >         * tree-vectorizer.cc (vec_info::vec_info): Add initialization of
> >         bbs and nbbs.
> >         (vec_info::insert_seq_on_entry): Access to bbs[0] via base vec_info
> >         class.
> >         * tree-vectorizer.h (vec_info): Add new fields bbs and nbbs.
> >         (_loop_vec_info): Remove field bbs.
> >         (_bb_vec_info): Rename old bbs field to bbs_as_vector, and make it
> >         be private.
> > ---
> >  gcc/tree-vect-loop.cc     |   6 +-
> >  gcc/tree-vect-patterns.cc | 142 +++++++++++---------------------------
> >  gcc/tree-vect-slp.cc      |  24 ++++---
> >  gcc/tree-vectorizer.cc    |   7 +-
> >  gcc/tree-vectorizer.h     |  19 ++---
> >  5 files changed, 72 insertions(+), 126 deletions(-)
> >
> > diff --git a/gcc/tree-vect-loop.cc b/gcc/tree-vect-loop.cc
> > index 83c0544b6aa..aef17420a5f 100644
> > --- a/gcc/tree-vect-loop.cc
> > +++ b/gcc/tree-vect-loop.cc
> > @@ -1028,7 +1028,6 @@ bb_in_loop_p (const_basic_block bb, const void *data)
> >  _loop_vec_info::_loop_vec_info (class loop *loop_in, vec_info_shared *shared)
> >    : vec_info (vec_info::loop, shared),
> >      loop (loop_in),
> > -    bbs (XCNEWVEC (basic_block, loop->num_nodes)),
> >      num_itersm1 (NULL_TREE),
> >      num_iters (NULL_TREE),
> >      num_iters_unchanged (NULL_TREE),
> > @@ -1079,8 +1078,9 @@ _loop_vec_info::_loop_vec_info (class loop *loop_in, vec_info_shared *shared)
> >       case of the loop forms we allow, a dfs order of the BBs would the same
> >       as reversed postorder traversal, so we are safe.  */
> >
> > -  unsigned int nbbs = dfs_enumerate_from (loop->header, 0, bb_in_loop_p,
> > -                                         bbs, loop->num_nodes, loop);
> > +  bbs = XCNEWVEC (basic_block, loop->num_nodes);
> > +  nbbs = dfs_enumerate_from (loop->header, 0, bb_in_loop_p, bbs,
> > +                            loop->num_nodes, loop);
> >    gcc_assert (nbbs == loop->num_nodes);
> >
> >    for (unsigned int i = 0; i < nbbs; i++)
> > diff --git a/gcc/tree-vect-patterns.cc b/gcc/tree-vect-patterns.cc
> > index a313dc64643..848a3195a93 100644
> > --- a/gcc/tree-vect-patterns.cc
> > +++ b/gcc/tree-vect-patterns.cc
> > @@ -6925,81 +6925,41 @@ vect_determine_stmt_precisions (vec_info *vinfo, stmt_vec_info stmt_info)
> >  void
> >  vect_determine_precisions (vec_info *vinfo)
> >  {
> > +  basic_block *bbs = vinfo->bbs;
> > +  unsigned int nbbs = vinfo->nbbs;
> > +
> >    DUMP_VECT_SCOPE ("vect_determine_precisions");
> >
> > -  if (loop_vec_info loop_vinfo = dyn_cast <loop_vec_info> (vinfo))
> > +  for (unsigned int i = 0; i < nbbs; i++)
> >      {
> > -      class loop *loop = LOOP_VINFO_LOOP (loop_vinfo);
> > -      basic_block *bbs = LOOP_VINFO_BBS (loop_vinfo);
> > -      unsigned int nbbs = loop->num_nodes;
> > -
> > -      for (unsigned int i = 0; i < nbbs; i++)
> > +      basic_block bb = bbs[i];
> > +      for (auto gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> >         {
> > -         basic_block bb = bbs[i];
> > -         for (auto gsi = gsi_start_phis (bb);
> > -              !gsi_end_p (gsi); gsi_next (&gsi))
> > -           {
> > -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi.phi ());
> > -             if (stmt_info)
> > -               vect_determine_mask_precision (vinfo, stmt_info);
> > -           }
> > -         for (auto si = gsi_start_bb (bb); !gsi_end_p (si); gsi_next (&si))
> > -           if (!is_gimple_debug (gsi_stmt (si)))
> > -             vect_determine_mask_precision
> > -               (vinfo, vinfo->lookup_stmt (gsi_stmt (si)));
> > +         stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi.phi ());
> > +         if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> > +           vect_determine_mask_precision (vinfo, stmt_info);
> >         }
> > -      for (unsigned int i = 0; i < nbbs; i++)
> > +      for (auto gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> >         {
> > -         basic_block bb = bbs[nbbs - i - 1];
> > -         for (gimple_stmt_iterator si = gsi_last_bb (bb);
> > -              !gsi_end_p (si); gsi_prev (&si))
> > -           if (!is_gimple_debug (gsi_stmt (si)))
> > -             vect_determine_stmt_precisions
> > -               (vinfo, vinfo->lookup_stmt (gsi_stmt (si)));
> > -         for (auto gsi = gsi_start_phis (bb);
> > -              !gsi_end_p (gsi); gsi_next (&gsi))
> > -           {
> > -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi.phi ());
> > -             if (stmt_info)
> > -               vect_determine_stmt_precisions (vinfo, stmt_info);
> > -           }
> > +         stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi_stmt (gsi));
> > +         if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> > +           vect_determine_mask_precision (vinfo, stmt_info);
> >         }
> >      }
> > -  else
> > +  for (unsigned int i = 0; i < nbbs; i++)
> >      {
> > -      bb_vec_info bb_vinfo = as_a <bb_vec_info> (vinfo);
> > -      for (unsigned i = 0; i < bb_vinfo->bbs.length (); ++i)
> > +      basic_block bb = bbs[nbbs - i - 1];
> > +      for (auto gsi = gsi_last_bb (bb); !gsi_end_p (gsi); gsi_prev (&gsi))
> >         {
> > -         basic_block bb = bb_vinfo->bbs[i];
> > -         for (auto gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> > -           {
> > -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi.phi ());
> > -             if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> > -               vect_determine_mask_precision (vinfo, stmt_info);
> > -           }
> > -         for (auto gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> > -           {
> > -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi_stmt (gsi));
> > -             if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> > -               vect_determine_mask_precision (vinfo, stmt_info);
> > -           }
> > +         stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi_stmt (gsi));
> > +         if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> > +           vect_determine_stmt_precisions (vinfo, stmt_info);
> >         }
> > -      for (int i = bb_vinfo->bbs.length () - 1; i != -1; --i)
> > +      for (auto gsi = gsi_start_phis (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> >         {
> > -         for (gimple_stmt_iterator gsi = gsi_last_bb (bb_vinfo->bbs[i]);
> > -              !gsi_end_p (gsi); gsi_prev (&gsi))
> > -           {
> > -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi_stmt (gsi));
> > -             if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> > -               vect_determine_stmt_precisions (vinfo, stmt_info);
> > -           }
> > -         for (auto gsi = gsi_start_phis (bb_vinfo->bbs[i]);
> > -              !gsi_end_p (gsi); gsi_next (&gsi))
> > -           {
> > -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi.phi ());
> > -             if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> > -               vect_determine_stmt_precisions (vinfo, stmt_info);
> > -           }
> > +         stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi.phi ());
> > +         if (stmt_info && STMT_VINFO_VECTORIZABLE (stmt_info))
> > +           vect_determine_stmt_precisions (vinfo, stmt_info);
> >         }
> >      }
> >  }
> > @@ -7328,56 +7288,32 @@ vect_pattern_recog_1 (vec_info *vinfo,
> >  void
> >  vect_pattern_recog (vec_info *vinfo)
> >  {
> > -  class loop *loop;
> > -  basic_block *bbs;
> > -  unsigned int nbbs;
> > -  gimple_stmt_iterator si;
> > -  unsigned int i, j;
> > +  basic_block *bbs = vinfo->bbs;
> > +  unsigned int nbbs = vinfo->nbbs;
> >
> >    vect_determine_precisions (vinfo);
> >
> >    DUMP_VECT_SCOPE ("vect_pattern_recog");
> >
> > -  if (loop_vec_info loop_vinfo = dyn_cast <loop_vec_info> (vinfo))
> > +  /* Scan through the stmts in the region, applying the pattern recognition
> > +     functions starting at each stmt visited.  */
> > +  for (unsigned i = 0; i < nbbs; i++)
> >      {
> > -      loop = LOOP_VINFO_LOOP (loop_vinfo);
> > -      bbs = LOOP_VINFO_BBS (loop_vinfo);
> > -      nbbs = loop->num_nodes;
> > +      basic_block bb = bbs[i];
> >
> > -      /* Scan through the loop stmts, applying the pattern recognition
> > -        functions starting at each stmt visited:  */
> > -      for (i = 0; i < nbbs; i++)
> > +      for (auto si = gsi_start_bb (bb); !gsi_end_p (si); gsi_next (&si))
> >         {
> > -         basic_block bb = bbs[i];
> > -         for (si = gsi_start_bb (bb); !gsi_end_p (si); gsi_next (&si))
> > -           {
> > -             if (is_gimple_debug (gsi_stmt (si)))
> > -               continue;
> > -             stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi_stmt (si));
> > -             /* Scan over all generic vect_recog_xxx_pattern functions.  */
> > -             for (j = 0; j < NUM_PATTERNS; j++)
> > -               vect_pattern_recog_1 (vinfo, &vect_vect_recog_func_ptrs[j],
> > -                                     stmt_info);
> > -           }
> > +         stmt_vec_info stmt_info = vinfo->lookup_stmt (gsi_stmt (si));
> > +
> > +         if (!stmt_info || !STMT_VINFO_VECTORIZABLE (stmt_info))
> > +           continue;
> > +
> > +         /* Scan over all generic vect_recog_xxx_pattern functions.  */
> > +         for (unsigned j = 0; j < NUM_PATTERNS; j++)
> > +           vect_pattern_recog_1 (vinfo, &vect_vect_recog_func_ptrs[j],
> > +                                 stmt_info);
> >         }
> >      }
> > -  else
> > -    {
> > -      bb_vec_info bb_vinfo = as_a <bb_vec_info> (vinfo);
> > -      for (unsigned i = 0; i < bb_vinfo->bbs.length (); ++i)
> > -       for (gimple_stmt_iterator gsi = gsi_start_bb (bb_vinfo->bbs[i]);
> > -            !gsi_end_p (gsi); gsi_next (&gsi))
> > -         {
> > -           stmt_vec_info stmt_info = bb_vinfo->lookup_stmt (gsi_stmt (gsi));
> > -           if (!stmt_info || !STMT_VINFO_VECTORIZABLE (stmt_info))
> > -             continue;
> > -
> > -           /* Scan over all generic vect_recog_xxx_pattern functions.  */
> > -           for (j = 0; j < NUM_PATTERNS; j++)
> > -             vect_pattern_recog_1 (vinfo,
> > -                                   &vect_vect_recog_func_ptrs[j], stmt_info);
> > -         }
> > -    }
> >
> >    /* After this no more add_stmt calls are allowed.  */
> >    vinfo->stmt_vec_info_ro = true;
> > diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc
> > index c7ed520b629..2e0244cf582 100644
> > --- a/gcc/tree-vect-slp.cc
> > +++ b/gcc/tree-vect-slp.cc
> > @@ -748,8 +748,7 @@ vect_get_and_check_slp_defs (vec_info *vinfo, unsigned char swap,
> >           && is_a <bb_vec_info> (vinfo)
> >           && TREE_CODE (oprnd) == SSA_NAME
> >           && !SSA_NAME_IS_DEFAULT_DEF (oprnd)
> > -         && !dominated_by_p (CDI_DOMINATORS,
> > -                             as_a <bb_vec_info> (vinfo)->bbs[0],
> > +         && !dominated_by_p (CDI_DOMINATORS, vinfo->bbs[0],
> >                               gimple_bb (SSA_NAME_DEF_STMT (oprnd))))
> >         {
> >           if (dump_enabled_p ())
> > @@ -6288,10 +6287,13 @@ vect_detect_hybrid_slp (loop_vec_info loop_vinfo)
> >
> >  _bb_vec_info::_bb_vec_info (vec<basic_block> _bbs, vec_info_shared *shared)
> >    : vec_info (vec_info::bb, shared),
> > -    bbs (_bbs),
> > +    bbs_as_vector (_bbs),
> >      roots (vNULL)
> >  {
> > -  for (unsigned i = 0; i < bbs.length (); ++i)
> > +  bbs = bbs_as_vector.address ();
> > +  nbbs = bbs_as_vector.length ();
> > +
> > +  for (unsigned i = 0; i < nbbs; ++i)
> >      {
> >        if (i != 0)
> >         for (gphi_iterator si = gsi_start_phis (bbs[i]); !gsi_end_p (si);
> > @@ -6319,8 +6321,12 @@ _bb_vec_info::_bb_vec_info (vec<basic_block> _bbs, vec_info_shared *shared)
> >
> >  _bb_vec_info::~_bb_vec_info ()
> >  {
> > +  /* Check that bbs stored in the vector is not changed externally.  */
> > +  gcc_assert (bbs == bbs_as_vector.address ());
> > +  gcc_assert (nbbs = bbs_as_vector.length ());
> > +
> >    /* Reset region marker.  */
> > -  for (unsigned i = 0; i < bbs.length (); ++i)
> > +  for (unsigned i = 0; i < nbbs; ++i)
> >      {
> >        if (i != 0)
> >         for (gphi_iterator si = gsi_start_phis (bbs[i]); !gsi_end_p (si);
> > @@ -7625,7 +7631,7 @@ vect_slp_is_lane_insert (gimple *use_stmt, tree vec, unsigned *this_lane)
> >  static void
> >  vect_slp_check_for_roots (bb_vec_info bb_vinfo)
> >  {
> > -  for (unsigned i = 0; i < bb_vinfo->bbs.length (); ++i)
> > +  for (unsigned i = 0; i < bb_vinfo->nbbs; ++i)
> >      for (gimple_stmt_iterator gsi = gsi_start_bb (bb_vinfo->bbs[i]);
> >          !gsi_end_p (gsi); gsi_next (&gsi))
> >      {
> > @@ -8109,7 +8115,7 @@ vect_slp_region (vec<basic_block> bbs, vec<data_reference_p> datarefs,
> >              we vectorized all if-converted code.  */
> >           if ((!profitable_subgraphs.is_empty () || force_clear) && orig_loop)
> >             {
> > -             gcc_assert (bb_vinfo->bbs.length () == 1);
> > +             gcc_assert (bb_vinfo->nbbs == 1);
> >               for (gimple_stmt_iterator gsi = gsi_start_bb (bb_vinfo->bbs[0]);
> >                    !gsi_end_p (gsi); gsi_next (&gsi))
> >                 {
> > @@ -9613,14 +9619,14 @@ vect_schedule_slp_node (vec_info *vinfo,
> >        if (!last_stmt)
> >         {
> >           gcc_assert (seen_vector_def);
> > -         si = gsi_after_labels (as_a <bb_vec_info> (vinfo)->bbs[0]);
> > +         si = gsi_after_labels (vinfo->bbs[0]);
> >         }
> >        else if (is_ctrl_altering_stmt (last_stmt))
> >         {
> >           /* We split regions to vectorize at control altering stmts
> >              with a definition so this must be an external which
> >              we can insert at the start of the region.  */
> > -         si = gsi_after_labels (as_a <bb_vec_info> (vinfo)->bbs[0]);
> > +         si = gsi_after_labels (vinfo->bbs[0]);
> >         }
> >        else if (is_a <bb_vec_info> (vinfo)
> >                && gimple_bb (last_stmt) != gimple_bb (stmt_info->stmt)
> > diff --git a/gcc/tree-vectorizer.cc b/gcc/tree-vectorizer.cc
> > index 9001b738bf3..1fb4fb36ed4 100644
> > --- a/gcc/tree-vectorizer.cc
> > +++ b/gcc/tree-vectorizer.cc
> > @@ -463,7 +463,9 @@ shrink_simd_arrays
> >  vec_info::vec_info (vec_info::vec_kind kind_in, vec_info_shared *shared_)
> >    : kind (kind_in),
> >      shared (shared_),
> > -    stmt_vec_info_ro (false)
> > +    stmt_vec_info_ro (false),
> > +    bbs (NULL),
> > +    nbbs (0)
> >  {
> >    stmt_vec_infos.create (50);
> >  }
> > @@ -660,9 +662,8 @@ vec_info::insert_seq_on_entry (stmt_vec_info context, gimple_seq seq)
> >      }
> >    else
> >      {
> > -      bb_vec_info bb_vinfo = as_a <bb_vec_info> (this);
> >        gimple_stmt_iterator gsi_region_begin
> > -       = gsi_after_labels (bb_vinfo->bbs[0]);
> > +       = gsi_after_labels (bbs[0]);
> >        gsi_insert_seq_before (&gsi_region_begin, seq, GSI_SAME_STMT);
> >      }
> >  }
> > diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
> > index 93bc30ef660..ffc2b223dd2 100644
> > --- a/gcc/tree-vectorizer.h
> > +++ b/gcc/tree-vectorizer.h
> > @@ -499,6 +499,12 @@ public:
> >       made any decisions about which vector modes to use.  */
> >    machine_mode vector_mode;
> >
> > +  /* The basic blocks in the vectorization region.  */
> > +  basic_block *bbs;
> > +
> > +  /* The count of the basic blocks in the vectorization region.  */
> > +  unsigned int nbbs;
> > +
> >  private:
> >    stmt_vec_info new_stmt_vec_info (gimple *stmt);
> >    void set_vinfo_for_stmt (gimple *, stmt_vec_info, bool = true);
> > @@ -679,9 +685,6 @@ public:
> >    /* The loop to which this info struct refers to.  */
> >    class loop *loop;
> >
> > -  /* The loop basic blocks.  */
> > -  basic_block *bbs;
> > -
> >    /* Number of latch executions.  */
> >    tree num_itersm1;
> >    /* Number of iterations.  */
> > @@ -1090,15 +1093,15 @@ struct slp_root
> >
> >  typedef class _bb_vec_info : public vec_info
> >  {
> > -public:
> > -  _bb_vec_info (vec<basic_block> bbs, vec_info_shared *);
> > -  ~_bb_vec_info ();
> > -
> >    /* The region we are operating on.  bbs[0] is the entry, excluding
> >       its PHI nodes.  In the future we might want to track an explicit
> >       entry edge to cover bbs[0] PHI nodes and have a region entry
> >       insert location.  */
> > -  vec<basic_block> bbs;
> > +  vec<basic_block> bbs_as_vector;
> > +
> > +public:
> > +  _bb_vec_info (vec<basic_block> bbs, vec_info_shared *);
> > +  ~_bb_vec_info ();
> >
> >    vec<slp_root> roots;
> >  } *bb_vec_info;
> > --
> > 2.17.1

  reply	other threads:[~2024-05-29  7:14 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-05-25 14:54 Feng Xue OS
2024-05-28  9:43 ` Richard Biener
2024-05-28 16:11   ` Feng Xue OS
2024-05-29  7:14     ` Richard Biener [this message]
2024-05-29  8:39       ` Feng Xue OS
2024-05-29  8:47         ` Richard Biener

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAFiYyc3OnhvecbeZVrL7zJGPnDnw5BNkfO0WbQekZ6cKenopQA@mail.gmail.com \
    --to=richard.guenther@gmail.com \
    --cc=fxue@os.amperecomputing.com \
    --cc=gcc-patches@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).