Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> writes:
> On Wed, 18 May 2022 at 17:27, Richard Sandiford
> <richard.sandif...@arm.com> wrote:
>>
>> Prathamesh Kulkarni <prathamesh.kulka...@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.)
> Hi,
> I updated the patch with doc and adjusted callers to explicitly pass op_mode.
> Bootstrapped + tested on x86_64-linux-gnu and aarch64-linux-gnu.
> Does it look OK to commit ?
>
> […]
> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> index c5006afc00d..f53068c5c53 100644
> --- a/gcc/doc/tm.texi
> +++ b/gcc/doc/tm.texi
> @@ -6088,13 +6088,13 @@ 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
> +vectors of mode @var{op_mode} using the permutation vector @code{sel}, and
> also to emit such a permutation. In the former case @var{in0}, @var{in1}
> and @var{out} are all null. In the latter case @var{in0} and @var{in1} are
How about:
This hook is used to test whether the target can permute up to two
vectors of mode @var{op_mode} using the permutation vector @code{sel},
producing a vector of mode @var{mode}. The hook is also used to emit
such a permutation.
When the hook is being to test whether the target supports a permutation,
@var{in0}, @var{in1}, and @var{out} are all null. When the hook is
being used to emit a permutation, @var{in0} and @var{in1} are …
so that we describe @var{mode} for both the testing and non-testing cases.
> -the source vectors and @var{out} is the destination vector; all three are
> -operands of mode @var{mode}. @var{in1} is the same as @var{in0} if
> +the source vectors of mode @var{op_mode} and @var{out} is the destination
> vector
> +of mode @var{mode}. @var{in1} is the same as @var{in0} if
> @var{sel} describes a permutation on one vector instead of two.
>
> Return true if the operation is possible, emitting instructions for it
> diff --git a/gcc/match.pd b/gcc/match.pd
> index f5efa77560c..b06d7013603 100644
> --- a/gcc/match.pd
> +++ b/gcc/match.pd
> @@ -7703,12 +7703,14 @@ and,
> 2-argument version. */
> tree oldop2 = op2;
> if (sel.ninputs () == 2
> - || can_vec_perm_const_p (TYPE_MODE (type), sel, false))
> + || can_vec_perm_const_p (TYPE_MODE (type), TYPE_MODE (type),
> + sel, false))
> op2 = vec_perm_indices_to_tree (TREE_TYPE (op2), sel);
> else
> {
> vec_perm_indices sel2 (builder, 2, nelts);
> - if (can_vec_perm_const_p (TYPE_MODE (type), sel2, false))
> + if (can_vec_perm_const_p (TYPE_MODE (type), TYPE_MODE (type),
> + sel2, false))
> op2 = vec_perm_indices_to_tree (TREE_TYPE (op2), sel2);
> else
> /* Not directly supported with either encoding,
Since this is matching an existing VEC_PERM_EXPR, we need to distinguish
between mode and op_mode. Might be worth adding both mode and op_mode
to the outermost “with” and changing later tests accordingly.
> diff --git a/gcc/optabs-query.h b/gcc/optabs-query.h
> index b9c9fd6f64d..32e52b20109 100644
> --- a/gcc/optabs-query.h
> +++ b/gcc/optabs-query.h
> @@ -178,7 +178,7 @@ bool can_conditionally_move_p (machine_mode mode);
> opt_machine_mode qimode_for_vec_perm (machine_mode);
> bool selector_fits_mode_p (machine_mode, const vec_perm_indices &);
> bool can_vec_perm_var_p (machine_mode);
> -bool can_vec_perm_const_p (machine_mode, const vec_perm_indices &,
> +bool can_vec_perm_const_p (machine_mode, machine_mode, const
> vec_perm_indices &,
> bool = true);
Nit: long line.
> /* Find a widening optab even if it doesn't widen as much as we want. */
> #define find_widening_optab_handler(A, B, C) \
> […]
> diff --git a/gcc/tree-vect-generic.cc b/gcc/tree-vect-generic.cc
> index d99e3207fbe..17de3375127 100644
> --- a/gcc/tree-vect-generic.cc
> +++ b/gcc/tree-vect-generic.cc
> @@ -1527,7 +1527,8 @@ lower_vec_perm (gimple_stmt_iterator *gsi)
> && tree_to_vec_perm_builder (&sel_int, mask))
> {
> vec_perm_indices indices (sel_int, 2, elements);
> - if (can_vec_perm_const_p (TYPE_MODE (vect_type), indices))
> + machine_mode vmode = TYPE_MODE (vect_type);
> + if (can_vec_perm_const_p (vmode, vmode, indices))
Similarly here: since this is testing an existing VEC_PERM_EXPR, I think
we need to read the mode of the lhs and pass both modes.
> […]
> diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
> index 8327e9d047e..207d1097d5b 100644
> --- a/gcc/tree-vect-stmts.cc
> +++ b/gcc/tree-vect-stmts.cc
> @@ -2016,7 +2016,7 @@ perm_mask_for_reverse (tree vectype)
> sel.quick_push (nunits - 1 - i);
>
> vec_perm_indices indices (sel, 1, nunits);
> - if (!can_vec_perm_const_p (TYPE_MODE (vectype), indices))
> + if (!can_vec_perm_const_p (TYPE_MODE (vectype), TYPE_MODE (vectype),
> indices))
Nit: long line.
Thanks,
Richard
