Hi Prathamesh,
I am just looking at this as it interacts with a change I am trying to
make, but I'm not a reviewer so take my comments with a pinch of salt ;)
I copied in bits of your patch below to comment.
> -@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}
Would be good to also explain what the new op_mode parameter is used for
here.
> @@ -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;
> }
I was surprised by the assert check here. The docs seem to insinuate
vec_perm needs both input vectors to be of the same mode, so I was
expecting this to be checked/enforced elsewhere? But it shouldn't hurt I
guess.
> @@ -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)
Same comment as the first, it could do with an inclusion of op_mode in
the comments explaining the function.
On 18/05/2022 08:06, Prathamesh Kulkarni via Gcc-patches wrote:
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
