[v3] Mode-Switching: Fix SET_SRC ICE when CLOBBER insn

  From: Pan Li <pan2.li@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.li@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
  

On 8/8/23 21:05, pan2.li@intel.com wrote:
> From: Pan Li <pan2.li@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.li@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
  

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 <jeffreyalaw@gmail.com> 
Sent: Saturday, August 12, 2023 4:57 AM
To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>
Subject: Re: [PATCH v3] Mode-Switching: Fix SET_SRC ICE when CLOBBER insn



On 8/8/23 21:05, pan2.li@intel.com wrote:
> From: Pan Li <pan2.li@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.li@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