RE: [PATCH 7/19]middle-end: Refactor vectorizer loop conditionals and separate out IV to new variables

[Richard Biener <rguenther@suse.de>]

On Thu, 13 Jul 2023, Tamar Christina wrote:

> > e7ac2b5f3db55de3dbbab7bd2bfe08388f4ec533..cab82d7960e5be517bba2
> > 621f7f4
> > > 888e7bf3c295 100644
> > > --- a/gcc/cfgloop.h
> > > +++ b/gcc/cfgloop.h
> > > @@ -272,6 +272,14 @@ public:
> > >       the basic-block from being collected but its index can still be
> > >       reused.  */
> > >    basic_block former_header;
> > > +
> > > +  /* The controlling loop IV for the current loop when vectorizing.  This IV
> > > +     controls the natural exits of the loop.  */  edge  GTY ((skip
> > > + (""))) vec_loop_iv;
> > > +
> > > +  /* If the loop has multiple exits this structure contains the alternate
> > > +     exits of the loop which are relevant for vectorization.  */
> > > + vec<edge> GTY ((skip (""))) vec_loop_alt_exits;
> > 
> > That's a quite heavy representation and as you say it's vectorizer specific.  May
> > I ask you to eliminate at _least_ vec_loop_alt_exits?
> > Are there not all exits in that vector?  Note there's already the list of exits and if
> > you have the canonical counting IV exit you can match against that to get all
> > the others?
> > 
> 
> Sure, though that means some filtering whenever one iterates over the alt exits,
> not a problem though.
> 
> > >  /* Given LOOP this function generates a new copy of it and puts it
> > >     on E which is either the entry or exit of LOOP.  If SCALAR_LOOP is
> > > @@ -1458,13 +1523,15 @@ slpeel_tree_duplicate_loop_to_edge_cfg (class
> > loop *loop,
> > >    edge exit, new_exit;
> > >    bool duplicate_outer_loop = false;
> > >
> > > -  exit = single_exit (loop);
> > > +  exit = loop->vec_loop_iv;
> > >    at_exit = (e == exit);
> > >    if (!at_exit && e != loop_preheader_edge (loop))
> > >      return NULL;
> > >
> > >    if (scalar_loop == NULL)
> > >      scalar_loop = loop;
> > > +  else
> > > +    vec_init_exit_info (scalar_loop);
> > >
> > >    bbs = XNEWVEC (basic_block, scalar_loop->num_nodes + 1);
> > >    pbbs = bbs + 1;
> > > @@ -1490,13 +1557,17 @@ slpeel_tree_duplicate_loop_to_edge_cfg (class
> > loop *loop,
> > >    bbs[0] = preheader;
> > >    new_bbs = XNEWVEC (basic_block, scalar_loop->num_nodes + 1);
> > >
> > > -  exit = single_exit (scalar_loop);
> > > +  exit = scalar_loop->vec_loop_iv;
> > >    copy_bbs (bbs, scalar_loop->num_nodes + 1, new_bbs,
> > >  	    &exit, 1, &new_exit, NULL,
> > >  	    at_exit ? loop->latch : e->src, true);
> > > -  exit = single_exit (loop);
> > > +  exit = loop->vec_loop_iv;
> > >    basic_block new_preheader = new_bbs[0];
> > >
> > > +  /* Record the new loop exit information.  new_loop doesn't have SCEV
> > data and
> > > +     so we must initialize the exit information.  */
> > > +  vec_init_exit_info (new_loop);
> > > +
> > 
> > You have a mapping of old to new BB so you should be able to
> > map old to new exit by mapping e->src/dest and looking up the new edge?
> > 
> > The vec_loop_iv exit is mapped directly (new_exit).
> > 
> > So I don't really understand what's missing there.
> 
> But I don't have the mapping when the loop as versioned, e.g. by ifcvt.  So in the cases
> where scalar_loop != loop in which case I still need them to match up.
> 
> vect_loop_form_info is destroyed after analysis though and is not available during
> peeling. That's why we copy relevant information out in vect_create_loop_vinfo.
> 
> But in general we only have 1 per loop as well, so it would be the same as using loop_vinfo.
> 
> I could move it into loop_vinfo and then require you to pass the edges to the peeling function
> as you mentioned.  This would solve the location we place them in, but still not sure what to do
> about versioned loops.  Would need to get its main edge "somewhere", would another field in
> loop_vinfo be ok?

I suppose since we're having ->scalar_loop adding ->scalar_loop_iv_exit
is straight-forward indeed.  As for matching them up I don't see how
you do that reliably right now?  It might be even that the if-converted
loop has one of the exits removed as unreachable (since we run VN
on its body) ...

What I could see working (but ick) is to extend the contract between
if-conversion and vectorization and for example record corresponding exit 
numbers in exits.  We have conveniently (*cough*) unused edge->aux
for this.  If you assign numbers to all edges of the original
loop the loop copies should inherit those (if I traced things
correctly - duplicate_block copies edge->aux but not bb->aux).

So in the vectorizer you could then match them up.

Richard.


