[05/10] vect: Add a vect_phi_initial_value helper function

This patch adds a helper function called vect_phi_initial_value
for returning the incoming value of a given loop phi.  The main
reason for adding it is to ensure that the right preheader edge
is used when vectorising nested loops.  (PHI_ARG_DEF_FROM_EDGE
itself doesn't assert that the given edge is for the right block,
although I guess that would be good to add separately.)

gcc/
	* tree-vectorizer.h: Include tree-ssa-operands.h.
	(vect_phi_initial_value): New function.
	* tree-vect-loop.c (neutral_op_for_slp_reduction): Use it.
	(get_initial_defs_for_reduction, info_for_reduction): Likewise.
	(vect_create_epilog_for_reduction, vectorizable_reduction): Likewise.
	(vect_transform_cycle_phi, vectorizable_induction): Likewise.
---
 gcc/tree-vect-loop.c  | 29 +++++++++--------------------
 gcc/tree-vectorizer.h | 21 ++++++++++++++++++++-
 2 files changed, 29 insertions(+), 21 deletions(-)

On Thu, Jul 8, 2021 at 2:45 PM Richard Sandiford via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> This patch adds a helper function called vect_phi_initial_value
> for returning the incoming value of a given loop phi.  The main
> reason for adding it is to ensure that the right preheader edge
> is used when vectorising nested loops.  (PHI_ARG_DEF_FROM_EDGE
> itself doesn't assert that the given edge is for the right block,
> although I guess that would be good to add separately.)

We were sometimes (most of the time?) using an explicit
loop where you now get it from the PHI - that makes the
assert somewhat pointless to some extent - of course it
makes sense on its own that the loop is the same as that
of the PHI def.  I just wonder if you think any of the existing
code might have been wrong?  If so the new assert doesn't
catch all originally wrong cases.

Otherwise OK,
Richard.

