From: "Kewen.Lin" <linkw@linux.ibm.com>
To: David Edelsohn <dje.gcc@gmail.com>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>,
Segher Boessenkool <segher@kernel.crashing.org>,
Bill Schmidt <wschmidt@linux.ibm.com>
Subject: Re: [PATCH] rs6000: Fix ICE of vect cost related to V1TI [PR102767]
Date: Thu, 28 Oct 2021 09:52:58 +0800 [thread overview]
Message-ID: <b8da3a5e-6809-38ba-33d5-d503dda41c90@linux.ibm.com> (raw)
In-Reply-To: <CAGWvnynbOBBkMyiCOA6yBtGgouVbf3tMSge7usYCOzWqgJrRJA@mail.gmail.com>
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
prev parent reply other threads:[~2021-10-28 1:53 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-10-25 3:04 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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b8da3a5e-6809-38ba-33d5-d503dda41c90@linux.ibm.com \
--to=linkw@linux.ibm.com \
--cc=dje.gcc@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=segher@kernel.crashing.org \
--cc=wschmidt@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).