Re: [PATCH] vect: Use statement vectype for conditional mask.

[Richard Biener <richard.guenther@gmail.com>]

On Wed, Nov 8, 2023 at 5:18 PM Robin Dapp <rdapp.gcc@gmail.com> wrote:
>
> Hi,
>
> as Tamar reported in PR112406 we still ICE on aarch64 in SPEC2017
> when creating COND_OPs in ifcvt.
>
> The problem is that we fail to deduce the mask's type from the statement
> vectype and then end up with a non-matching mask in expand.  This patch
> checks if the current op is equal to the mask operand and, if so, uses
> the truth type from the stmt_vectype.  Is that a valid approach?
>
> Bootstrapped and regtested on aarch64, x86 is running.
>
> Besides, the testcase is Tamar's reduced example, originally from
> SPEC.  I hope it's ok to include it as is (as imagick is open source
> anyway).

For the fortran testcase we don't even run into this but hit an
internal def and assert on

      gcc_assert (STMT_VINFO_VEC_STMTS (def_stmt_info).length () == ncopies);

I think this shows missing handling of .COND_* in the bool pattern recognition
as we get the 'bool' condition as boolean data vector rather than a mask.  The
same is true for the testcase with the invariant condition.  This causes us to
select the wrong vector type here.  The "easiest" might be to look at
how COND_EXPR is handled in vect_recog_bool_pattern and friends and
handle .COND_* IFNs the same for the mask operand.

Richard.

> Regards
>  Robin
>
> gcc/ChangeLog:
>
>         PR middle-end/112406
>
>         * tree-vect-stmts.cc (vect_get_vec_defs_for_operand): Handle
>         masks of conditional ops.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.dg/pr112406.c: New test.
> ---
>  gcc/testsuite/gcc.dg/pr112406.c | 37 +++++++++++++++++++++++++++++++++
>  gcc/tree-vect-stmts.cc          | 20 +++++++++++++++++-
>  2 files changed, 56 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/gcc.dg/pr112406.c
>
> diff --git a/gcc/testsuite/gcc.dg/pr112406.c b/gcc/testsuite/gcc.dg/pr112406.c
> new file mode 100644
> index 00000000000..46459c68c4a
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/pr112406.c
> @@ -0,0 +1,37 @@
> +/* { dg-do compile { target { aarch64*-*-* } } } */
> +/* { dg-options "-march=armv8-a+sve -w -Ofast" } */
> +
> +typedef struct {
> +  int red
> +} MagickPixelPacket;
> +
> +GetImageChannelMoments_image, GetImageChannelMoments_image_0,
> +    GetImageChannelMoments___trans_tmp_1, GetImageChannelMoments_M11_0,
> +    GetImageChannelMoments_pixel_3, GetImageChannelMoments_y,
> +    GetImageChannelMoments_p;
> +
> +double GetImageChannelMoments_M00_0, GetImageChannelMoments_M00_1,
> +    GetImageChannelMoments_M01_1;
> +
> +MagickPixelPacket GetImageChannelMoments_pixel;
> +
> +SetMagickPixelPacket(int color, MagickPixelPacket *pixel) {
> +  pixel->red = color;
> +}
> +
> +GetImageChannelMoments() {
> +  for (; GetImageChannelMoments_y; GetImageChannelMoments_y++) {
> +    SetMagickPixelPacket(GetImageChannelMoments_p,
> +                         &GetImageChannelMoments_pixel);
> +    GetImageChannelMoments_M00_1 += GetImageChannelMoments_pixel.red;
> +    if (GetImageChannelMoments_image)
> +      GetImageChannelMoments_M00_1++;
> +    GetImageChannelMoments_M01_1 +=
> +        GetImageChannelMoments_y * GetImageChannelMoments_pixel_3;
> +    if (GetImageChannelMoments_image_0)
> +      GetImageChannelMoments_M00_0++;
> +    GetImageChannelMoments_M01_1 +=
> +        GetImageChannelMoments_y * GetImageChannelMoments_p++;
> +  }
> +  GetImageChannelMoments___trans_tmp_1 = atan(GetImageChannelMoments_M11_0);
> +}
> diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
> index 65883e04ad7..6793b01bf44 100644
> --- a/gcc/tree-vect-stmts.cc
> +++ b/gcc/tree-vect-stmts.cc
> @@ -1238,10 +1238,28 @@ vect_get_vec_defs_for_operand (vec_info *vinfo, stmt_vec_info stmt_vinfo,
>        tree stmt_vectype = STMT_VINFO_VECTYPE (stmt_vinfo);
>        tree vector_type;
>
> +      /* For a COND_OP the mask operand's type must not be deduced from the
> +        scalar type but from the statement's vectype.  */
> +      bool use_stmt_vectype = false;
> +      gcall *call;
> +      if ((call = dyn_cast <gcall *> (STMT_VINFO_STMT (stmt_vinfo)))
> +         && gimple_call_internal_p (call))
> +       {
> +         internal_fn ifn = gimple_call_internal_fn (call);
> +         int mask_idx = -1;
> +         if (ifn != IFN_LAST
> +             && (mask_idx = internal_fn_mask_index (ifn)) != -1)
> +           {
> +             tree maskop = gimple_call_arg (call, mask_idx);
> +             if (op == maskop)
> +               use_stmt_vectype = true;
> +           }
> +       }
> +
>        if (vectype)
>         vector_type = vectype;
>        else if (VECT_SCALAR_BOOLEAN_TYPE_P (TREE_TYPE (op))
> -              && VECTOR_BOOLEAN_TYPE_P (stmt_vectype))
> +              && (use_stmt_vectype || VECTOR_BOOLEAN_TYPE_P (stmt_vectype)))
>         vector_type = truth_type_for (stmt_vectype);
>        else
>         vector_type = get_vectype_for_scalar_type (loop_vinfo, TREE_TYPE (op));
> --
> 2.41.0