Hi Alfie,
Apologies for the very late reply.
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...
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"?
> --- 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?
> @@ -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?
> {
> /* 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 :)
> 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?
> +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.
--
Regards
Robin