> gcc/
>         * tree-vectorizer.h: Include tree-ssa-operands.h.
>         (vect_phi_initial_value): New function.
>         * tree-vect-loop.c (neutral_op_for_slp_reduction): Use it.
>         (get_initial_defs_for_reduction, info_for_reduction): Likewise.
>         (vect_create_epilog_for_reduction, vectorizable_reduction): Likewise.
>         (vect_transform_cycle_phi, vectorizable_induction): Likewise.
> ---
>  gcc/tree-vect-loop.c  | 29 +++++++++--------------------
>  gcc/tree-vectorizer.h | 21 ++++++++++++++++++++-
>  2 files changed, 29 insertions(+), 21 deletions(-)
>
> diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
> index 1bd9a6ea52c..a31d7621c3b 100644
> --- a/gcc/tree-vect-loop.c
> +++ b/gcc/tree-vect-loop.c
> @@ -3288,8 +3288,7 @@ neutral_op_for_slp_reduction (slp_tree slp_node, tree vector_type,
>          has only a single initial value, so that value is neutral for
>          all statements.  */
>        if (reduc_chain)
> -       return PHI_ARG_DEF_FROM_EDGE (stmt_vinfo->stmt,
> -                                     loop_preheader_edge (loop));
> +       return vect_phi_initial_value (stmt_vinfo);
>        return NULL_TREE;
>
>      default:
> @@ -4829,13 +4828,13 @@ get_initial_defs_for_reduction (vec_info *vinfo,
>        /* 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.  */
>        if (reduc_chain)
> -       op = j != 0 ? neutral_op : PHI_ARG_DEF_FROM_EDGE (stmt_vinfo->stmt, pe);
> +       op = j != 0 ? neutral_op : vect_phi_initial_value (stmt_vinfo);
>        else if (((vec_oprnds->length () + 1) * nunits
>                 - number_of_places_left_in_vector >= group_size)
>                && neutral_op)
>         op = neutral_op;
>        else
> -       op = PHI_ARG_DEF_FROM_EDGE (stmt_vinfo->stmt, pe);
> +       op = vect_phi_initial_value (stmt_vinfo);
>
>        /* Create 'vect_ = {op0,op1,...,opn}'.  */
>        number_of_places_left_in_vector--;
> @@ -4906,9 +4905,7 @@ info_for_reduction (vec_info *vinfo, stmt_vec_info stmt_info)
>      }
>    else if (STMT_VINFO_DEF_TYPE (stmt_info) == vect_nested_cycle)
>      {
> -      edge pe = loop_preheader_edge (gimple_bb (phi)->loop_father);
> -      stmt_vec_info info
> -         = vinfo->lookup_def (PHI_ARG_DEF_FROM_EDGE (phi, pe));
> +      stmt_vec_info info = vinfo->lookup_def (vect_phi_initial_value (phi));
>        if (info && STMT_VINFO_DEF_TYPE (info) == vect_double_reduction_def)
>         stmt_info = info;
>      }
> @@ -5042,8 +5039,7 @@ vect_create_epilog_for_reduction (loop_vec_info loop_vinfo,
>      {
>        /* Get at the scalar def before the loop, that defines the initial value
>          of the reduction variable.  */
> -      initial_def = PHI_ARG_DEF_FROM_EDGE (reduc_def_stmt,
> -                                          loop_preheader_edge (loop));
> +      initial_def = vect_phi_initial_value (reduc_def_stmt);
>        /* Optimize: for induction condition reduction, if we can't use zero
>           for induc_val, use initial_def.  */
>        if (STMT_VINFO_REDUC_TYPE (reduc_info) == INTEGER_INDUC_COND_REDUCTION)
> @@ -5558,9 +5554,7 @@ vect_create_epilog_for_reduction (loop_vec_info loop_vinfo,
>              for MIN and MAX reduction, for example.  */
>           if (!neutral_op)
>             {
> -             tree scalar_value
> -               = PHI_ARG_DEF_FROM_EDGE (orig_phis[i]->stmt,
> -                                        loop_preheader_edge (loop));
> +             tree scalar_value = vect_phi_initial_value (orig_phis[i]);
>               scalar_value = gimple_convert (&seq, TREE_TYPE (vectype),
>                                              scalar_value);
>               vector_identity = gimple_build_vector_from_val (&seq, vectype,
> @@ -6752,10 +6746,7 @@ vectorizable_reduction (loop_vec_info loop_vinfo,
>        else if (cond_reduc_dt == vect_constant_def)
>         {
>           enum vect_def_type cond_initial_dt;
> -         tree cond_initial_val
> -           = PHI_ARG_DEF_FROM_EDGE (reduc_def_phi, loop_preheader_edge (loop));
> -
> -         gcc_assert (cond_reduc_val != NULL_TREE);
> +         tree cond_initial_val = vect_phi_initial_value (reduc_def_phi);
>           vect_is_simple_use (cond_initial_val, loop_vinfo, &cond_initial_dt);
>           if (cond_initial_dt == vect_constant_def
>               && types_compatible_p (TREE_TYPE (cond_initial_val),
> @@ -7528,8 +7519,7 @@ vect_transform_cycle_phi (loop_vec_info loop_vinfo,
>      {
>        /* Get at the scalar def before the loop, that defines the initial
>          value of the reduction variable.  */
> -      tree initial_def = PHI_ARG_DEF_FROM_EDGE (phi,
> -                                               loop_preheader_edge (loop));
> +      tree initial_def = vect_phi_initial_value (phi);
>        /* Optimize: if initial_def is for REDUC_MAX smaller than the base
>          and we can't use zero for induc_val, use initial_def.  Similarly
>          for REDUC_MIN and initial_def larger than the base.  */
> @@ -8175,8 +8165,7 @@ vectorizable_induction (loop_vec_info loop_vinfo,
>        return true;
>      }
>
> -  init_expr = PHI_ARG_DEF_FROM_EDGE (phi,
> -                                    loop_preheader_edge (iv_loop));
> +  init_expr = vect_phi_initial_value (phi);
>
>    gimple_seq stmts = NULL;
>    if (!nested_in_vect_loop)
> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
> index fa28336d429..e2fd3609fee 100644
> --- a/gcc/tree-vectorizer.h
> +++ b/gcc/tree-vectorizer.h
> @@ -27,7 +27,7 @@ typedef class _stmt_vec_info *stmt_vec_info;
>  #include "tree-hash-traits.h"
>  #include "target.h"
>  #include "internal-fn.h"
> -
> +#include "tree-ssa-operands.h"
>
>  /* Used for naming of new temporaries.  */
>  enum vect_var_kind {
> @@ -1369,6 +1369,25 @@ nested_in_vect_loop_p (class loop *loop, stmt_vec_info stmt_info)
>           && (loop->inner == (gimple_bb (stmt_info->stmt))->loop_father));
>  }
>
> +/* PHI is either a scalar reduction phi or a scalar induction phi.
> +   Return the initial value of the variable on entry to the containing
> +   loop.  */
> +
> +static inline tree
> +vect_phi_initial_value (gphi *phi)
> +{
> +  basic_block bb = gimple_bb (phi);
> +  edge pe = loop_preheader_edge (bb->loop_father);
> +  gcc_assert (pe->dest == bb);
> +  return PHI_ARG_DEF_FROM_EDGE (phi, pe);
> +}
> +
> +static inline tree
> +vect_phi_initial_value (stmt_vec_info stmt_info)
> +{
> +  return vect_phi_initial_value (as_a <gphi *> (stmt_info->stmt));
> +}
> +
>  /* Return true if STMT_INFO should produce a vector mask type rather than
>     a normal nonmask type.  */
>

Richard Biener <richard.guenther@gmail.com> writes:
> On Thu, Jul 8, 2021 at 2:45 PM Richard Sandiford via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> This patch adds a helper function called vect_phi_initial_value
>> for returning the incoming value of a given loop phi.  The main
>> reason for adding it is to ensure that the right preheader edge
>> is used when vectorising nested loops.  (PHI_ARG_DEF_FROM_EDGE
>> itself doesn't assert that the given edge is for the right block,
>> although I guess that would be good to add separately.)
>
> We were sometimes (most of the time?) using an explicit
> loop where you now get it from the PHI - that makes the
> assert somewhat pointless to some extent - of course it
> makes sense on its own that the loop is the same as that
> of the PHI def.  I just wonder if you think any of the existing
> code might have been wrong?  If so the new assert doesn't
> catch all originally wrong cases.

I don't remember seeing a case where the existing code got it wrong,
but I think one of the patches in the series did initially use the
wrong loop's preheader.

But yeah, the function and assert only help to avoid using
PHI_ARG_DEF_FROM_EDGE with the wrong edge.  If the problem was instead
passing the wrong phi then the patch doesn't help to catch that.

The edge mistake is more likely to be a silent failure though,
since the edge indices for both loops might happen to be the same
(but might not).

Thanks,
Richard

>
> Otherwise OK,
> Richard.
>
>> gcc/
>>         * tree-vectorizer.h: Include tree-ssa-operands.h.
>>         (vect_phi_initial_value): New function.
>>         * tree-vect-loop.c (neutral_op_for_slp_reduction): Use it.
>>         (get_initial_defs_for_reduction, info_for_reduction): Likewise.
>>         (vect_create_epilog_for_reduction, vectorizable_reduction): Likewise.
>>         (vect_transform_cycle_phi, vectorizable_induction): Likewise.
>> ---
>>  gcc/tree-vect-loop.c  | 29 +++++++++--------------------
>>  gcc/tree-vectorizer.h | 21 ++++++++++++++++++++-
>>  2 files changed, 29 insertions(+), 21 deletions(-)
>>
>> diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c
>> index 1bd9a6ea52c..a31d7621c3b 100644
>> --- a/gcc/tree-vect-loop.c
>> +++ b/gcc/tree-vect-loop.c
>> @@ -3288,8 +3288,7 @@ neutral_op_for_slp_reduction (slp_tree slp_node, tree vector_type,
>>          has only a single initial value, so that value is neutral for
>>          all statements.  */
>>        if (reduc_chain)
>> -       return PHI_ARG_DEF_FROM_EDGE (stmt_vinfo->stmt,
>> -                                     loop_preheader_edge (loop));
>> +       return vect_phi_initial_value (stmt_vinfo);
>>        return NULL_TREE;
>>
>>      default:
>> @@ -4829,13 +4828,13 @@ get_initial_defs_for_reduction (vec_info *vinfo,
>>        /* 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.  */
>>        if (reduc_chain)
>> -       op = j != 0 ? neutral_op : PHI_ARG_DEF_FROM_EDGE (stmt_vinfo->stmt, pe);
>> +       op = j != 0 ? neutral_op : vect_phi_initial_value (stmt_vinfo);
>>        else if (((vec_oprnds->length () + 1) * nunits
>>                 - number_of_places_left_in_vector >= group_size)
>>                && neutral_op)
>>         op = neutral_op;
>>        else
>> -       op = PHI_ARG_DEF_FROM_EDGE (stmt_vinfo->stmt, pe);
>> +       op = vect_phi_initial_value (stmt_vinfo);
>>
>>        /* Create 'vect_ = {op0,op1,...,opn}'.  */
>>        number_of_places_left_in_vector--;
>> @@ -4906,9 +4905,7 @@ info_for_reduction (vec_info *vinfo, stmt_vec_info stmt_info)
>>      }
>>    else if (STMT_VINFO_DEF_TYPE (stmt_info) == vect_nested_cycle)
>>      {
>> -      edge pe = loop_preheader_edge (gimple_bb (phi)->loop_father);
>> -      stmt_vec_info info
>> -         = vinfo->lookup_def (PHI_ARG_DEF_FROM_EDGE (phi, pe));
>> +      stmt_vec_info info = vinfo->lookup_def (vect_phi_initial_value (phi));
>>        if (info && STMT_VINFO_DEF_TYPE (info) == vect_double_reduction_def)
>>         stmt_info = info;
>>      }
>> @@ -5042,8 +5039,7 @@ vect_create_epilog_for_reduction (loop_vec_info loop_vinfo,
>>      {
>>        /* Get at the scalar def before the loop, that defines the initial value
>>          of the reduction variable.  */
>> -      initial_def = PHI_ARG_DEF_FROM_EDGE (reduc_def_stmt,
>> -                                          loop_preheader_edge (loop));
>> +      initial_def = vect_phi_initial_value (reduc_def_stmt);
>>        /* Optimize: for induction condition reduction, if we can't use zero
>>           for induc_val, use initial_def.  */
>>        if (STMT_VINFO_REDUC_TYPE (reduc_info) == INTEGER_INDUC_COND_REDUCTION)
>> @@ -5558,9 +5554,7 @@ vect_create_epilog_for_reduction (loop_vec_info loop_vinfo,
>>              for MIN and MAX reduction, for example.  */
>>           if (!neutral_op)
>>             {
>> -             tree scalar_value
>> -               = PHI_ARG_DEF_FROM_EDGE (orig_phis[i]->stmt,
>> -                                        loop_preheader_edge (loop));
>> +             tree scalar_value = vect_phi_initial_value (orig_phis[i]);
>>               scalar_value = gimple_convert (&seq, TREE_TYPE (vectype),
>>                                              scalar_value);
>>               vector_identity = gimple_build_vector_from_val (&seq, vectype,
>> @@ -6752,10 +6746,7 @@ vectorizable_reduction (loop_vec_info loop_vinfo,
>>        else if (cond_reduc_dt == vect_constant_def)
>>         {
>>           enum vect_def_type cond_initial_dt;
>> -         tree cond_initial_val
>> -           = PHI_ARG_DEF_FROM_EDGE (reduc_def_phi, loop_preheader_edge (loop));
>> -
>> -         gcc_assert (cond_reduc_val != NULL_TREE);
>> +         tree cond_initial_val = vect_phi_initial_value (reduc_def_phi);
>>           vect_is_simple_use (cond_initial_val, loop_vinfo, &cond_initial_dt);
>>           if (cond_initial_dt == vect_constant_def
>>               && types_compatible_p (TREE_TYPE (cond_initial_val),
>> @@ -7528,8 +7519,7 @@ vect_transform_cycle_phi (loop_vec_info loop_vinfo,
>>      {
>>        /* Get at the scalar def before the loop, that defines the initial
>>          value of the reduction variable.  */
>> -      tree initial_def = PHI_ARG_DEF_FROM_EDGE (phi,
>> -                                               loop_preheader_edge (loop));
>> +      tree initial_def = vect_phi_initial_value (phi);
>>        /* Optimize: if initial_def is for REDUC_MAX smaller than the base
>>          and we can't use zero for induc_val, use initial_def.  Similarly
>>          for REDUC_MIN and initial_def larger than the base.  */
>> @@ -8175,8 +8165,7 @@ vectorizable_induction (loop_vec_info loop_vinfo,
>>        return true;
>>      }
>>
>> -  init_expr = PHI_ARG_DEF_FROM_EDGE (phi,
>> -                                    loop_preheader_edge (iv_loop));
>> +  init_expr = vect_phi_initial_value (phi);
>>
>>    gimple_seq stmts = NULL;
>>    if (!nested_in_vect_loop)
>> diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h
>> index fa28336d429..e2fd3609fee 100644
>> --- a/gcc/tree-vectorizer.h
>> +++ b/gcc/tree-vectorizer.h
>> @@ -27,7 +27,7 @@ typedef class _stmt_vec_info *stmt_vec_info;
>>  #include "tree-hash-traits.h"
>>  #include "target.h"
>>  #include "internal-fn.h"
>> -
>> +#include "tree-ssa-operands.h"
>>
>>  /* Used for naming of new temporaries.  */
>>  enum vect_var_kind {
>> @@ -1369,6 +1369,25 @@ nested_in_vect_loop_p (class loop *loop, stmt_vec_info stmt_info)
>>           && (loop->inner == (gimple_bb (stmt_info->stmt))->loop_father));
>>  }
>>
>> +/* PHI is either a scalar reduction phi or a scalar induction phi.
>> +   Return the initial value of the variable on entry to the containing
>> +   loop.  */
>> +
>> +static inline tree
>> +vect_phi_initial_value (gphi *phi)
>> +{
>> +  basic_block bb = gimple_bb (phi);
>> +  edge pe = loop_preheader_edge (bb->loop_father);
>> +  gcc_assert (pe->dest == bb);
>> +  return PHI_ARG_DEF_FROM_EDGE (phi, pe);
>> +}
>> +
>> +static inline tree
>> +vect_phi_initial_value (stmt_vec_info stmt_info)
>> +{
>> +  return vect_phi_initial_value (as_a <gphi *> (stmt_info->stmt));
>> +}
>> +
>>  /* Return true if STMT_INFO should produce a vector mask type rather than
>>     a normal nonmask type.  */
>>