Thanks Jeff for comments.
> I'd put it in that code. Probably something like
> && GET_CODE (return_copy_pat) = SET
> && OBJECT_P (SET_SRC (return_copy_pat)))
> That way we make it clear that we should only be looking at SET_SRC of
> an actual SET.
> Is there some reason you put the guard earlier?
It looks like the code under "if (j >= 0)" only care about set insns, but I am
not 100% confirmed this as the
existing comments don't have a explicit description about this.
I will have a try for your suggestion and will set the v4 if no surprise from
tests.
Pan
-----Original Message-----
From: Jeff Law <jeffreya...@gmail.com>
Sent: Saturday, August 12, 2023 4:57 AM
To: Li, Pan2 <pan2...@intel.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zh...@rivai.ai; kito.ch...@sifive.com; Wang, Yanzhang
<yanzhang.w...@intel.com>
Subject: Re: [PATCH v3] Mode-Switching: Fix SET_SRC ICE when CLOBBER insn
On 8/8/23 21:05, pan2...@intel.com wrote:
> From: Pan Li <pan2...@intel.com>
>
> In same cases, like gcc/testsuite/gcc.dg/pr78148.c in RISC-V, there will
> be only 1 operand when SET_SRC in create_pre_exit. For example as below.
>
> (insn 13 9 14 2 (clobber (reg/i:TI 10 a0))
> "gcc/testsuite/gcc.dg/pr78148.c":24:1 -1
> (expr_list:REG_UNUSED (reg/i:TI 10 a0)
> (nil)))
>
> Unfortunately, SET_SRC requires at least 2 operands and then Segment
> Fault here. For SH4 part result in Segment Fault, it looks like only
> valid when the return_copy_pat is load or something like that. Thus,
> this patch try to fix it by ingnoring the CLOBBER insn for SH4.
>
> Signed-off-by: Pan Li <pan2...@intel.com>
>
> gcc/ChangeLog:
>
> * mode-switching.cc (create_pre_exit): Add CLOBBER check.
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/riscv/mode-switch-ice-1.c: New test.
> ---
> gcc/mode-switching.cc | 2 +-
> .../gcc.target/riscv/mode-switch-ice-1.c | 22 +++++++++++++++++++
> 2 files changed, 23 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
>
> diff --git a/gcc/mode-switching.cc b/gcc/mode-switching.cc
> index 64ae2bc29c3..b034cf7d437 100644
> --- a/gcc/mode-switching.cc
> +++ b/gcc/mode-switching.cc
> @@ -392,7 +392,7 @@ create_pre_exit (int n_entities, int *entity_map, const
> int *num_modes)
> && mode != targetm.mode_switching.exit (e))
> break;
> }
> - if (j >= 0)
> + if (j >= 0 && GET_CODE (return_copy_pat) != CLOBBER)
> {
> /* __builtin_return emits a sequence of loads to all
> return registers. One of them might require
I'd tend to prefer to guard the code a bit later so that the test for
CLOBBERS is closer to the point where they're not allowed. ie
>
> /* For the SH4, floating point loads depend on fpscr,
> thus we might need to put the final mode switch
> after the return value copy. That is still OK,
> because a floating point return value does not
> conflict with address reloads. */
> if (copy_start >= ret_start
> && copy_start + copy_num <= ret_end
> && OBJECT_P (SET_SRC (return_copy_pat)))
> forced_late_switch = true;
> break;
I'd put it in that code. Probably something like
&& GET_CODE (return_copy_pat) = SET
&& OBJECT_P (SET_SRC (return_copy_pat)))
That way we make it clear that we should only be looking at SET_SRC of
an actual SET.
Is there some reason you put the guard earlier?
jeff
