[v2] Mode-Switching: Fix SET_SRC ICE when USE or CLOBBER
Checks
Commit Message
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
Comments
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
>
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
>
> Could you please help to share how to enable checks here?
Build with --enable-checking or rather --enable-checking=extra.
Regards
Robin
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
@@ -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;
@@ -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) \
new file mode 100644
@@ -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 ();
+}