> -----Original Message-----
> From: Richard Biener <rguent...@suse.de>
> Sent: Tuesday, November 7, 2023 3:04 PM
> To: Tamar Christina <tamar.christ...@arm.com>
> Cc: gcc-patches@gcc.gnu.org; nd <n...@arm.com>; j...@ventanamicro.com
> Subject: Re: [PATCH 5/21]middle-end: update vectorizer's control update to
> support picking an exit other than loop latch
> 
> On Mon, 6 Nov 2023, Tamar Christina wrote:
> 
> > Hi All,
> >
> > As requested, the vectorizer is now free to pick it's own exit which
> > can be different than what the loop CFG infrastucture uses.  The
> > vectorizer makes use of this to vectorize loops that it previously could 
> > not.
> >
> > But this means that loop control must be materialized in the block
> > that needs it less we corrupt the SSA chain.  This makes it so we use
> > the vectorizer's main IV block instead of the loop infra.
> >
> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> >
> > Ok for master?
> >
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> >     * tree-ssa-loop-manip.cc (standard_iv_increment_position):
> Conditionally
> >     take dest BB.
> >     * tree-ssa-loop-manip.h (standard_iv_increment_position): Likewise.
> >     * tree-vect-loop-manip.cc (vect_set_loop_controls_directly): Use it.
> >     (vect_set_loop_condition_partial_vectors_avx512): Likewise.
> >     (vect_set_loop_condition_normal): Likewise.
> >
> > --- inline copy of patch --
> > diff --git a/gcc/tree-ssa-loop-manip.h b/gcc/tree-ssa-loop-manip.h
> > index
> >
> bda09f51d5619420331c513a9906831c779fd2b4..5938588c8882d842b00
> 301423df1
> > 11cbe7bf7ba8 100644
> > --- a/gcc/tree-ssa-loop-manip.h
> > +++ b/gcc/tree-ssa-loop-manip.h
> > @@ -38,7 +38,8 @@ extern basic_block split_loop_exit_edge (edge, bool
> > = false);  extern basic_block ip_end_pos (class loop *);  extern
> > basic_block ip_normal_pos (class loop *);  extern void
> > standard_iv_increment_position (class loop *,
> > -                                       gimple_stmt_iterator *, bool *);
> > +                                       gimple_stmt_iterator *, bool *,
> > +                                       basic_block = NULL);
> >  extern bool
> >  gimple_duplicate_loop_body_to_header_edge (class loop *, edge, unsigned
> int,
> >                                        sbitmap, edge, vec<edge> *, int); 
> > diff
> --git
> > a/gcc/tree-ssa-loop-manip.cc b/gcc/tree-ssa-loop-manip.cc index
> >
> e7436915e01297e7af2a3bcf1afd01e014de6f32..bdc7a3d74a788f450ca5d
> de6c294
> > 92ce4d4e4550 100644
> > --- a/gcc/tree-ssa-loop-manip.cc
> > +++ b/gcc/tree-ssa-loop-manip.cc
> > @@ -792,14 +792,19 @@ ip_normal_pos (class loop *loop)
> >
> >  /* Stores the standard position for induction variable increment in LOOP
> >     (just before the exit condition if it is available and latch block is 
> > empty,
> > -   end of the latch block otherwise) to BSI.  INSERT_AFTER is set to true 
> > if
> > -   the increment should be inserted after *BSI.  */
> > +   end of the latch block otherwise) to BSI.  If DEST_BB is specified then 
> > that
> > +   basic block is used as the destination instead of the loop latch source
> > +   block.  INSERT_AFTER is set to true if the increment should be inserted
> after
> > +   *BSI.  */
> >
> >  void
> >  standard_iv_increment_position (class loop *loop, gimple_stmt_iterator
> *bsi,
> > -                           bool *insert_after)
> > +                           bool *insert_after, basic_block dest_bb)
> >  {
> > -  basic_block bb = ip_normal_pos (loop), latch = ip_end_pos (loop);
> > +  basic_block bb = dest_bb;
> > +  if (!bb)
> > +    bb = ip_normal_pos (loop);
> > +  basic_block latch = ip_end_pos (loop);
> 
> I don't think that's a good API extension.  Given that we don't support an 
> early
> exit after the main IV exit doesn't this code already work fine as-is?  It 
> chooses
> the last exit.  The position is also not semantically relevant, we just try 
> to keep
> the latch empty here (that is, it's a bit of a "bad" API).
> 
> So, do you really need this change?

I'll double check. I remember needing it to fix an ICE before, but also re-did 
the
way the alternative main exits were handled later.  At the end of the series as 
I
was writing the cover letter this change also seemed... off to me, I should have
checked it again before submitting it.

> 
> Maybe we're really using standard_iv_increment_position wrong here, the
> result is supposed to _only_ feed the PHI latch argument.

Could be, this was needed before I changed the way I handled the IV updates for
the alternate exit loops.  I'll double check and drop If not needed.

Thanks,
Tamar

> 
> Richard.
> 
> >    gimple *last = last_nondebug_stmt (latch);
> >
> >    if (!bb
> > diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc
> > index
> >
> 6fbb5b80986fd657814b48eb009b52b094f331e6..3d59119787d6afdc5a64
> 65a547d1
> > ea2d3d940373 100644
> > --- a/gcc/tree-vect-loop-manip.cc
> > +++ b/gcc/tree-vect-loop-manip.cc
> > @@ -531,7 +531,8 @@ vect_set_loop_controls_directly (class loop *loop,
> loop_vec_info loop_vinfo,
> >    tree index_before_incr, index_after_incr;
> >    gimple_stmt_iterator incr_gsi;
> >    bool insert_after;
> > -  standard_iv_increment_position (loop, &incr_gsi, &insert_after);
> > +  edge exit_e = LOOP_VINFO_IV_EXIT (loop_vinfo);
> > + standard_iv_increment_position (loop, &incr_gsi, &insert_after,
> > + exit_e->src);
> >    if (LOOP_VINFO_USING_DECREMENTING_IV_P (loop_vinfo))
> >      {
> >        /* Create an IV that counts down from niters_total and whose
> > step @@ -1017,7 +1018,8 @@
> vect_set_loop_condition_partial_vectors_avx512 (class loop *loop,
> >    tree index_before_incr, index_after_incr;
> >    gimple_stmt_iterator incr_gsi;
> >    bool insert_after;
> > -  standard_iv_increment_position (loop, &incr_gsi, &insert_after);
> > +  standard_iv_increment_position (loop, &incr_gsi, &insert_after,
> > +                             exit_edge->src);
> >    create_iv (niters_adj, MINUS_EXPR, iv_step, NULL_TREE, loop,
> >          &incr_gsi, insert_after, &index_before_incr,
> >          &index_after_incr);
> > @@ -1185,7 +1187,7 @@ vect_set_loop_condition_partial_vectors_avx512
> (class loop *loop,
> >     loop handles exactly VF scalars per iteration.  */
> >
> >  static gcond *
> > -vect_set_loop_condition_normal (loop_vec_info /* loop_vinfo */, edge
> > exit_edge,
> > +vect_set_loop_condition_normal (loop_vec_info loop_vinfo, edge
> > +exit_edge,
> >                             class loop *loop, tree niters, tree step,
> >                             tree final_iv, bool niters_maybe_zero,
> >                             gimple_stmt_iterator loop_cond_gsi) @@ -
> 1278,7 +1280,8 @@
> > vect_set_loop_condition_normal (loop_vec_info /* loop_vinfo */, edge
> exit_edge,
> >     }
> >      }
> >
> > -  standard_iv_increment_position (loop, &incr_gsi, &insert_after);
> > +  standard_iv_increment_position (loop, &incr_gsi, &insert_after,
> > +                             exit_edge->src);
> >    create_iv (init, PLUS_EXPR, step, NULL_TREE, loop,
> >               &incr_gsi, insert_after, &indx_before_incr, &indx_after_incr);
> >    indx_after_incr = force_gimple_operand_gsi (&loop_cond_gsi,
> > indx_after_incr,
> >
> >
> >
> >
> >
> 
> --
> Richard Biener <rguent...@suse.de>
> SUSE Software Solutions Germany GmbH,
> Frankenstrasse 146, 90461 Nuernberg, Germany;
> GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG
> Nuernberg)

Reply via email to