* [PATCH] rs6000: Preserve REG_EH_REGION when replacing load/store [PR106091]
@ 2022-07-07 8:55 Kewen.Lin
2022-07-07 9:03 ` Richard Biener
0 siblings, 1 reply; 3+ messages in thread
From: Kewen.Lin @ 2022-07-07 8:55 UTC (permalink / raw)
To: GCC Patches; +Cc: Segher Boessenkool, David Edelsohn, Richard Biener
Hi,
As test case in PR106091 shows, rs6000 specific pass swaps
doesn't preserve the reg_note REG_EH_REGION when replacing
some load insn at the end of basic block, it causes the
flow info verification to fail unexpectedly. Since memory
reference rtx may trap, this patch is to ensure we copy
REG_EH_REGION reg_note while replacing swapped aligned load
or store.
Bootstrapped and regtested on powerpc64-linux-gnu P7 & P8,
and powerpc64le-linux-gnu P9 & P10.
Richi, could you help to review this patch from a point view
of non-call-exceptions expert?
I'm going to install it if it looks good to you. Thanks!
-----
PR target/106091
gcc/ChangeLog:
* config/rs6000/rs6000-p8swap.cc (replace_swapped_aligned_store): Copy
REG_EH_REGION when replacing one store insn having it.
(replace_swapped_aligned_load): Likewise.
gcc/testsuite/ChangeLog:
* gcc.target/powerpc/pr106091.c: New test.
---
gcc/config/rs6000/rs6000-p8swap.cc | 20 ++++++++++++++++++--
gcc/testsuite/gcc.target/powerpc/pr106091.c | 15 +++++++++++++++
2 files changed, 33 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106091.c
diff --git a/gcc/config/rs6000/rs6000-p8swap.cc b/gcc/config/rs6000/rs6000-p8swap.cc
index 275702fee1b..19fbbfb67dc 100644
--- a/gcc/config/rs6000/rs6000-p8swap.cc
+++ b/gcc/config/rs6000/rs6000-p8swap.cc
@@ -1690,7 +1690,15 @@ replace_swapped_aligned_store (swap_web_entry *insn_entry,
gcc_assert ((GET_CODE (new_body) == SET)
&& MEM_P (SET_DEST (new_body)));
- set_block_for_insn (new_insn, BLOCK_FOR_INSN (store_insn));
+ basic_block bb = BLOCK_FOR_INSN (store_insn);
+ set_block_for_insn (new_insn, bb);
+ /* Handle REG_EH_REGION note. */
+ if (cfun->can_throw_non_call_exceptions && BB_END (bb) == store_insn)
+ {
+ rtx note = find_reg_note (store_insn, REG_EH_REGION, NULL_RTX);
+ if (note)
+ add_reg_note (new_insn, REG_EH_REGION, XEXP (note, 0));
+ }
df_insn_rescan (new_insn);
df_insn_delete (store_insn);
@@ -1784,7 +1792,15 @@ replace_swapped_aligned_load (swap_web_entry *insn_entry, rtx swap_insn)
gcc_assert ((GET_CODE (new_body) == SET)
&& MEM_P (SET_SRC (new_body)));
- set_block_for_insn (new_insn, BLOCK_FOR_INSN (def_insn));
+ basic_block bb = BLOCK_FOR_INSN (def_insn);
+ set_block_for_insn (new_insn, bb);
+ /* Handle REG_EH_REGION note. */
+ if (cfun->can_throw_non_call_exceptions && BB_END (bb) == def_insn)
+ {
+ rtx note = find_reg_note (def_insn, REG_EH_REGION, NULL_RTX);
+ if (note)
+ add_reg_note (new_insn, REG_EH_REGION, XEXP (note, 0));
+ }
df_insn_rescan (new_insn);
df_insn_delete (def_insn);
diff --git a/gcc/testsuite/gcc.target/powerpc/pr106091.c b/gcc/testsuite/gcc.target/powerpc/pr106091.c
new file mode 100644
index 00000000000..61ce8cf4733
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr106091.c
@@ -0,0 +1,15 @@
+/* { dg-options "-O -fnon-call-exceptions -fno-tree-dce -fno-tree-forwprop -w" } */
+
+/* Verify there is no ICE. */
+
+typedef short __attribute__ ((__vector_size__ (64))) V;
+V v, w;
+
+inline V foo (V a, V b);
+
+V
+foo (V a, V b)
+{
+ b &= v < b;
+ return (V){foo (b, w)[3], (V){}[3]};
+}
--
2.25.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] rs6000: Preserve REG_EH_REGION when replacing load/store [PR106091]
2022-07-07 8:55 [PATCH] rs6000: Preserve REG_EH_REGION when replacing load/store [PR106091] Kewen.Lin
@ 2022-07-07 9:03 ` Richard Biener
2022-07-07 9:17 ` Kewen.Lin
0 siblings, 1 reply; 3+ messages in thread
From: Richard Biener @ 2022-07-07 9:03 UTC (permalink / raw)
To: Kewen.Lin, Eric Botcazou; +Cc: GCC Patches, Segher Boessenkool, David Edelsohn
On Thu, Jul 7, 2022 at 10:55 AM Kewen.Lin <linkw@linux.ibm.com> wrote:
>
> Hi,
>
> As test case in PR106091 shows, rs6000 specific pass swaps
> doesn't preserve the reg_note REG_EH_REGION when replacing
> some load insn at the end of basic block, it causes the
> flow info verification to fail unexpectedly. Since memory
> reference rtx may trap, this patch is to ensure we copy
> REG_EH_REGION reg_note while replacing swapped aligned load
> or store.
>
> Bootstrapped and regtested on powerpc64-linux-gnu P7 & P8,
> and powerpc64le-linux-gnu P9 & P10.
>
> Richi, could you help to review this patch from a point view
> of non-call-exceptions expert?
I think it looks OK but I do wonder if in RTL there's a better
way to transfer EH info from one stmt to another when you
are replacing it? On gimple gsi_replace would do, but I
can't immediately find a proper RTL replacement for your
emit_insn_before (..., X); remove_insn (X); (plus DF assorted
things).
Eric?
>
> I'm going to install it if it looks good to you. Thanks!
>
> -----
> PR target/106091
>
> gcc/ChangeLog:
>
> * config/rs6000/rs6000-p8swap.cc (replace_swapped_aligned_store): Copy
> REG_EH_REGION when replacing one store insn having it.
> (replace_swapped_aligned_load): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/powerpc/pr106091.c: New test.
> ---
> gcc/config/rs6000/rs6000-p8swap.cc | 20 ++++++++++++++++++--
> gcc/testsuite/gcc.target/powerpc/pr106091.c | 15 +++++++++++++++
> 2 files changed, 33 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106091.c
>
> diff --git a/gcc/config/rs6000/rs6000-p8swap.cc b/gcc/config/rs6000/rs6000-p8swap.cc
> index 275702fee1b..19fbbfb67dc 100644
> --- a/gcc/config/rs6000/rs6000-p8swap.cc
> +++ b/gcc/config/rs6000/rs6000-p8swap.cc
> @@ -1690,7 +1690,15 @@ replace_swapped_aligned_store (swap_web_entry *insn_entry,
> gcc_assert ((GET_CODE (new_body) == SET)
> && MEM_P (SET_DEST (new_body)));
>
> - set_block_for_insn (new_insn, BLOCK_FOR_INSN (store_insn));
> + basic_block bb = BLOCK_FOR_INSN (store_insn);
> + set_block_for_insn (new_insn, bb);
> + /* Handle REG_EH_REGION note. */
> + if (cfun->can_throw_non_call_exceptions && BB_END (bb) == store_insn)
> + {
> + rtx note = find_reg_note (store_insn, REG_EH_REGION, NULL_RTX);
> + if (note)
> + add_reg_note (new_insn, REG_EH_REGION, XEXP (note, 0));
> + }
> df_insn_rescan (new_insn);
>
> df_insn_delete (store_insn);
> @@ -1784,7 +1792,15 @@ replace_swapped_aligned_load (swap_web_entry *insn_entry, rtx swap_insn)
> gcc_assert ((GET_CODE (new_body) == SET)
> && MEM_P (SET_SRC (new_body)));
>
> - set_block_for_insn (new_insn, BLOCK_FOR_INSN (def_insn));
> + basic_block bb = BLOCK_FOR_INSN (def_insn);
> + set_block_for_insn (new_insn, bb);
> + /* Handle REG_EH_REGION note. */
> + if (cfun->can_throw_non_call_exceptions && BB_END (bb) == def_insn)
> + {
> + rtx note = find_reg_note (def_insn, REG_EH_REGION, NULL_RTX);
> + if (note)
> + add_reg_note (new_insn, REG_EH_REGION, XEXP (note, 0));
> + }
> df_insn_rescan (new_insn);
>
> df_insn_delete (def_insn);
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr106091.c b/gcc/testsuite/gcc.target/powerpc/pr106091.c
> new file mode 100644
> index 00000000000..61ce8cf4733
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr106091.c
> @@ -0,0 +1,15 @@
> +/* { dg-options "-O -fnon-call-exceptions -fno-tree-dce -fno-tree-forwprop -w" } */
> +
> +/* Verify there is no ICE. */
> +
> +typedef short __attribute__ ((__vector_size__ (64))) V;
> +V v, w;
> +
> +inline V foo (V a, V b);
> +
> +V
> +foo (V a, V b)
> +{
> + b &= v < b;
> + return (V){foo (b, w)[3], (V){}[3]};
> +}
> --
> 2.25.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] rs6000: Preserve REG_EH_REGION when replacing load/store [PR106091]
2022-07-07 9:03 ` Richard Biener
@ 2022-07-07 9:17 ` Kewen.Lin
0 siblings, 0 replies; 3+ messages in thread
From: Kewen.Lin @ 2022-07-07 9:17 UTC (permalink / raw)
To: Richard Biener, Eric Botcazou
Cc: GCC Patches, Segher Boessenkool, David Edelsohn
on 2022/7/7 17:03, Richard Biener wrote:
> On Thu, Jul 7, 2022 at 10:55 AM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>
>> Hi,
>>
>> As test case in PR106091 shows, rs6000 specific pass swaps
>> doesn't preserve the reg_note REG_EH_REGION when replacing
>> some load insn at the end of basic block, it causes the
>> flow info verification to fail unexpectedly. Since memory
>> reference rtx may trap, this patch is to ensure we copy
>> REG_EH_REGION reg_note while replacing swapped aligned load
>> or store.
>>
>> Bootstrapped and regtested on powerpc64-linux-gnu P7 & P8,
>> and powerpc64le-linux-gnu P9 & P10.
>>
>> Richi, could you help to review this patch from a point view
>> of non-call-exceptions expert?
>
> I think it looks OK but I do wonder if in RTL there's a better
> way to transfer EH info from one stmt to another when you
> are replacing it? On gimple gsi_replace would do, but I
> can't immediately find a proper RTL replacement for your
> emit_insn_before (..., X); remove_insn (X); (plus DF assorted
> things).
>
Thanks for so prompt review! For the question, I'm not sure :(,
when I was drafting this patch, I wondered if there is one function
passing/copying reg_note REG_EH_REGION for this kind of need,
so I went through almost all the places related to REG_EH_REGION,
but nothing desired was found (though I may miss sth.).
BR,
Kewen
> Eric?
>
>>
>> I'm going to install it if it looks good to you. Thanks!
>>
>> -----
>> PR target/106091
>>
>> gcc/ChangeLog:
>>
>> * config/rs6000/rs6000-p8swap.cc (replace_swapped_aligned_store): Copy
>> REG_EH_REGION when replacing one store insn having it.
>> (replace_swapped_aligned_load): Likewise.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * gcc.target/powerpc/pr106091.c: New test.
>> ---
>> gcc/config/rs6000/rs6000-p8swap.cc | 20 ++++++++++++++++++--
>> gcc/testsuite/gcc.target/powerpc/pr106091.c | 15 +++++++++++++++
>> 2 files changed, 33 insertions(+), 2 deletions(-)
>> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr106091.c
>>
>> diff --git a/gcc/config/rs6000/rs6000-p8swap.cc b/gcc/config/rs6000/rs6000-p8swap.cc
>> index 275702fee1b..19fbbfb67dc 100644
>> --- a/gcc/config/rs6000/rs6000-p8swap.cc
>> +++ b/gcc/config/rs6000/rs6000-p8swap.cc
>> @@ -1690,7 +1690,15 @@ replace_swapped_aligned_store (swap_web_entry *insn_entry,
>> gcc_assert ((GET_CODE (new_body) == SET)
>> && MEM_P (SET_DEST (new_body)));
>>
>> - set_block_for_insn (new_insn, BLOCK_FOR_INSN (store_insn));
>> + basic_block bb = BLOCK_FOR_INSN (store_insn);
>> + set_block_for_insn (new_insn, bb);
>> + /* Handle REG_EH_REGION note. */
>> + if (cfun->can_throw_non_call_exceptions && BB_END (bb) == store_insn)
>> + {
>> + rtx note = find_reg_note (store_insn, REG_EH_REGION, NULL_RTX);
>> + if (note)
>> + add_reg_note (new_insn, REG_EH_REGION, XEXP (note, 0));
>> + }
>> df_insn_rescan (new_insn);
>>
>> df_insn_delete (store_insn);
>> @@ -1784,7 +1792,15 @@ replace_swapped_aligned_load (swap_web_entry *insn_entry, rtx swap_insn)
>> gcc_assert ((GET_CODE (new_body) == SET)
>> && MEM_P (SET_SRC (new_body)));
>>
>> - set_block_for_insn (new_insn, BLOCK_FOR_INSN (def_insn));
>> + basic_block bb = BLOCK_FOR_INSN (def_insn);
>> + set_block_for_insn (new_insn, bb);
>> + /* Handle REG_EH_REGION note. */
>> + if (cfun->can_throw_non_call_exceptions && BB_END (bb) == def_insn)
>> + {
>> + rtx note = find_reg_note (def_insn, REG_EH_REGION, NULL_RTX);
>> + if (note)
>> + add_reg_note (new_insn, REG_EH_REGION, XEXP (note, 0));
>> + }
>> df_insn_rescan (new_insn);
>>
>> df_insn_delete (def_insn);
>> diff --git a/gcc/testsuite/gcc.target/powerpc/pr106091.c b/gcc/testsuite/gcc.target/powerpc/pr106091.c
>> new file mode 100644
>> index 00000000000..61ce8cf4733
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/pr106091.c
>> @@ -0,0 +1,15 @@
>> +/* { dg-options "-O -fnon-call-exceptions -fno-tree-dce -fno-tree-forwprop -w" } */
>> +
>> +/* Verify there is no ICE. */
>> +
>> +typedef short __attribute__ ((__vector_size__ (64))) V;
>> +V v, w;
>> +
>> +inline V foo (V a, V b);
>> +
>> +V
>> +foo (V a, V b)
>> +{
>> + b &= v < b;
>> + return (V){foo (b, w)[3], (V){}[3]};
>> +}
>> --
>> 2.25.1
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-07-07 9:17 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-07 8:55 [PATCH] rs6000: Preserve REG_EH_REGION when replacing load/store [PR106091] Kewen.Lin
2022-07-07 9:03 ` Richard Biener
2022-07-07 9:17 ` 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).