> Cheers,
> Tamar
> 
> > > +  if (!loop->vec_loop_iv)
> > > +    return opt_result::failure_at (vect_location,
> > > +				   "not vectorized:"
> > > +				   " could not determine main exit from"
> > > +				   " loop with multiple exits.\n");
> > > +
> > >    /* Different restrictions apply when we are considering an inner-most loop,
> > >       vs. an outer (nested) loop.
> > >       (FORNOW. May want to relax some of these restrictions in the future).  */
> > > @@ -3025,9 +3032,8 @@ start_over:
> > >        if (dump_enabled_p ())
> > >          dump_printf_loc (MSG_NOTE, vect_location, "epilog loop required\n");
> > >        if (!vect_can_advance_ivs_p (loop_vinfo)
> > > -	  || !slpeel_can_duplicate_loop_p (LOOP_VINFO_LOOP (loop_vinfo),
> > > -					   single_exit (LOOP_VINFO_LOOP
> > > -							 (loop_vinfo))))
> > > +	  || !slpeel_can_duplicate_loop_p (loop_vinfo,
> > > +					   LOOP_VINFO_IV_EXIT (loop_vinfo)))
> > >          {
> > >  	  ok = opt_result::failure_at (vect_location,
> > >  				       "not vectorized: can't create required "
> > > @@ -5964,7 +5970,7 @@ vect_create_epilog_for_reduction (loop_vec_info
> > loop_vinfo,
> > >           Store them in NEW_PHIS.  */
> > >    if (double_reduc)
> > >      loop = outer_loop;
> > > -  exit_bb = single_exit (loop)->dest;
> > > +  exit_bb = LOOP_VINFO_IV_EXIT (loop_vinfo)->dest;
> > >    exit_gsi = gsi_after_labels (exit_bb);
> > >    reduc_inputs.create (slp_node ? vec_num : ncopies);
> > >    for (unsigned i = 0; i < vec_num; i++)
> > > @@ -5980,7 +5986,7 @@ vect_create_epilog_for_reduction (loop_vec_info
> > loop_vinfo,
> > >  	  phi = create_phi_node (new_def, exit_bb);
> > >  	  if (j)
> > >  	    def = gimple_get_lhs (STMT_VINFO_VEC_STMTS (rdef_info)[j]);
> > > -	  SET_PHI_ARG_DEF (phi, single_exit (loop)->dest_idx, def);
> > > +	  SET_PHI_ARG_DEF (phi, LOOP_VINFO_IV_EXIT (loop_vinfo)-
> > >dest_idx, def);
> > >  	  new_def = gimple_convert (&stmts, vectype, new_def);
> > >  	  reduc_inputs.quick_push (new_def);
> > >  	}
> > > @@ -10301,12 +10307,12 @@ vectorizable_live_operation (vec_info
> > *vinfo,
> > >  	   lhs' = new_tree;  */
> > >
> > >        class loop *loop = LOOP_VINFO_LOOP (loop_vinfo);
> > > -      basic_block exit_bb = single_exit (loop)->dest;
> > > +      basic_block exit_bb = LOOP_VINFO_IV_EXIT (loop_vinfo)->dest;
> > >        gcc_assert (single_pred_p (exit_bb));
> > >
> > >        tree vec_lhs_phi = copy_ssa_name (vec_lhs);
> > >        gimple *phi = create_phi_node (vec_lhs_phi, exit_bb);
> > > -      SET_PHI_ARG_DEF (phi, single_exit (loop)->dest_idx, vec_lhs);
> > > +      SET_PHI_ARG_DEF (phi, LOOP_VINFO_IV_EXIT (loop_vinfo)->dest_idx,
> > vec_lhs);
> > >
> > >        gimple_seq stmts = NULL;
> > >        tree new_tree;
> > > @@ -10829,7 +10835,8 @@ scale_profile_for_vect_loop (class loop *loop,
> > unsigned vf)
> > >        scale_loop_frequencies (loop, p);
> > >      }
> > >
> > > -  edge exit_e = single_exit (loop);
> > > +  edge exit_e = loop->vec_loop_iv;
> > > +
> > >    exit_e->probability = profile_probability::always () / (new_est_niter + 1);
> > >
> > >    edge exit_l = single_pred_edge (loop->latch);
> > > @@ -11177,7 +11184,7 @@ vect_transform_loop (loop_vec_info
> > loop_vinfo, gimple *loop_vectorized_call)
> > >
> > >    /* Make sure there exists a single-predecessor exit bb.  Do this before
> > >       versioning.   */
> > > -  edge e = single_exit (loop);
> > > +  edge e = LOOP_VINFO_IV_EXIT (loop_vinfo);
> > >    if (! single_pred_p (e->dest))
> > >      {
> > >        split_loop_exit_edge (e, true);
> > > diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
> > > index
> > a36974c2c0d2103b0a2d0397d06ab84dace08129..bd5eceb5da7a45ef036c
> > d14609ebe091799320bf 100644
> > > --- a/gcc/tree-vectorizer.h
> > > +++ b/gcc/tree-vectorizer.h
> > > @@ -917,6 +917,8 @@ public:
> > >
> > >  /* Access Functions.  */
> > >  #define LOOP_VINFO_LOOP(L)                 (L)->loop
> > > +#define LOOP_VINFO_IV_EXIT(L)              (L)->loop->vec_loop_iv
> > > +#define LOOP_VINFO_ALT_EXITS(L)            (L)->loop->vec_loop_alt_exits
> > >  #define LOOP_VINFO_BBS(L)                  (L)->bbs
> > >  #define LOOP_VINFO_NITERSM1(L)             (L)->num_itersm1
> > >  #define LOOP_VINFO_NITERS(L)               (L)->num_iters
> > > @@ -2162,6 +2164,7 @@ extern void vect_prepare_for_masked_peels
> > (loop_vec_info);
> > >  extern dump_user_location_t find_loop_location (class loop *);
> > >  extern bool vect_can_advance_ivs_p (loop_vec_info);
> > >  extern void vect_update_inits_of_drs (loop_vec_info, tree, tree_code);
> > > +extern void vec_init_exit_info (class loop *);
> > >
> > >  /* In tree-vect-stmts.cc.  */
> > >  extern tree get_related_vectype_for_scalar_type (machine_mode, tree,
> > 
> > So I didn't really see why we should need to have the info in
> > struct loop.
> > 
> > Richard.
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)