[PATCH] sel-sched: Verify change before replacing dest in EXPR_INSN_RTX [PR112995]

[PATCH] sel-sched: Verify change before replacing dest in EXPR_INSN_RTX [PR112995]

[Kewen.Lin]

Hi,

PR112995 exposed one issue in current try_replace_dest_reg
that the result rtx insn after replace_dest_with_reg_in_expr
is probably unable to match any constraints.  Although there
are some checks on the changes onto dest or src of orig_insn,
none is performed on the EXPR_INSN_RTX.

This patch is to add the check before actually replacing dest
in expr with reg.

Bootstrapped and regtested on x86_64-redhat-linux and
powerpc64{,le}-linux-gnu.

Is it ok for trunk?

BR,
Kewen
-----
	PR rtl-optimization/112995

gcc/ChangeLog:

	* sel-sched.cc (try_replace_dest_reg): Check the validity of the
	replaced insn before actually replacing dest in expr.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/pr112995.c: New test.
---
 gcc/sel-sched.cc                            | 10 +++++++++-
 gcc/testsuite/gcc.target/powerpc/pr112995.c | 14 ++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr112995.c

diff --git a/gcc/sel-sched.cc b/gcc/sel-sched.cc
index 1925f4a9461..a35b5e16c91 100644
--- a/gcc/sel-sched.cc
+++ b/gcc/sel-sched.cc
@@ -1614,7 +1614,15 @@ try_replace_dest_reg (ilist_t orig_insns, rtx best_reg, expr_t expr)
   /* Make sure that EXPR has the right destination
      register.  */
   if (expr_dest_regno (expr) != REGNO (best_reg))
-    replace_dest_with_reg_in_expr (expr, best_reg);
+    {
+      rtx_insn *vinsn = EXPR_INSN_RTX (expr);
+      validate_change (vinsn, &SET_DEST (PATTERN (vinsn)), best_reg, 1);
+      bool res = verify_changes (0);
+      cancel_changes (0);
+      if (!res)
+	return false;
+      replace_dest_with_reg_in_expr (expr, best_reg);
+    }
   else
     EXPR_TARGET_AVAILABLE (expr) = 1;

diff --git a/gcc/testsuite/gcc.target/powerpc/pr112995.c b/gcc/testsuite/gcc.target/powerpc/pr112995.c
new file mode 100644
index 00000000000..4adcb5f3851
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/pr112995.c
@@ -0,0 +1,14 @@
+/* { dg-require-effective-target float128 } */
+/* { dg-options "-O2 -mdejagnu-cpu=power9 -fselective-scheduling2" } */
+
+/* Verify there is no ICE.  */
+
+int a[10];
+int b(_Float128 e) {
+  int c;
+  _Float128 d;
+  c = e;
+  d = c;
+  d = a[c] + d;
+  return d;
+}
--
2.39.3

Re: [PATCH] sel-sched: Verify change before replacing dest in EXPR_INSN_RTX [PR112995]

[Jeff Law]

On 12/15/23 01:52, Kewen.Lin wrote:
> Hi,
> 
> PR112995 exposed one issue in current try_replace_dest_reg
> that the result rtx insn after replace_dest_with_reg_in_expr
> is probably unable to match any constraints.  Although there
> are some checks on the changes onto dest or src of orig_insn,
> none is performed on the EXPR_INSN_RTX.
> 
> This patch is to add the check before actually replacing dest
> in expr with reg.
> 
> Bootstrapped and regtested on x86_64-redhat-linux and
> powerpc64{,le}-linux-gnu.
> 
> Is it ok for trunk?
> 
> BR,
> Kewen
> -----
> 	PR rtl-optimization/112995
> 
> gcc/ChangeLog:
> 
> 	* sel-sched.cc (try_replace_dest_reg): Check the validity of the
> 	replaced insn before actually replacing dest in expr.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/powerpc/pr112995.c: New test.
Setting aside whether or not we should just deprecate/remove sel-sched 
for now....



 From the PR:
> with moving up, we have:
> 
> (insn 46 0 0 (set (reg:DI 64 0 [135])
>         (sign_extend:DI (reg/v:SI 64 0 [orig:119 c ] [119]))) 31 {extendsidi2}
>      (expr_list:REG_DEAD (reg/v:SI 9 9 [orig:119 c ] [119])
>         (nil)))
> 
> in try_replace_dest_reg, we updated the above EXPR_INSN_RTX to:
> 
> (insn 48 0 0 (set (reg:DI 32 0)
>         (sign_extend:DI (reg/v:SI 64 0 [orig:119 c ] [119]))) 31 {extendsidi2}
>      (nil))
> 
> This doesn't match any constraint and it's an unexpected modification.


