| Message ID | mptr1g9q8qa.fsf@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/10] vect: Simplify epilogue reduction code | expand |
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? > 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); > }
Richard Biener <richard.guenther@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? 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: ----------------------------------------------------------------------- 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); >> }
On Thu, Jul 8, 2021 at 6:48 PM Richard Sandiford <richard.sandiford@arm.com> wrote: > > Richard Biener <richard.guenther@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); > >> }
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); }