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

["Kewen.Lin" <linkw@linux.ibm.com>]

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