Hi David,
Thanks for the review!
on 2021/10/27 下午9:12, David Edelsohn wrote:
> On Sun, Oct 24, 2021 at 11:04 PM Kewen.Lin <li...@linux.ibm.com> wrote:
>>
>> Hi,
>>
>> As PR102767 shows, the commit r12-3482 exposed one ICE in function
>> rs6000_builtin_vectorization_cost. We claims V1TI supports movmisalign
>> on rs6000 (See define_expand "movmisalign<mode>"), so it return true in
>> rs6000_builtin_support_vector_misalignment for misalign 8. Later in
>> the cost querying rs6000_builtin_vectorization_cost, we don't have
>> the arms to handle the V1TI input under (TARGET_VSX &&
>> TARGET_ALLOW_MOVMISALIGN).
>>
>> The proposed fix is to add the consideration for V1TI, simply make it
>> as the cost for doubleword which is apparently bigger than the cost of
>> scalar, won't have the vectorization to happen, just to keep consistency
>> and avoid ICE. Another thought is to not support movmisalign for V1TI,
>> but it sounds like a bad idea since it doesn't match the reality.
>>
>> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
>> powerpc64-linux-gnu P8.
>>
>> Is it ok for trunk?
>>
>> BR,
>> Kewen
>> -----
>> gcc/ChangeLog:
>>
>> PR target/102767
>> * config/rs6000/rs6000.c (rs6000_builtin_vectorization_cost):
>> Consider
>> V1T1 mode for unaligned load and store.
>>
>> gcc/testsuite/ChangeLog:
>>
>> PR target/102767
>> * gcc.target/powerpc/ppc-fortran/pr102767.f90: New file.
>>
>> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
>> index b7ea1483da5..73d3e06c3fc 100644
>> --- a/gcc/config/rs6000/rs6000.c
>> +++ b/gcc/config/rs6000/rs6000.c
>> @@ -5145,7 +5145,8 @@ rs6000_builtin_vectorization_cost (enum
>> vect_cost_for_stmt type_of_cost,
>> if (TARGET_VSX && TARGET_ALLOW_MOVMISALIGN)
>> {
>> elements = TYPE_VECTOR_SUBPARTS (vectype);
>> - if (elements == 2)
>> + /* See PR102767, consider V1TI to keep consistency. */
>> + if (elements == 2 || elements == 1)
>> /* Double word aligned. */
>> return 4;
>>
>> @@ -5184,10 +5185,11 @@ rs6000_builtin_vectorization_cost (enum
>> vect_cost_for_stmt type_of_cost,
>>
>> if (TARGET_VSX && TARGET_ALLOW_MOVMISALIGN)
>> {
>> - elements = TYPE_VECTOR_SUBPARTS (vectype);
>> - if (elements == 2)
>> - /* Double word aligned. */
>> - return 2;
>> + elements = TYPE_VECTOR_SUBPARTS (vectype);
>> + /* See PR102767, consider V1TI to keep consistency. */
>> + if (elements == 2 || elements == 1)
>> + /* Double word aligned. */
>> + return 2;
>
> This section of the patch incorrectly changes the indentation. Please
> use the correct indentation.
>
The indentation change is intentional since the original identation is
wrong (more than 8 spaces leading the lines), there are more wrong
identation lines above the first changed line, but I thought it seems a
bad idea to fix them too when they are unrelated to what this patch
wants to fix, so I left them alone.
With the above clarification, may I push this patch without any updates
for the mentioned indentation issue?
>>
>> if (elements == 4)
>> {
>> diff --git a/gcc/testsuite/gcc.target/powerpc/ppc-fortran/pr102767.f90
>> b/gcc/testsuite/gcc.target/powerpc/ppc-fortran/pr102767.f90
>> new file mode 100644
>> index 00000000000..a4122482989
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/ppc-fortran/pr102767.f90
>> @@ -0,0 +1,21 @@
>> +! { dg-require-effective-target powerpc_vsx_ok }
>> +! { dg-options "-mvsx -O2 -ftree-vectorize -mno-efficient-unaligned-vsx" }
>> +
>> +INTERFACE
>> + FUNCTION elemental_mult (a, b, c)
>> + type(*), DIMENSION(..) :: a, b, c
>> + END
>> +END INTERFACE
>> +
>> +allocatable z
>> +integer, dimension(2,2) :: a, b
>> +call test_CFI_address
>> +contains
>> + subroutine test_CFI_address
>> + if (elemental_mult (z, x, y) .ne. 0) stop
>> + a = reshape ([4,3,2,1], [2,2])
>> + b = reshape ([2,3,4,5], [2,2])
>> + if (elemental_mult (i, a, b) .ne. 0) stop
>> + end
>> +end
>> +
>>
>
> The patch is okay with the indentation correction.
>
> Thanks, David
>
Thanks!
BR,
Kewen
