public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] rs6000: Fix ICE of vect cost related to V1TI [PR102767]
@ 2021-10-25  3:04 Kewen.Lin
  2021-10-27 13:12 ` David Edelsohn
  0 siblings, 1 reply; 5+ messages in thread
From: Kewen.Lin @ 2021-10-25  3:04 UTC (permalink / raw)
  To: GCC Patches; +Cc: Segher Boessenkool, David Edelsohn, Bill Schmidt

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;

             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
+


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rs6000: Fix ICE of vect cost related to V1TI [PR102767]
  2021-10-25  3:04 [PATCH] rs6000: Fix ICE of vect cost related to V1TI [PR102767] Kewen.Lin
@ 2021-10-27 13:12 ` David Edelsohn
  2021-10-28  1:30   ` Kewen.Lin
  0 siblings, 1 reply; 5+ messages in thread
From: David Edelsohn @ 2021-10-27 13:12 UTC (permalink / raw)
  To: Kewen.Lin; +Cc: GCC Patches, Segher Boessenkool, Bill Schmidt

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rs6000: Fix ICE of vect cost related to V1TI [PR102767]
  2021-10-27 13:12 ` David Edelsohn
@ 2021-10-28  1:30   ` Kewen.Lin
  2021-10-28  1:43     ` David Edelsohn
  0 siblings, 1 reply; 5+ messages in thread
From: Kewen.Lin @ 2021-10-28  1:30 UTC (permalink / raw)
  To: David Edelsohn; +Cc: GCC Patches, Segher Boessenkool, Bill Schmidt

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rs6000: Fix ICE of vect cost related to V1TI [PR102767]
  2021-10-28  1:30   ` Kewen.Lin
@ 2021-10-28  1:43     ` David Edelsohn
  2021-10-28  1:52       ` Kewen.Lin
  0 siblings, 1 reply; 5+ messages in thread
From: David Edelsohn @ 2021-10-28  1:43 UTC (permalink / raw)
  To: Kewen.Lin; +Cc: GCC Patches, Segher Boessenkool, Bill Schmidt

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] rs6000: Fix ICE of vect cost related to V1TI [PR102767]
  2021-10-28  1:43     ` David Edelsohn
@ 2021-10-28  1:52       ` Kewen.Lin
  0 siblings, 0 replies; 5+ messages in thread
From: Kewen.Lin @ 2021-10-28  1:52 UTC (permalink / raw)
  To: David Edelsohn; +Cc: GCC Patches, Segher Boessenkool, Bill Schmidt

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-10-28  1:53 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-25  3:04 [PATCH] rs6000: Fix ICE of vect cost related to V1TI [PR102767] Kewen.Lin
2021-10-27 13:12 ` David Edelsohn
2021-10-28  1:30   ` Kewen.Lin
2021-10-28  1:43     ` David Edelsohn
2021-10-28  1:52       ` Kewen.Lin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).