rs6000: Fix ICE of vect cost related to V1TI [PR102767]

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.

On Sun, Oct 24, 2021 at 11:04 PM Kewen.Lin <linkw@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.

>
>              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

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 <linkw@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

On Wed, Oct 27, 2021 at 9:30 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>
> 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 <linkw@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 you correct the indentation, you should adjust it for the entire
block, not just the lines that you change.  If you want to fix the
entire block to TAB+spaces as well, okay.  You didn't mention that you
were fixing the indentation in the explanation of the patch.

Thank, David

>
> >>
> >>              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

on 2021/10/28 上午9:43, David Edelsohn wrote:
> On Wed, Oct 27, 2021 at 9:30 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>
>> 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 <linkw@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 you correct the indentation, you should adjust it for the entire
> block, not just the lines that you change.  If you want to fix the
> entire block to TAB+spaces as well, okay.  You didn't mention that you
> were fixing the indentation in the explanation of the patch.
> 

Sorry for not mentioning that.  Got it, I'll reformat the entire block then,
also with additional notes in the commit log.

Thanks again.

BR,
Kewen

> Thank, David
> 
>>
>>>>
>>>>              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