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

[Jeff Law <jeffreyalaw@gmail.com>]

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