Re: [PATCH]AArch64 Fix vector re-interpretation between partial SIMD modes

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

Tamar Christina <tamar.christina@arm.com> writes:
> Hi All,
>
> While writing a patch series I started getting incorrect codegen out from
> VEC_PERM on partial struct types.
>
> It turns out that this was happening because the TARGET_CAN_CHANGE_MODE_CLASS
> implementation has a slight bug in it.  The hook only checked for SIMD to
> Partial but never Partial to SIMD.   This resulted in incorrect subregs to be
> generated from the fallback code in VEC_PERM_EXPR expansions.
>
> I have unfortunately not been able to trigger it using a standalone testcase as
> the mid-end optimizes away the permute every time I try to describe a permute
> that would result in the bug.
>
> The patch now rejects any conversion of partial SIMD struct types, unless they
> are both partial structures of the same number of registers or one is a SIMD
> type who's size is less than 8 bytes.
>
> Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
>
> Ok for master? And backport to GCC 12?
>
> Thanks,
> Tamar
>
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64.cc (aarch64_can_change_mode_class): Restrict
> 	conversions between partial struct types properly.
>
> --- inline copy of patch -- 
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index d3c3650d7d728f56adb65154127dc7b72386c5a7..84dbe2f4ea7d03b424602ed98a34e7824217dc91 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -26471,9 +26471,10 @@ aarch64_can_change_mode_class (machine_mode from,
>    bool from_pred_p = (from_flags & VEC_SVE_PRED);
>    bool to_pred_p = (to_flags & VEC_SVE_PRED);
>  
> -  bool from_full_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT));
>    bool to_partial_advsimd_struct_p = (to_flags == (VEC_ADVSIMD | VEC_STRUCT
>  						   | VEC_PARTIAL));
> +  bool from_partial_advsimd_struct_p = (from_flags == (VEC_ADVSIMD | VEC_STRUCT
> +						   | VEC_PARTIAL));
>  
>    /* Don't allow changes between predicate modes and other modes.
>       Only predicate registers can hold predicate modes and only
> @@ -26496,9 +26497,23 @@ aarch64_can_change_mode_class (machine_mode from,
>      return false;
>  
>    /* Don't allow changes between partial and full Advanced SIMD structure
> -     modes.  */
> -  if (from_full_advsimd_struct_p && to_partial_advsimd_struct_p)
> -    return false;
> +     modes unless both are a partial struct with the same number of registers
> +     or the vector bitsizes must be the same.  */
> +  if (to_partial_advsimd_struct_p ^ from_partial_advsimd_struct_p)
> +    {
> +      /* If they're both partial structures, allow if they have the same number
> +	 or registers.  */
> +      if (to_partial_advsimd_struct_p == from_partial_advsimd_struct_p)
> +	return known_eq (GET_MODE_SIZE (from), GET_MODE_SIZE (to));

It looks like the ^ makes this line unreachable.  I guess it should
be a separate top-level condition.

> +      /* If one is a normal SIMD register, allow only if no larger than 64-bit.  */
> +      if ((to_flags & VEC_ADVSIMD) == to_flags)
> +	return known_le (GET_MODE_SIZE (to), 8);
> +      else if ((from_flags & VEC_ADVSIMD) == from_flags)
> +	return known_le (GET_MODE_SIZE (from), 8);
> +
> +      return false;
> +    }

I don't think we need to restrict this to SIMD modes.  A plain DI would
be OK too.  So I think it should just be:

    return (known_le (GET_MODE_SIZE (to), 8)
            || known_le (GET_MODE_SIZE (from, 8));

Thanks,
Richard

>  
>    if (maybe_ne (BITS_PER_SVE_VECTOR, 128u))
>      {