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

[Richard Biener via Gcc-patches]

> Am 25.05.2022 um 21:03 schrieb Prathamesh Kulkarni 
> <prathamesh.kulka...@linaro.org>:
> 
> On Wed, 25 May 2022 at 18:27, Richard Biener <richard.guent...@gmail.com> 
> wrote:
>> 
>>> On Tue, May 24, 2022 at 9:22 PM Prathamesh Kulkarni via Gcc-patches
>>> <gcc-patches@gcc.gnu.org> wrote:
>>> 
>>> On Tue, 24 May 2022 at 14:50, Richard Sandiford
>>> <richard.sandif...@arm.com> wrote:
>>>> 
>>>> Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> writes:
>>>>> diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
>>>>> index c5006afc00d..0a3c733ada9 100644
>>>>> --- a/gcc/doc/tm.texi
>>>>> +++ b/gcc/doc/tm.texi
>>>>> @@ -6088,14 +6088,18 @@ 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}
>>>>> -and @var{out} are all null.  In the latter case @var{in0} and @var{in1} 
>>>>> are
>>>>> -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
>>>>> -@var{sel} describes a permutation on one vector instead of 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
>>>> 
>>>> Should be two spaces between “@var{mode}.” and “The”.
>>>> 
>>>>> +a permutation.
>>>>> +
>>>>> +When the hook is being used to test whether the target supports a 
>>>>> permutation,
>>>>> +@var{in0}, @var{in1}, and @var{out} are all null.When the hook is being 
>>>>> used
>>>> 
>>>> Same here: missing spaces before “When”.
>>>> 
>>>>> +to emit a permutation, @var{in0} and @var{in1} are 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
>>>>> if rtxes are provided.
>>>>> diff --git a/gcc/match.pd b/gcc/match.pd
>>>>> index f5efa77560c..f2a527d9c42 100644
>>>>> --- a/gcc/match.pd
>>>>> +++ b/gcc/match.pd
>>>>> @@ -7596,6 +7596,8 @@ and,
>>>>>  (with
>>>>>   {
>>>>>     tree op0 = @0, op1 = @1, op2 = @2;
>>>>> +    machine_mode result_mode = TYPE_MODE (type);
>>>>> +    machine_mode op_mode = TYPE_MODE (TREE_TYPE (op0));
>>>>> 
>>>>>     /* Build a vector of integers from the tree mask.  */
>>>>>     vec_perm_builder builder;
>>>>> @@ -7703,12 +7705,12 @@ 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 (result_mode, op_mode, 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 (result_mode, op_mode, sel2, 
>>>>> false))
>>>>>                op2 = vec_perm_indices_to_tree (TREE_TYPE (op2), sel2);
>>>>>              else
>>>>>                /* Not directly supported with either encoding,
>>>> 
>>>> Please replace the use of TYPE_MODE here:
>>>> 
>>>>        /* See if the permutation is performing a single element
>>>>           insert from a CONSTRUCTOR or constant and use a BIT_INSERT_EXPR
>>>>           in that case.  But only if the vector mode is supported,
>>>>           otherwise this is invalid GIMPLE.  */
>>>>        if (TYPE_MODE (type) != BLKmode
>>>> 
>>>> as well.
>>>> 
>>>> OK with those changes, thanks.
>>> Thanks, committed the patch in ae8decf1d2b8329af59592b4fa78ee8dfab3ba5e.
>> 
>> So the present state allows to ask can_vec_perm_const_p but the
>> implementation asks for
>> 
>> 431           if (direct_optab_handler (vec_perm_optab, mode) !=
>> CODE_FOR_nothing)
>> 432             return true;
>> 
>> which then breaks.  Also the VEC_PERMs are not yet valid.  Any reason this
>> was committed half-way through the review process of the series?
> Hi Richard,
> I am very sorry about that. I thought the patch was approved, and
> committed it after testing on x86_64 and aarch64.
> Should I revert it ?

No need.

> Um sorry to ask a silly question -- I am not sure why does the patch
> break the above condition ?
> The patch still passes mode to direct_optab_handler as before, and
> IIUC does not affect modes
> for vec_perm_optab, so it shouldn't affect the call to
> direct_optab_handler above ?

x86 now accepts V4SI V8SI permutes because we don’t ask it correctly and thus 
my naive attempt to use the new function API breaks . Not to mention the 
VEC_PERM IL is still rejected. I will wait for the rest of the series to be 
approved and pushed.

Richard.

> Thanks,
> Prathamesh
>> 
>> At least I have a user in the vectorizer ready - allowing more permutes
>> from existing vectors (of different sizes now) to be SLP vectorized.
>> 
>> Thanks,
>> Richard.
>> 
>>> Thanks,
>>> Prathamesh
>>>> 
>>>> Richard