[PATCH v1] Mode-Switching: Fix SET_SRC ICE when only one operand

[PATCH v1] Mode-Switching: Fix SET_SRC ICE when only one operand

[pan2.li]

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. This patch would like to fix this ICE by adding operand
length check before SET_SRC.

Signed-off-by: Pan Li <pan2.li@intel.com>

gcc/ChangeLog:

	* mode-switching.cc (create_pre_exit): Add operand length 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, 24 insertions(+)
 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..cbf03c02159 100644
--- a/gcc/mode-switching.cc
+++ b/gcc/mode-switching.cc
@@ -411,6 +411,8 @@ create_pre_exit (int n_entities, int *entity_map, const int *num_modes)
 			   conflict with address reloads.  */
 			if (copy_start >= ret_start
 			    && copy_start + copy_num <= ret_end
+			    && GET_RTX_LENGTH (GET_CODE (return_copy_pat)) >= 2
+			    /* SET_SRC requires at least 2 operands.  */
 			    && OBJECT_P (SET_SRC (return_copy_pat)))
 			  forced_late_switch = true;
 			break;
diff --git a/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
new file mode 100644
index 00000000000..1b34a471904
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+struct A { char e, f; };
+
+struct B
+{
+  int g;
+  struct A h[4];
+};
+
+extern void bar (int, int);
+
+struct B foo (void)
+{
+  bar (2, 1);
+}
+
+void baz ()
+{
+  foo ();
+}
-- 
2.34.1

Re: [PATCH v1] Mode-Switching: Fix SET_SRC ICE when only one operand

[Richard Biener]

On Mon, Aug 7, 2023 at 2:30 PM Pan Li via Gcc-patches
<gcc-patches@gcc.gnu.org> 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. This patch would like to fix this ICE by adding operand
> length check before SET_SRC.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
>
> gcc/ChangeLog:
>
>         * mode-switching.cc (create_pre_exit): Add operand length 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, 24 insertions(+)
>  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..cbf03c02159 100644
> --- a/gcc/mode-switching.cc
> +++ b/gcc/mode-switching.cc
> @@ -411,6 +411,8 @@ create_pre_exit (int n_entities, int *entity_map, const int *num_modes)
>                            conflict with address reloads.  */
>                         if (copy_start >= ret_start
>                             && copy_start + copy_num <= ret_end
> +                           && GET_RTX_LENGTH (GET_CODE (return_copy_pat)) >= 2
> +                           /* SET_SRC requires at least 2 operands.  */

maybe check that return_copy_pat is a SET instead?

>                             && OBJECT_P (SET_SRC (return_copy_pat)))
>                           forced_late_switch = true;
>                         break;
> diff --git a/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
> new file mode 100644
> index 00000000000..1b34a471904
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
> @@ -0,0 +1,22 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +struct A { char e, f; };
> +
> +struct B
> +{
> +  int g;
> +  struct A h[4];
> +};
> +
> +extern void bar (int, int);
> +
> +struct B foo (void)
> +{
> +  bar (2, 1);
> +}
> +
> +void baz ()
> +{
> +  foo ();
> +}
> --
> 2.34.1
>

RE: [PATCH v1] Mode-Switching: Fix SET_SRC ICE when only one operand

[Li, Pan2]

> maybe check that return_copy_pat is a SET instead?

You mean something like this? I tried this but I am not sure single_set is good enough here, because it should be "narrow" than GET_RTX_LENGTH.

+                           && single_set (return_copy)

Pan

-----Original Message-----
From: Richard Biener <richard.guenther@gmail.com> 
Sent: Monday, August 7, 2023 9:14 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>; jeffreyalaw@gmail.com
Subject: Re: [PATCH v1] Mode-Switching: Fix SET_SRC ICE when only one operand

