Message ID | 1af9d697-722c-de5b-9968-4c579d04e717@linux.ibm.com |
---|---|
State | New |
Headers | show |
Series | rs6000: Preserve REG_EH_REGION when replacing load/store [PR106091] | expand |
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
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
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]}; +}