Re: [0/9] [middle-end] Add param to vec_perm_const hook to specify mode of input operand

[Richard Sandiford <richard.sandiford@arm.com>]

Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> Hi,
> The attached patch adds another parameter machine_mode op_mode to vec_perm_const
> hook to specify mode of input operands. The motivation for doing this
> is PR96463,
> where we create vec_perm_expr of the form:
> lhs = vec_perm_expr<rhs, mask>
> where lhs and rhs have different vector types but same element type
> (lhs is SVE and rhs is corresponding advsimd vector).
>
> It seems the following targets were affected: aarch64, i386, arm, ia64,
> mips, rs6000, s390, sparc, gcn.
>
> Bootstrapped+tested on x86_64-linux-gnu, aarch64-linux-gnu.
> For other targets, I did make all-gcc stage1, which seems to build OK.
>
> Thanks,
> Prathamesh
>
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index c5006afc00d..31ff6ef3f92 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -6088,7 +6088,7 @@ for the given scalar type @var{type}.  @var{is_packed} is false if the scalar
>  access using @var{type} is known to be naturally aligned.
>  @end deftypefn
>  
> -@deftypefn {Target Hook} bool TARGET_VECTORIZE_VEC_PERM_CONST (machine_mode @var{mode}, rtx @var{output}, rtx @var{in0}, rtx @var{in1}, const vec_perm_indices @var{&sel})
> +@deftypefn {Target Hook} bool TARGET_VECTORIZE_VEC_PERM_CONST (machine_mode @var{mode}, machine_mode @var{op_mode}, rtx @var{output}, rtx @var{in0}, rtx @var{in1}, const vec_perm_indices @var{&sel})
>  This hook is used to test whether the target can permute up to two
>  vectors of mode @var{mode} using the permutation vector @code{sel}, and
>  also to emit such a permutation.  In the former case @var{in0}, @var{in1}

Like Andre says, the documentation should describe op_mode (and mode).

> diff --git a/gcc/optabs-query.cc b/gcc/optabs-query.cc
> index 68dc679cc6a..aef9d4c5d28 100644
> --- a/gcc/optabs-query.cc
> +++ b/gcc/optabs-query.cc
> @@ -417,8 +417,8 @@ can_vec_perm_var_p (machine_mode mode)
>     with here.  */
>  
>  bool
> -can_vec_perm_const_p (machine_mode mode, const vec_perm_indices &sel,
> -		      bool allow_variable_p)
> +can_vec_perm_const_p (machine_mode mode, machine_mode op_mode,
> +		      const vec_perm_indices &sel, bool allow_variable_p)
>  {

The function comment should describe the new parameter.

>    /* If the target doesn't implement a vector mode for the vector type,
>       then no operations are supported.  */
> @@ -448,7 +448,7 @@ can_vec_perm_const_p (machine_mode mode, const vec_perm_indices &sel,
>  
>    if (targetm.vectorize.vec_perm_const != NULL)
>      {
> -      if (targetm.vectorize.vec_perm_const (mode, NULL_RTX, NULL_RTX,
> +      if (targetm.vectorize.vec_perm_const (mode, op_mode, NULL_RTX, NULL_RTX,
>  					    NULL_RTX, sel))
>  	return true;
>  
> @@ -462,6 +462,13 @@ can_vec_perm_const_p (machine_mode mode, const vec_perm_indices &sel,
>    return false;
>  }
>  
> +bool
> +can_vec_perm_const_p (machine_mode mode, const vec_perm_indices &sel,
> +		      bool allow_variable_p)
> +{
> +  return can_vec_perm_const_p (mode, mode, sel, allow_variable_p);
> +}
> +

I can understand why you went for this, but now that we've opened
the door to mismatched modes, I think it would be better if all callers
specified the input mode explicitly.

> diff --git a/gcc/optabs.cc b/gcc/optabs.cc
> index 3d8fa3abdfe..55f10c41789 100644
> --- a/gcc/optabs.cc
> +++ b/gcc/optabs.cc
> @@ -6250,7 +6250,9 @@ expand_vec_perm_const (machine_mode mode, rtx v0, rtx v1,
>        if (single_arg_p)
>  	v1 = v0;
>  
> -      if (targetm.vectorize.vec_perm_const (mode, target, v0, v1, indices))
> +      gcc_checking_assert (GET_MODE (v0) == GET_MODE (v1));
> +      machine_mode op_mode = GET_MODE (v0);
> +      if (targetm.vectorize.vec_perm_const (mode, op_mode, target, v0, v1, indices))
>  	return target;
>      }
>  

(FWIW, I agree the assert is worth having.)

Thanks,
Richard

> @@ -6264,7 +6266,7 @@ expand_vec_perm_const (machine_mode mode, rtx v0, rtx v1,
>        v0_qi = gen_lowpart (qimode, v0);
>        v1_qi = gen_lowpart (qimode, v1);
>        if (targetm.vectorize.vec_perm_const != NULL
> -	  && targetm.vectorize.vec_perm_const (qimode, target_qi, v0_qi,
> +	  && targetm.vectorize.vec_perm_const (qimode, qimode, target_qi, v0_qi,
>  					       v1_qi, qimode_indices))
>  	return gen_lowpart (mode, target_qi);
>      }
> diff --git a/gcc/target.def b/gcc/target.def
> index d85adf36a39..2713c31dc3f 100644
> --- a/gcc/target.def
> +++ b/gcc/target.def
> @@ -1894,7 +1894,7 @@ try the equivalent byte operation.  If that also fails, it will try forcing\n\
>  the selector into a register and using the @var{vec_perm@var{mode}}\n\
>  instruction pattern.  There is no need for the hook to handle these two\n\
>  implementation approaches itself.",
> - bool, (machine_mode mode, rtx output, rtx in0, rtx in1,
> + bool, (machine_mode mode, machine_mode op_mode, rtx output, rtx in0, rtx in1,
>  	const vec_perm_indices &sel),
>   NULL)
>