On Mon, Aug 7, 2023 at 2:30 PM Pan Li via Gcc-patches
<gcc-patches@gcc.gnu.org> 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. This patch would like to fix this ICE by adding operand
> length check before SET_SRC.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
>
> gcc/ChangeLog:
>
>         * mode-switching.cc (create_pre_exit): Add operand length 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, 24 insertions(+)
>  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..cbf03c02159 100644
> --- a/gcc/mode-switching.cc
> +++ b/gcc/mode-switching.cc
> @@ -411,6 +411,8 @@ create_pre_exit (int n_entities, int *entity_map, const int *num_modes)
>                            conflict with address reloads.  */
>                         if (copy_start >= ret_start
>                             && copy_start + copy_num <= ret_end
> +                           && GET_RTX_LENGTH (GET_CODE (return_copy_pat)) >= 2
> +                           /* SET_SRC requires at least 2 operands.  */

maybe check that return_copy_pat is a SET instead?

>                             && OBJECT_P (SET_SRC (return_copy_pat)))
>                           forced_late_switch = true;
>                         break;
> diff --git a/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
> new file mode 100644
> index 00000000000..1b34a471904
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
> @@ -0,0 +1,22 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +struct A { char e, f; };
> +
> +struct B
> +{
> +  int g;
> +  struct A h[4];
> +};
> +
> +extern void bar (int, int);
> +
> +struct B foo (void)
> +{
> +  bar (2, 1);
> +}
> +
> +void baz ()
> +{
> +  foo ();
> +}
> --
> 2.34.1
>

Re: [PATCH v1] Mode-Switching: Fix SET_SRC ICE when only one operand

[Jeff Law]

