diff mbox series

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

Message ID 834db636-da81-ded9-3385-ae65a4cb7c91@linux.ibm.com
State Accepted
Headers
Series sel-sched: Verify change before replacing dest in EXPR_INSN_RTX [PR112995] |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Kewen.Lin Dec. 15, 2023, 8:52 a.m. UTC 
  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

--
2.39.3

Comments

Jeff Law Dec. 20, 2023, 8:30 p.m. UTC | #1 
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
Kewen.Lin Dec. 21, 2023, 5:45 a.m. UTC | #2 
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
diff mbox series

Patch

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;
+}