Richard Sandiford via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> Tamar Christina <tamar.christ...@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));
Looking again, all the other tests return false if they found a definite
problem and fall through to later code otherwise. I think we should do
the same here.
Thanks,
Richard
