Re: [r13-7135 Regression] FAIL: gcc.dg/vect/vect-simd-clone-18f.c scan-tree-dump-times vect "[\\n\\r] [^\\n]* = foo\\.simdclone" 2 on Linux/x86_64

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

On Fri, Apr 14, 2023 at 11:42 AM Andre Vieira (lists)
<andre.simoesdiasvieira@arm.com> wrote:
>
>
>
> On 14/04/2023 10:09, Richard Biener wrote:
> > On Fri, Apr 14, 2023 at 10:43 AM Andre Vieira (lists)
> > <andre.simoesdiasvieira@arm.com> wrote:
> >>
> >> Resending this to everyone (sorry for the double send Richard).
> >>
> >> On 14/04/2023 09:15, Andre Vieira (lists) wrote:
> >>   >
> >>   >
> >>   > On 14/04/2023 07:55, Richard Biener wrote:
> >>   >> On Thu, Apr 13, 2023 at 4:25 PM Andre Vieira (lists)
> >>   >> <andre.simoesdiasvieira@arm.com> wrote:
> >>   >>>
> >>   >>>
> >>   >>>
> >>   >>> On 13/04/2023 15:00, Richard Biener wrote:
> >>   >>>> On Thu, Apr 13, 2023 at 3:00 PM Andre Vieira (lists) via Gcc-patches
> >>   >>>> <gcc-patches@gcc.gnu.org> wrote:
> >>   >>>>>
> >>   >>>>>
> >>   >>>>>
> >>   >>>
> >>   >>> But that's not it, I've been looking at it, and there is code in place
> >>   >>> that does what I expected which is defer the choice of vectype for simd
> >>   >>> clones until vectorizable_simd_clone_call, unfortunately it has a
> >>   >>> mistaken assumption that simdclones don't return :/
> >>   >>
> >>   >> I think that's not it - when the SIMD clone returns a vector we have to
> >>   >> determine the vector type in this function.  We cannot defer this.
> >>   >
> >>   > What's 'this function' here, do you mean we have to determine the
> >>   > vectype in 'vect_get_vector_types_for_stmt' &
> >>   > 'vect_determine_vf_for_stmt' ?
> >
> > Yes.
> >
> >> Because at that time we don't yet know
> >>   > what clone we will be using, this choice is done inside
> >>   > vectorizable_simd_clone_call. In fact, to choose the simd clone, we need
> >>   > to know the vf as that has to be a multiple of the chosen clone's
> >>   > simdlen. So we simply can't use the simdclone's types (as that depends
> >>   > on the simdlen) to choose the vf because the choice of simdlend depends
> >>   > on the vf. And there was already code in place to handle this,
> >>   > unfortunately that code was wrong and had the wrong assumption that
> >>   > simdclones didn't return (probably was true at some point and bitrotted).
> >
> > But to compute the VF we need to know the vector types!  We're only
> > calling vectorizable_* when the VF is final.  That said, the code you quote:
> >
> >>   >>
> >>   >>> see vect_get_vector_types_for_stmt:
> >>   >>> ...
> >>   >>>     if (gimple_get_lhs (stmt) == NULL_TREE
> >
> > is just for the case of a function without return value.  For this case
> > it's OK to do nothing - 'vectype' is the vector type of all vector defs
> > a stmt produces.
> >
> > For calls with a LHS it should fall through to generic code doing
> > get_vectype_for_scalar_type on the LHS type.
>
> I think that may work, but right now it will still go and look at the
> arguments of the call and use the smallest type among them to adjust the
> nunits (in 'vect_get_vector_types_for_stmt').
>
> In fact (this is just for illustration) if I hack that function like this:
> --- a/gcc/tree-vect-stmts.cc
> +++ b/gcc/tree-vect-stmts.cc
> @@ -12745,8 +12745,11 @@ vect_get_vector_types_for_stmt (vec_info
> *vinfo, stmt_vec_info stmt_info,
>         /* The number of units is set according to the smallest scalar
>           type (or the largest vector size, but we only support one
>           vector size per vectorization).  */
> -      scalar_type = vect_get_smallest_scalar_type (stmt_info,
> -                                                  TREE_TYPE (vectype));
> +      if (simd_clone_call_p (stmt_info->stmt))
> +       scalar_type = TREE_TYPE (vectype);
> +      else
> +       scalar_type = vect_get_smallest_scalar_type (stmt_info,
> +                                                    TREE_TYPE (vectype));
>         if (scalar_type != TREE_TYPE (vectype))
>          {
>            if (dump_enabled_p ())
>
> It will use the same types as before without (-m32), like I explained
> before the -m32 turns the pointer inside MASK_CALL into a 32-bit pointer
> so now the smallest size is 32-bits. This makes it pick V8SI instead of
> the original V4DI (scalar return type is DImode). Changing the VF to 8,
> thus unrolling the loop as it needs to make 2 calls, each handling 4 nunits.

Ah, but then vect_get_smallest_scalar_type should simply ignore that
pointer in MASK_CALL.  It should only look at the arguments relevant
for vectorization.  So

diff --git a/gcc/tree-vect-data-refs.cc b/gcc/tree-vect-data-refs.cc
index 8daf7bd7dd3..70bb8595951 100644
--- a/gcc/tree-vect-data-refs.cc
+++ b/gcc/tree-vect-data-refs.cc
@@ -165,6 +165,8 @@ vect_get_smallest_scalar_type (stmt_vec_info
stmt_info, tree scalar_type)
            }
          else if (internal_fn_mask_index (ifn) == 0)
            i = 1;
+         else if (ifn == IFN_MASK_CALL)
+           i = 1;
        }
       if (i < gimple_call_num_args (call))
        {

?

Richard.

>
>