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
