Re: [PATCH] rs6000: Preserve REG_EH_REGION when replacing load/store [PR106091]

[Richard Biener <richard.guenther@gmail.com>]

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