The 06/15/2026 18:07, Robin Dapp wrote:
> Hi Alfie,
>
> Apologies for the very late reply.
NP! Thanks for the review.
>
> I don't have major concerns, just bike-shedding. One bit of bike shedding is
> that I got confused with increment, step, and bump :) Not that I have a
> great
> idea how to unify or semantically match those, though...
Yes fair point.
I think "bump" is well defined as incrementing between data-refs
within a single iteration.
But "increment" and "step" are pretty mixed up. I think I should probably
change this
so "increment" only refers to the number of scalar iterations being handled, and
"step" refers to the value derived from "increment" to add to the IV?
>
> Some SELECT_VL comment, it won't change anything for this patch: IMHO our
> implementation currently is more conservative than it needs to be and in
> reality it behaves exactly like a MIN_EXPR for most uarchs. I have been
> pondering a "SELECT_VL_SANE" for a while but so far haven't convinced myself
> of
> the benefits.
>
> > void
> > create_iv (tree base, tree_code incr_op, tree step, tree var, class loop
> > *loop,
> > gimple_stmt_iterator *incr_pos, bool after, tree *var_before,
> > - tree *var_after)
> > + tree *var_after, bool loop_invariant_step)
>
> Extremely 'nit'y but just "invariant_step"?
Sure
>
> > --- a/gcc/tree-ssa-loop-manip.h
> > +++ b/gcc/tree-ssa-loop-manip.h
> > @@ -24,7 +24,8 @@ typedef void (*transform_callback)(class loop *, void *);
>
> > + gimple_stmt_iterator *, bool, tree *, tree *,
> > + bool = true);
> > extern void rewrite_into_loop_closed_ssa (bitmap, unsigned);
> > + Increment a pointer (to a vector type) by update.
>
> Maybe "Increment DATAREF_PTR by UPDATE"?
>
> > @@ -6062,13 +6045,11 @@ vect_create_data_ref_ptr (vec_info *vinfo,
> > stmt_vec_info stmt_info,
> >
> > tree
> > bump_vector_ptr (vec_info *vinfo,
> > - tree dataref_ptr, gimple *ptr_incr, gimple_stmt_iterator *gsi,
> > + tree dataref_ptr, gimple_stmt_iterator *gsi,
> > stmt_vec_info stmt_info, tree update)
> > {
> > struct data_reference *dr = STMT_VINFO_DATA_REF (stmt_info);
> > gimple *incr_stmt;
> > - ssa_op_iter iter;
> > - use_operand_p use_p;
> > tree new_dataref_ptr;
> >
> > if (TREE_CODE (dataref_ptr) == SSA_NAME)
> > @@ -6099,20 +6080,6 @@ bump_vector_ptr (vec_info *vinfo,
> > /* Copy the points-to information if it exists. */
> > duplicate_ssa_name_ptr_info (new_dataref_ptr, DR_PTR_INFO (dr));
> >
> > - if (!ptr_incr)
> > - return new_dataref_ptr;
> > -
> > - /* Update the vector-pointer's cross-iteration increment. */
> > - FOR_EACH_SSA_USE_OPERAND (use_p, ptr_incr, iter, SSA_OP_USE)
> > - {
> > - tree use = USE_FROM_PTR (use_p);
> > -
> > - if (use == dataref_ptr)
> > - SET_USE (use_p, new_dataref_ptr);
> > - else
> > - gcc_assert (operand_equal_p (use, update, 0));
> > - }
> > -
> > return new_dataref_ptr;
> > }
> >
> > diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc
> > index d2c854e16d4..722742fd046 100644
> > --- a/gcc/tree-vect-loop-manip.cc
> > +++ b/gcc/tree-vect-loop-manip.cc
> > @@ -514,7 +514,6 @@ vect_set_loop_controls_directly (class loop *loop,
> > loop_vec_info loop_vinfo,
> > tree ctrl_type = rgc->type;
> > unsigned int nitems_per_iter = rgc->max_nscalars_per_iter * rgc->factor;
> > poly_uint64 nitems_per_ctrl = TYPE_VECTOR_SUBPARTS (ctrl_type) *
> > rgc->factor;
> > - poly_uint64 vf = LOOP_VINFO_VECT_FACTOR (loop_vinfo);
> > tree length_limit = NULL_TREE;
> > /* For length, we need length_limit to ensure length in range. */
> > if (!use_masks_p)
> > @@ -525,7 +524,15 @@ vect_set_loop_controls_directly (class loop *loop,
> > loop_vec_info loop_vinfo,
> > of the vector loop, and the number that it should skip during the
> > first iteration of the vector loop. */
> > tree nitems_total = niters;
> > - tree nitems_step = build_int_cst (iv_type, vf);
> > + tree nitems_vf
> > + = build_int_cst (iv_type, LOOP_VINFO_VECT_FACTOR (loop_vinfo));
> > + tree nitems_step
> > + = LOOP_VINFO_IV_INCREMENT_INVARIANT_P (loop_vinfo)
> > + ? gimple_convert (preheader_seq, iv_type,
> > + LOOP_VINFO_IV_INCREMENT (loop_vinfo))
> > + : gimple_convert (&loop_cond_gsi, true, GSI_SAME_STMT, UNKNOWN_LOCATION,
> > + iv_type, LOOP_VINFO_IV_INCREMENT (loop_vinfo));
>
> > + nitems_vf = gimple_build (preheader_seq, MULT_EXPR, iv_type,
> > + nitems_vf, iv_factor);
> > + nitems_step = LOOP_VINFO_IV_INCREMENT_INVARIANT_P (loop_vinfo)
> > + ? gimple_build (preheader_seq, MULT_EXPR, iv_type,
> > + nitems_step, iv_factor)
> > + : gimple_build (&loop_cond_gsi, true, GSI_SAME_STMT,
> > + UNKNOWN_LOCATION, MULT_EXPR, iv_type,
> > + nitems_step, iv_factor);
> > if (nitems_skip)
> > nitems_skip = gimple_build (preheader_seq, MULT_EXPR, compare_type,
> > nitems_skip, compare_factor);
> > @@ -578,9 +591,14 @@ vect_set_loop_controls_directly (class loop *loop,
> > loop_vec_info loop_vinfo,
> > insert_after, &index_before_incr, &index_after_incr);
> > tree vectype = build_zero_cst (rgc->type);
> > tree len = gimple_build (header_seq, IFN_SELECT_VL, iv_type,
> > - index_before_incr, nitems_step,
> > + index_before_incr, nitems_vf,
>
> Do we need the nitems_step gimple_convert above when we switch to nitems_vf
> for SELECT_VL?
I need it for the upcoming FFR patch where the step is set elsewhere.
>
> > @@ -2979,7 +3003,8 @@ vect_gen_vector_loop_niters (loop_vec_info
> > loop_vinfo,
> > tree niters,
> > /* To silence some unexpected warnings, simply initialize to 0. */
> > unsigned HOST_WIDE_INT const_vf = 0;
> > if (vf.is_constant (&const_vf)
> > - && !LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo))
> > + && !LOOP_VINFO_USING_PARTIAL_VECTORS_P (loop_vinfo)
> > + && is_gimple_constant (LOOP_VINFO_IV_INCREMENT (loop_vinfo)))
>
> Is this going to become LOOP_VINFO_IV_INCREMENT_INVARIANT_P later?
Ah I missed this (I previously tried using is_gimple_constant instead of
the LOOP_VINFO_IV_INCREMENT_INVARIANT_P). Thanks
>
> > {
> > /* Create: niters / vf, which is equivalent to niters >> log2(vf)
> > when
> > vf is a power of two, and when not we approximate using a
> > @@ -3054,6 +3079,23 @@ vect_gen_vector_loop_niters (loop_vec_info
> > loop_vinfo, tree niters,
> > return;
> > }
>
> > + /* Fill this in later when building the loop controls. */
> > + tree iv_increment = make_temp_ssa_name (sizetype, NULL,
> > "iv_increment");
> > + SSA_NAME_DEF_STMT (iv_increment) = gimple_build_nop ();
>
> I was about to comment but then realized that you mentioned IFN_VARYING in
> the
> cover letter already :)
>
> > + /* Write the adjustments at the end of the iv increment. */
> > + bool insert_after;
> > + gimple_stmt_iterator incr_gsi;
> > + vect_iv_increment_position (LOOP_VINFO_MAIN_EXIT (loop_vinfo), &incr_gsi,
> > + &insert_after);
> > +
> > + gsi_insert_seq_before (&incr_gsi, stmts, GSI_SAME_STMT);
>
> I suppose this is preexisting but inserting before regardless of insert_after
> seems slightly odd :)
Will fix!
>
> > static tree
> > -vect_get_loop_variant_data_ptr_increment (
> > - vec_info *vinfo, tree aggr_type, gimple_stmt_iterator *gsi,
> > - vec_loop_lens *loop_lens, dr_vec_info *dr_info,
> > - vect_memory_access_type memory_access_type)
> > +vect_get_data_ptr_increment (vec_info *vinfo,
> > + dr_vec_info *dr_info, tree aggr_type,
> > + vect_memory_access_type memory_access_type)
> > {
> > + if (memory_access_type == VMAT_INVARIANT)
> > + return size_zero_node;
> > +
> > loop_vec_info loop_vinfo = dyn_cast<loop_vec_info> (vinfo);
> > - tree step = vect_dr_behavior (vinfo, dr_info)->step;
> > + if (!loop_vinfo)
> > + {
> > + /* For slp there is no next iteration? */
> > + tree iv_step = TYPE_SIZE_UNIT (aggr_type);
> > + return build_zero_cst (TREE_TYPE (iv_step));
>
> Does size_zero_node not work here?
Wasn't aware that existed, will change
>
> > +vect_get_data_ptr_bump (vec_info *vinfo,
> > + dr_vec_info *dr_info, tree aggr_type,
> > + vect_memory_access_type memory_access_type)
> > {
> > if (memory_access_type == VMAT_INVARIANT)
> > return size_zero_node;
> >
> > loop_vec_info loop_vinfo = dyn_cast<loop_vec_info> (vinfo);
> > + /* We do not support SLP loads where num_vec != 1 with SELECT_VL so this
> > value
> > + should never be needed. */
>
> I'm fairly certain we could support those, we just haven't bothered so far.
>
The same is not true for FFR, multiple lane loads are going to be very
complicated
and I kind of doubt they would ever be profitable, so this may need some careful
handling in the future.
> --
> Regards
> Robin
>
--
Alfie Richards
