Re: [PATCH] rs6000: Fix ICE of vect cost related to V1TI [PR102767]
On Sun, Oct 24, 2021 at 11:04 PM Kewen.Lin 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"), 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 000..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
Re: [PATCH] rs6000: Fix ICE of vect cost related to V1TI [PR102767]
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 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"), 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 000..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
Re: [PATCH] rs6000: Fix ICE of vect cost related to V1TI [PR102767]
On Wed, Oct 27, 2021 at 9:30 PM Kewen.Lin 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 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"), 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 000..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
Re: [PATCH] rs6000: Fix ICE of vect cost related to V1TI [PR102767]
on 2021/10/28 上午9:43, David Edelsohn wrote: > On Wed, Oct 27, 2021 at 9:30 PM Kewen.Lin 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 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"), 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 000..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 indentatio