On Thu, Jul 8, 2021 at 6:48 PM Richard Sandiford
<richard.sandif...@arm.com> wrote:
>
> Richard Biener <richard.guent...@gmail.com> writes:
> > On Thu, Jul 8, 2021 at 2:46 PM Richard Sandiford via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> This patch passes the reduc_info to get_initial_defs_for_reduction,
> >> so that the function can get general information from there rather
> >> than from the first SLP statement. This isn't a win on its own,
> >> but it becomes important with later patches.
> >
> > So the original code should have used SLP_TREE_REPRESENTATIVE
> > instead of SLP_TREE_SCALAR_STMTS ()[0] (there might have been
> > issues with doing that - my recollection is weak here).
> >
> > I'm not sure if reduc_info is actually better - only the representative
> > will have STMT_VINFO_VECTYPE set, for the reduc_info
> > there's STMT_VINFO_REDUC_VECTYPE (and STMT_VINFO_REDUC_VECTYPE_IN).
> >
> > So I think if you want to use reduc_info then you want to use
> > STMT_VINFO_REDUC_VECTYPE?
>
> I guess I'm a bit fuzzy on the details, but AIUI STMT_VINFO_REDUC_VECTYPE
> is the type that we do the arithmetic in, which might be different from
> the types of the phis. Is that right?
Hmm, yeah (my recollection is fuzzy as well here...).
> In this context we want the types of the phis, since the routine is
> providing the initial values. Using STMT_VINFO_REDUC_VECTYPE gives
> things like:
OK, I see. So there's the reduc_info vs. SLP_TREE_REPRESENTATIVE issue
left. At least I don't see that we reliably set STMT_VINFO_VECTYPE on
all scalar PHIs of a SLP reduction. The reduc_info happens to be one of the
PHI stmt_infos (but that's an implementation detail as well).
The reduction SLP instance has the reduc_phis member to get at the
PHIs vector type (via SLP_TREE_VECTYPE). I think we don't have
anything explicit that's good here but I notice that
vect_create_epilog_for_reduction
uses STMT_VINFO_VECTYPE (reduc_info) as well.
So I guess the patch is OK as-is.
Thanks,
Richard.
> -----------------------------------------------------------------------
> gcc.dg/torture/pr92345.c:8:1: error: incompatible types in 'PHI' argument 1
> vector(4) int
>
> vector(4) unsigned int
>
> vect_fr_lsm.11_58 = PHI <vect__7.14_64(6), { 0, 0, 0, 0 }(10)>
> -----------------------------------------------------------------------
>
> Thanks,
> Richard
>
> >
> >> gcc/
> >> * tree-vect-loop.c (get_initial_defs_for_reduction): Take the
> >> reduc_info as an additional parameter.
> >> (vect_transform_cycle_phi): Update accordingly.
> >> ---
> >> gcc/tree-vect-loop.c | 23 ++++++++++-------------
> >> 1 file changed, 10 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
> >> index a31d7621c3b..565c2859477 100644
> >> --- a/gcc/tree-vect-loop.c
> >> +++ b/gcc/tree-vect-loop.c
> >> @@ -4764,32 +4764,28 @@ get_initial_def_for_reduction (loop_vec_info
> >> loop_vinfo,
> >> return init_def;
> >> }
> >>
> >> -/* Get at the initial defs for the reduction PHIs in SLP_NODE.
> >> - NUMBER_OF_VECTORS is the number of vector defs to create.
> >> - If NEUTRAL_OP is nonnull, introducing extra elements of that
> >> - value will not change the result. */
> >> +/* Get at the initial defs for the reduction PHIs for REDUC_INFO, whose
> >> + associated SLP node is SLP_NODE. NUMBER_OF_VECTORS is the number of
> >> vector
> >> + defs to create. If NEUTRAL_OP is nonnull, introducing extra elements
> >> of
> >> + that value will not change the result. */
> >>
> >> static void
> >> get_initial_defs_for_reduction (vec_info *vinfo,
> >> + stmt_vec_info reduc_info,
> >> slp_tree slp_node,
> >> vec<tree> *vec_oprnds,
> >> unsigned int number_of_vectors,
> >> bool reduc_chain, tree neutral_op)
> >> {
> >> vec<stmt_vec_info> stmts = SLP_TREE_SCALAR_STMTS (slp_node);
> >> - stmt_vec_info stmt_vinfo = stmts[0];
> >> unsigned HOST_WIDE_INT nunits;
> >> unsigned j, number_of_places_left_in_vector;
> >> - tree vector_type;
> >> + tree vector_type = STMT_VINFO_VECTYPE (reduc_info);
> >> unsigned int group_size = stmts.length ();
> >> unsigned int i;
> >> class loop *loop;
> >>
> >> - vector_type = STMT_VINFO_VECTYPE (stmt_vinfo);
> >> -
> >> - gcc_assert (STMT_VINFO_DEF_TYPE (stmt_vinfo) == vect_reduction_def);
> >> -
> >> - loop = (gimple_bb (stmt_vinfo->stmt))->loop_father;
> >> + loop = (gimple_bb (reduc_info->stmt))->loop_father;
> >> gcc_assert (loop);
> >> edge pe = loop_preheader_edge (loop);
> >>
> >> @@ -4823,7 +4819,7 @@ get_initial_defs_for_reduction (vec_info *vinfo,
> >> {
> >> tree op;
> >> i = j % group_size;
> >> - stmt_vinfo = stmts[i];
> >> + stmt_vec_info stmt_vinfo = stmts[i];
> >>
> >> /* Get the def before the loop. In reduction chain we have only
> >> one initial value. Else we have as many as PHIs in the group. */
> >> @@ -7510,7 +7506,8 @@ vect_transform_cycle_phi (loop_vec_info loop_vinfo,
> >> = neutral_op_for_slp_reduction (slp_node, vectype_out,
> >> STMT_VINFO_REDUC_CODE
> >> (reduc_info),
> >> first != NULL);
> >> - get_initial_defs_for_reduction (loop_vinfo,
> >> slp_node_instance->reduc_phis,
> >> + get_initial_defs_for_reduction (loop_vinfo, reduc_info,
> >> + slp_node_instance->reduc_phis,
> >> &vec_initial_defs, vec_num,
> >> first != NULL, neutral_op);
> >> }
