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