Re: [PATCH 1/3] vect: Pass stmt_vec_info to TARGET_SIMD_CLONE_USABLE

["Andre Vieira (lists)" <andre.simoesdiasvieira@arm.com>]

On 31/01/2024 14:03, Richard Biener wrote:
> On Wed, 31 Jan 2024, Richard Biener wrote:
> 
>> On Wed, 31 Jan 2024, Andre Vieira (lists) wrote:
>>
>>>
>>>
>>> On 31/01/2024 12:13, Richard Biener wrote:
>>>> On Wed, 31 Jan 2024, Richard Biener wrote:
>>>>
>>>>> On Tue, 30 Jan 2024, Andre Vieira wrote:
>>>>>
>>>>>>
>>>>>> This patch adds stmt_vec_info to TARGET_SIMD_CLONE_USABLE to make sure the
>>>>>> target can reject a simd_clone based on the vector mode it is using.
>>>>>> This is needed because for VLS SVE vectorization the vectorizer accepts
>>>>>> Advanced SIMD simd clones when vectorizing using SVE types because the
>>>>>> simdlens
>>>>>> might match.  This will cause type errors later on.
>>>>>>
>>>>>> Other targets do not currently need to use this argument.
>>>>>
>>>>> Can you instead pass down the mode?
>>>>
>>>> Thinking about that again the cgraph_simd_clone info in the clone
>>>> should have sufficient information to disambiguate.  If it doesn't
>>>> then we should amend it.
>>>>
>>>> Richard.
>>>
>>> Hi Richard,
>>>
>>> Thanks for the review, I don't think cgraph_simd_clone_info is the right place
>>> to pass down this information, since this is information about the caller
>>> rather than the simdclone itself. What we are trying to achieve here is making
>>> the vectorizer being able to accept or reject simdclones based on the ISA we
>>> are vectorizing for. To distinguish between SVE and Advanced SIMD ISAs we use
>>> modes, I am also not sure that's ideal but it is what we currently use. So to
>>> answer your earlier question, yes I can also pass down mode if that's
>>> preferable.
>>
>> Note cgraph_simd_clone_info has simdlen and we seem to check elsewhere
>> whether that's POLY or constant.  I wonder how aarch64_sve_mode_p
>> comes into play here which in the end classifies VLS SVE modes as
>> non-SVE?
> 
> Maybe it's just a bit non-obvious as you key on mangling:
> 
>   static int
> -aarch64_simd_clone_usable (struct cgraph_node *node)
> +aarch64_simd_clone_usable (struct cgraph_node *node, stmt_vec_info
> stmt_vinfo)
>   {
>     switch (node->simdclone->vecsize_mangle)
>       {
>       case 'n':
>         if (!TARGET_SIMD)
>          return -1;
> +      if (STMT_VINFO_VECTYPE (stmt_vinfo)
> +         && aarch64_sve_mode_p (TYPE_MODE (STMT_VINFO_VECTYPE
> (stmt_vinfo))))
> +       return -1;
> 
> ?  What does 'n' mean?  It's documented as
> 
>    /* The mangling character for a given vector size.  This is used
>       to determine the ISA mangling bit as specified in the Intel
>       Vector ABI.  */
>    unsigned char vecsize_mangle;

I'll update the comment, but yeh 'n' is for Advanced SIMD, 's' is for SVE.
> 
> which is slightly misleading.