It would have been helpful to include that in the patch, along with the 
fact that (reg 32) and (reg 64) are FP and VREGs respectively.  That 
makes it clearer why the constraints might not match after the change.

OK for the trunk.
jeff

Re: [PATCH] sel-sched: Verify change before replacing dest in EXPR_INSN_RTX [PR112995]

[Kewen.Lin]

Hi Jeff,

on 2023/12/21 04:30, Jeff Law wrote:
> 
> 
> On 12/15/23 01:52, Kewen.Lin wrote:
>> Hi,
>>
>> PR112995 exposed one issue in current try_replace_dest_reg
>> that the result rtx insn after replace_dest_with_reg_in_expr
>> is probably unable to match any constraints.  Although there
>> are some checks on the changes onto dest or src of orig_insn,
>> none is performed on the EXPR_INSN_RTX.
>>
>> This patch is to add the check before actually replacing dest
>> in expr with reg.
>>
>> Bootstrapped and regtested on x86_64-redhat-linux and
>> powerpc64{,le}-linux-gnu.
>>
>> Is it ok for trunk?
>>
>> BR,
>> Kewen
>> -----
>>     PR rtl-optimization/112995
>>
>> gcc/ChangeLog:
>>
>>     * sel-sched.cc (try_replace_dest_reg): Check the validity of the
>>     replaced insn before actually replacing dest in expr.
>>
>> gcc/testsuite/ChangeLog:
>>
>>     * gcc.target/powerpc/pr112995.c: New test.
> Setting aside whether or not we should just deprecate/remove sel-sched for now....
> 
> 
> 
> From the PR:
>> with moving up, we have:
>>
>> (insn 46 0 0 (set (reg:DI 64 0 [135])
>>         (sign_extend:DI (reg/v:SI 64 0 [orig:119 c ] [119]))) 31 {extendsidi2}
>>      (expr_list:REG_DEAD (reg/v:SI 9 9 [orig:119 c ] [119])
>>         (nil)))
>>
>> in try_replace_dest_reg, we updated the above EXPR_INSN_RTX to:
>>
>> (insn 48 0 0 (set (reg:DI 32 0)
>>         (sign_extend:DI (reg/v:SI 64 0 [orig:119 c ] [119]))) 31 {extendsidi2}
>>      (nil))
>>
>> This doesn't match any constraint and it's an unexpected modification.
> 
> 
> It would have been helpful to include that in the patch, along with the fact that (reg 32) and (reg 64) are FP and VREGs respectively.  That makes it clearer why the constraints might not match after the change.
> 

Good idea!

> OK for the trunk.

Thanks for the review, as suggested I updated the commit log
as below and committed it as r14-6768-g5fbc77726f68a3.

-----
PR112995 exposed one issue in current try_replace_dest_reg
that the result rtx insn after replace_dest_with_reg_in_expr
is probably unable to match any constraints.  Although there
are some checks on the changes onto dest or src of orig_insn,
none is performed on the EXPR_INSN_RTX.

Initially we have:

(insn 31 6 10 2 (set (reg/v:SI 9 9 [orig:119 c ] [119])
                     (reg/v:SI 64 0 [orig:119 c ] [119]))
                "test.i":5:5 555 {*movsi_internal1} ... )
...
(insn 25 10 27 2 (set (reg:DI 64 0 [135])
                      (sign_extend:DI
                         (reg/v:SI 9 9 [orig:119 c ] [119])))
                 "test.i":6:5 31 {extendsidi2} ...)

with moving up, we have:

(insn 46 0 0 (set (reg:DI 64 0 [135])
                  (sign_extend:DI
                      (reg/v:SI 64 0 [orig:119 c ] [119])))
                   31 {extendsidi2} ...)

in try_replace_dest_reg, we updated the above EXPR_INSN_RTX to:

(insn 48 0 0 (set (reg:DI 32 0)
                  (sign_extend:DI
                      (reg/v:SI 64 0 [orig:119 c ] [119])))
                   31 {extendsidi2} ...)

The dest (reg 64) is a VR (also VSX REG), the updating makes
it become to (reg 32) which is a FPR (also VSX REG), we have
an alternative to match "VR,VR" but no one to match "FPR/VSX,
VR/VSX", so it fails with ICE.

This patch is to add the check before actually replacing dest
in expr with reg.
-----

BR,
Kewen