On 8/7/23 07:13, Richard Biener wrote:
> On Mon, Aug 7, 2023 at 2:30 PM Pan Li via Gcc-patches
> <gcc-patches@gcc.gnu.org> 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. This patch would like to fix this ICE by adding operand
>> length check before SET_SRC.
>>
>> Signed-off-by: Pan Li <pan2.li@intel.com>
>>
>> gcc/ChangeLog:
>>
>>          * mode-switching.cc (create_pre_exit): Add operand length 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, 24 insertions(+)
>>   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..cbf03c02159 100644
>> --- a/gcc/mode-switching.cc
>> +++ b/gcc/mode-switching.cc
>> @@ -411,6 +411,8 @@ create_pre_exit (int n_entities, int *entity_map, const int *num_modes)
>>                             conflict with address reloads.  */
>>                          if (copy_start >= ret_start
>>                              && copy_start + copy_num <= ret_end
>> +                           && GET_RTX_LENGTH (GET_CODE (return_copy_pat)) >= 2
>> +                           /* SET_SRC requires at least 2 operands.  */
> 
> maybe check that return_copy_pat is a SET instead?
Or just ignore the case of an insn that is a toplevel USE/CLOBBER?  I 
bet we even have a function for that somewhere given how commonly we 
need to do that.

jeff

[PATCH v2] Mode-Switching: Fix SET_SRC ICE when USE or CLOBBER

[pan2.li]

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. This patch would like to fix this ICE by ignoring the USE
and CLOBBER insn before SET_SRC.

Signed-off-by: Pan Li <pan2.li@intel.com>

gcc/ChangeLog:

	* mode-switching.cc (create_pre_exit): Add USE and CLOBBER check.
	* rtl.h (CLOBBER_OR_USE_P): New macro.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/mode-switch-ice-1.c: New test.
---
 gcc/mode-switching.cc                         |  1 +
 gcc/rtl.h                                     |  4 ++++
 .../gcc.target/riscv/mode-switch-ice-1.c      | 22 +++++++++++++++++++
 3 files changed, 27 insertions(+)
 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..8bb016a811d 100644
--- a/gcc/mode-switching.cc
+++ b/gcc/mode-switching.cc
@@ -411,6 +411,7 @@ create_pre_exit (int n_entities, int *entity_map, const int *num_modes)
 			   conflict with address reloads.  */
 			if (copy_start >= ret_start
 			    && copy_start + copy_num <= ret_end
+			    && !CLOBBER_OR_USE_P (return_copy_pat)
 			    && OBJECT_P (SET_SRC (return_copy_pat)))
 			  forced_late_switch = true;
 			break;
diff --git a/gcc/rtl.h b/gcc/rtl.h
index e1c51156f90..cdc2941c210 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -1768,6 +1768,10 @@ extern const char * const reg_note_name[];
 #define VAR_LOC_UNKNOWN_P(X) \
   (GET_CODE (X) == CLOBBER && XEXP ((X), 0) == const0_rtx)
 
+/* Determine whether RTX is USE or CLOBBER.  */
+#define CLOBBER_OR_USE_P(RTX) \
+  (GET_CODE (RTX) == USE || GET_CODE (RTX) == CLOBBER)
+
 /* 1 if RTX is emitted after a call, but it should take effect before
    the call returns.  */
 #define NOTE_DURING_CALL_P(RTX)				\
diff --git a/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
new file mode 100644
index 00000000000..1b34a471904
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+struct A { char e, f; };
+
+struct B
+{
+  int g;
+  struct A h[4];
+};
+
+extern void bar (int, int);
+
+struct B foo (void)
+{
+  bar (2, 1);
+}
+
+void baz ()
+{
+  foo ();
+}
-- 
2.34.1

RE: [PATCH v1] Mode-Switching: Fix SET_SRC ICE when only one operand

[Li, Pan2]

Thanks Jeff. 

> Or just ignore the case of an insn that is a toplevel USE/CLOBBER?  I 
> bet we even have a function for that somewhere given how commonly we 
> need to do that.

 I found there are something like "GET_CODE(x) == USE || GET_CODE(x) == CLOBBER" everywhere but lack of a function or macro. Thus, I add new macro for this as below v2.

https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626590.html

Pan

-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com> 
Sent: Tuesday, August 8, 2023 12:31 AM
To: Richard Biener <richard.guenther@gmail.com>; Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>
Subject: Re: [PATCH v1] Mode-Switching: Fix SET_SRC ICE when only one operand



On 8/7/23 07:13, Richard Biener wrote:
> On Mon, Aug 7, 2023 at 2:30 PM Pan Li via Gcc-patches
> <gcc-patches@gcc.gnu.org> 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. This patch would like to fix this ICE by adding operand
>> length check before SET_SRC.
>>
>> Signed-off-by: Pan Li <pan2.li@intel.com>
>>
>> gcc/ChangeLog:
>>
>>          * mode-switching.cc (create_pre_exit): Add operand length 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, 24 insertions(+)
>>   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..cbf03c02159 100644
>> --- a/gcc/mode-switching.cc
>> +++ b/gcc/mode-switching.cc
>> @@ -411,6 +411,8 @@ create_pre_exit (int n_entities, int *entity_map, const int *num_modes)
>>                             conflict with address reloads.  */
>>                          if (copy_start >= ret_start
>>                              && copy_start + copy_num <= ret_end
>> +                           && GET_RTX_LENGTH (GET_CODE (return_copy_pat)) >= 2
>> +                           /* SET_SRC requires at least 2 operands.  */
> 
> maybe check that return_copy_pat is a SET instead?
Or just ignore the case of an insn that is a toplevel USE/CLOBBER?  I 
bet we even have a function for that somewhere given how commonly we 
need to do that.

jeff

Re: [PATCH v2] Mode-Switching: Fix SET_SRC ICE when USE or CLOBBER

[Richard Biener]

On Tue, Aug 8, 2023 at 5:10 AM Pan Li via Gcc-patches
<gcc-patches@gcc.gnu.org> 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. This patch would like to fix this ICE by ignoring the USE
> and CLOBBER insn before SET_SRC.

Note I don't think we're supposed to arrive here and it needs more explanation
as to why this is the correct fix to deal with the CLOBBER arriving
here.  Simply
making sure to not ICE can lead to wrong-code.

Earlier in the code we make sure to either have a single_set or a CLOBBER:

                    /* If the return register is not (in its entirety)
                       likely spilled, the return copy might be
                       partially or completely optimized away.  */
                    return_copy_pat = single_set (return_copy);
                    if (!return_copy_pat)
                      {
                        return_copy_pat = PATTERN (return_copy);
                        if (GET_CODE (return_copy_pat) != CLOBBER)
                          break;
                        else if (!optimize)
                          {
                            /* This might be (clobber (reg [<result>]))
                               when not optimizing.  Then check if
                               the previous insn is the clobber for
                               the return register.  */
                            copy_reg = SET_DEST (return_copy_pat);
...
                     }
                   copy_reg = SET_DEST (return_copy_pat);

so it looks "wrong" here already (try to enable RTL checking and extra
checking).  Well, we have

#define SET_DEST(RTX) XC2EXP (RTX, 0, SET, CLOBBER)
#define SET_SRC(RTX) XCEXP (RTX, 1, SET)

so SET_DEST is fine for CLOBBER, so I suppose the SH4 code is incorrect.
Please check when that was added and why and how that should be made
more correct.

Richard.



> Signed-off-by: Pan Li <pan2.li@intel.com>
>
> gcc/ChangeLog:
>
>         * mode-switching.cc (create_pre_exit): Add USE and CLOBBER check.
>         * rtl.h (CLOBBER_OR_USE_P): New macro.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/riscv/mode-switch-ice-1.c: New test.
> ---
>  gcc/mode-switching.cc                         |  1 +
>  gcc/rtl.h                                     |  4 ++++
>  .../gcc.target/riscv/mode-switch-ice-1.c      | 22 +++++++++++++++++++
>  3 files changed, 27 insertions(+)
>  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..8bb016a811d 100644
> --- a/gcc/mode-switching.cc
> +++ b/gcc/mode-switching.cc
> @@ -411,6 +411,7 @@ create_pre_exit (int n_entities, int *entity_map, const int *num_modes)
>                            conflict with address reloads.  */
>                         if (copy_start >= ret_start
>                             && copy_start + copy_num <= ret_end
> +                           && !CLOBBER_OR_USE_P (return_copy_pat)
>                             && OBJECT_P (SET_SRC (return_copy_pat)))
>                           forced_late_switch = true;
>                         break;
> diff --git a/gcc/rtl.h b/gcc/rtl.h
> index e1c51156f90..cdc2941c210 100644
> --- a/gcc/rtl.h
> +++ b/gcc/rtl.h
> @@ -1768,6 +1768,10 @@ extern const char * const reg_note_name[];
>  #define VAR_LOC_UNKNOWN_P(X) \
>    (GET_CODE (X) == CLOBBER && XEXP ((X), 0) == const0_rtx)
>
> +/* Determine whether RTX is USE or CLOBBER.  */
> +#define CLOBBER_OR_USE_P(RTX) \
> +  (GET_CODE (RTX) == USE || GET_CODE (RTX) == CLOBBER)
> +
>  /* 1 if RTX is emitted after a call, but it should take effect before
>     the call returns.  */
>  #define NOTE_DURING_CALL_P(RTX)                                \
> diff --git a/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
> new file mode 100644
> index 00000000000..1b34a471904
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
> @@ -0,0 +1,22 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +struct A { char e, f; };
> +
> +struct B
> +{
> +  int g;
> +  struct A h[4];
> +};
> +
> +extern void bar (int, int);
> +
> +struct B foo (void)
> +{
> +  bar (2, 1);
> +}
> +
> +void baz ()
> +{
> +  foo ();
> +}
> --
> 2.34.1
>

RE: [PATCH v2] Mode-Switching: Fix SET_SRC ICE when USE or CLOBBER

[Li, Pan2]

Hi Richard B,

Thanks for comments. Looks this SH4 parts existed in lcm.c from day 1 about 19 years ago, see 
this commit https://github.com/gcc-mirror/gcc/commit/cf99f196e2e18b62becbb660761fc2a586b85d78.

As you said, the problem exists from the original commit. Aks the return_copy_pat can be either single_set or CLOBBER
but it may still SET_SRC anyway. Thus, I think below parts should be only valid when single_set as the comments keep 
taking load/reloads. Then we can filter out the CLOBBER similar like "if (j >= 0 && GET_CODE (return_copy_pat) != CLOBBER)".
Not quit sure if my understanding is correct as not familiar with SH4.

                    if (j >= 0)
                      {
                        /* __builtin_return emits a sequence of loads to all
                           return registers.  One of them might require
                           another mode than MODE_EXIT, even if it is
                           unrelated to the return value, so we want to put
                           the final mode switch after it.  */
                        if (multi_reg_return
                            && targetm.calls.function_value_regno_p
                                (copy_start))
                          forced_late_switch = true;

                        /* 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;
                      }

> so it looks "wrong" here already (try to enable RTL checking and extra
> checking).  

Could you please help to share how to enable checks here?

Pan

-----Original Message-----
From: Richard Biener <richard.guenther@gmail.com> 
Sent: Tuesday, August 8, 2023 3:54 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>; jeffreyalaw@gmail.com
Subject: Re: [PATCH v2] Mode-Switching: Fix SET_SRC ICE when USE or CLOBBER

On Tue, Aug 8, 2023 at 5:10 AM Pan Li via Gcc-patches
<gcc-patches@gcc.gnu.org> 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. This patch would like to fix this ICE by ignoring the USE
> and CLOBBER insn before SET_SRC.

Note I don't think we're supposed to arrive here and it needs more explanation
as to why this is the correct fix to deal with the CLOBBER arriving
here.  Simply
making sure to not ICE can lead to wrong-code.

Earlier in the code we make sure to either have a single_set or a CLOBBER:

                    /* If the return register is not (in its entirety)
                       likely spilled, the return copy might be
                       partially or completely optimized away.  */
                    return_copy_pat = single_set (return_copy);
                    if (!return_copy_pat)
                      {
                        return_copy_pat = PATTERN (return_copy);
                        if (GET_CODE (return_copy_pat) != CLOBBER)
                          break;
                        else if (!optimize)
                          {
                            /* This might be (clobber (reg [<result>]))
                               when not optimizing.  Then check if
                               the previous insn is the clobber for
                               the return register.  */
                            copy_reg = SET_DEST (return_copy_pat);
...
                     }
                   copy_reg = SET_DEST (return_copy_pat);

so it looks "wrong" here already (try to enable RTL checking and extra
checking).  Well, we have

#define SET_DEST(RTX) XC2EXP (RTX, 0, SET, CLOBBER)
#define SET_SRC(RTX) XCEXP (RTX, 1, SET)

so SET_DEST is fine for CLOBBER, so I suppose the SH4 code is incorrect.
Please check when that was added and why and how that should be made
more correct.

Richard.



> Signed-off-by: Pan Li <pan2.li@intel.com>
>
> gcc/ChangeLog:
>
>         * mode-switching.cc (create_pre_exit): Add USE and CLOBBER check.
>         * rtl.h (CLOBBER_OR_USE_P): New macro.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/riscv/mode-switch-ice-1.c: New test.
> ---
>  gcc/mode-switching.cc                         |  1 +
>  gcc/rtl.h                                     |  4 ++++
>  .../gcc.target/riscv/mode-switch-ice-1.c      | 22 +++++++++++++++++++
>  3 files changed, 27 insertions(+)
>  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..8bb016a811d 100644
> --- a/gcc/mode-switching.cc
> +++ b/gcc/mode-switching.cc
> @@ -411,6 +411,7 @@ create_pre_exit (int n_entities, int *entity_map, const int *num_modes)
>                            conflict with address reloads.  */
>                         if (copy_start >= ret_start
>                             && copy_start + copy_num <= ret_end
> +                           && !CLOBBER_OR_USE_P (return_copy_pat)
>                             && OBJECT_P (SET_SRC (return_copy_pat)))
>                           forced_late_switch = true;
>                         break;
> diff --git a/gcc/rtl.h b/gcc/rtl.h
> index e1c51156f90..cdc2941c210 100644
> --- a/gcc/rtl.h
> +++ b/gcc/rtl.h
> @@ -1768,6 +1768,10 @@ extern const char * const reg_note_name[];
>  #define VAR_LOC_UNKNOWN_P(X) \
>    (GET_CODE (X) == CLOBBER && XEXP ((X), 0) == const0_rtx)
>
> +/* Determine whether RTX is USE or CLOBBER.  */
> +#define CLOBBER_OR_USE_P(RTX) \
> +  (GET_CODE (RTX) == USE || GET_CODE (RTX) == CLOBBER)
> +
>  /* 1 if RTX is emitted after a call, but it should take effect before
>     the call returns.  */
>  #define NOTE_DURING_CALL_P(RTX)                                \
> diff --git a/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
> new file mode 100644
> index 00000000000..1b34a471904
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
> @@ -0,0 +1,22 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +struct A { char e, f; };
> +
> +struct B
> +{
> +  int g;
> +  struct A h[4];
> +};
> +
> +extern void bar (int, int);
> +
> +struct B foo (void)
> +{
> +  bar (2, 1);
> +}
> +
> +void baz ()
> +{
> +  foo ();
> +}
> --
> 2.34.1
>

Re: [PATCH v2] Mode-Switching: Fix SET_SRC ICE when USE or CLOBBER

[Robin Dapp]

> Could you please help to share how to enable checks here?
Build with --enable-checking or rather --enable-checking=extra.

Regards
 Robin

RE: [PATCH v2] Mode-Switching: Fix SET_SRC ICE when USE or CLOBBER

[Li, Pan2]

Thanks Robin, will have a try without this PATCH.

Pan

-----Original Message-----
From: Robin Dapp <rdapp.gcc@gmail.com> 
Sent: Tuesday, August 8, 2023 5:43 PM
To: Li, Pan2 <pan2.li@intel.com>; Richard Biener <richard.guenther@gmail.com>
Cc: rdapp.gcc@gmail.com; gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>; jeffreyalaw@gmail.com
Subject: Re: [PATCH v2] Mode-Switching: Fix SET_SRC ICE when USE or CLOBBER

> Could you please help to share how to enable checks here?
Build with --enable-checking or rather --enable-checking=extra.

Regards
 Robin

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

[pan2.li]

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
diff --git a/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
new file mode 100644
index 00000000000..1b34a471904
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+struct A { char e, f; };
+
+struct B
+{
+  int g;
+  struct A h[4];
+};
+
+extern void bar (int, int);
+
+struct B foo (void)
+{
+  bar (2, 1);
+}
+
+void baz ()
+{
+  foo ();
+}
-- 
2.34.1

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

[Jeff Law]

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

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

[Li, Pan2]

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

[PATCH v4] Mode-Switching: Fix SET_SRC ICE for create_pre_exit

[pan2.li]

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 restrict the SET insn for SET_SRC.

Signed-off-by: Pan Li <pan2.li@intel.com>

gcc/ChangeLog:

	* mode-switching.cc (create_pre_exit): Add SET insn check.

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/mode-switch-ice-1.c: New test.
---
 gcc/mode-switching.cc                         |  1 +
 .../gcc.target/riscv/mode-switch-ice-1.c      | 22 +++++++++++++++++++
 2 files changed, 23 insertions(+)
 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..f483c831c35 100644
--- a/gcc/mode-switching.cc
+++ b/gcc/mode-switching.cc
@@ -411,6 +411,7 @@ create_pre_exit (int n_entities, int *entity_map, const int *num_modes)
 			   conflict with address reloads.  */
 			if (copy_start >= ret_start
 			    && copy_start + copy_num <= ret_end
+			    && GET_CODE (return_copy_pat) == SET
 			    && OBJECT_P (SET_SRC (return_copy_pat)))
 			  forced_late_switch = true;
 			break;
diff --git a/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
new file mode 100644
index 00000000000..1b34a471904
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/mode-switch-ice-1.c
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+struct A { char e, f; };
+
+struct B
+{
+  int g;
+  struct A h[4];
+};
+
+extern void bar (int, int);
+
+struct B foo (void)
+{
+  bar (2, 1);
+}
+
+void baz ()
+{
+  foo ();
+}
-- 
2.34.1

Re: [PATCH v4] Mode-Switching: Fix SET_SRC ICE for create_pre_exit

[Jeff Law]

On 8/12/23 18:56, 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 restrict the SET insn for SET_SRC.
> 
> Signed-off-by: Pan Li <pan2.li@intel.com>
> 
> gcc/ChangeLog:
> 
> 	* mode-switching.cc (create_pre_exit): Add SET insn check.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/riscv/mode-switch-ice-1.c: New test.
OK.  Thanks for the updated version.

jeff

RE: [PATCH v4] Mode-Switching: Fix SET_SRC ICE for create_pre_exit

[Li, Pan2]

Committed as passed both the bootstrap and regression test in x86, thanks Jeff.

Pan

-----Original Message-----
From: Jeff Law <jeffreyalaw@gmail.com> 
Sent: Tuesday, August 15, 2023 1:21 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 v4] Mode-Switching: Fix SET_SRC ICE for create_pre_exit



On 8/12/23 18:56, 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 restrict the SET insn for SET_SRC.
> 
> Signed-off-by: Pan Li <pan2.li@intel.com>
> 
> gcc/ChangeLog:
> 
> 	* mode-switching.cc (create_pre_exit): Add SET insn check.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/riscv/mode-switch-ice-1.c: New test.
OK.  Thanks for the updated version.

jeff