LRA for avr: Clobber with match_dup

[Vladimir Makarov]

On 7/20/23 07:17, SenthilKumar.Selvaraj@microchip.com wrote:
> Hi,
>
>    The avr backend has this define_insn_and_split
>
> (define_insn_and_split "*tablejump_split"
>    [(set (pc)
>          (unspec:HI [(match_operand:HI 0 "register_operand" "!z,*r,z")]
>                     UNSPEC_INDEX_JMP))
>     (use (label_ref (match_operand 1 "" "")))
>     (clobber (match_dup 0))
>     (clobber (const_int 0))]
>    "!AVR_HAVE_EIJMP_EICALL"
>    "#"
>    "&& reload_completed"
>    [(parallel [(set (pc)
>                     (unspec:HI [(match_dup 0)]
>                                UNSPEC_INDEX_JMP))
>                (use (label_ref (match_dup 1)))
>                (clobber (match_dup 0))
>                (clobber (const_int 0))
>                (clobber (reg:CC REG_CC))])]
>    ""
>    [(set_attr "isa" "rjmp,rjmp,jmp")])
>
> Note the (clobber (match_dup 0)). When building
>
> $ avr-gcc -mmcu=avr51 gcc/gcc/testsuite/gcc.c-torture/compile/930120-1.c -O3 -funroll-loops -fdump-rtl-all
>
> The web pass transforms this insn from
>
> (jump_insn 120 538 124 25 (parallel [
>              (set (pc)
>                  (unspec:HI [
>                          (reg:HI 138)
>                      ] UNSPEC_INDEX_JMP))
>              (use (label_ref 121))
>              (clobber (reg:HI 138))
>              (clobber (const_int 0 [0]))
>          ]) "gcc/gcc/testsuite/gcc.c-torture/compile/930120-1.c":55:5 779 {*tablejump_split}
>       (expr_list:REG_DEAD (reg:HI 138)
>          (expr_list:REG_UNUSED (reg:HI 138)
>              (nil)))
>   -> 121)
>
> to
>
>   Web oldreg=138 newreg=279
> Updating insn 120 (138->279)
> <snip>
> (jump_insn 120 538 124 25 (parallel [
>              (set (pc)
>                  (unspec:HI [
>                          (reg:HI 138)
>                      ] UNSPEC_INDEX_JMP))
>              (use (label_ref 121))
>              (clobber (reg:HI 279))
>              (clobber (const_int 0 [0]))
>          ]) "gcc/gcc/testsuite/gcc.c-torture/compile/930120-1.c":55:5 779 {*tablejump_split}
>       (expr_list:REG_DEAD (reg:HI 138)
>          (expr_list:REG_UNUSED (reg:HI 138)
>              (nil)))
>   -> 121)
>
> Note the reg in the clobber is now 279, and not 138.
>
> With classic reload, however, this gets set back to whatever hardreg was assigned to r138.
It is just a fortunate side effect of algorithm how the reload pass 
changes pseudos to their hard registers.
> (jump_insn 120 538 121 26 (parallel [
>              (set (pc)
>                  (unspec:HI [
>                          (reg/f:HI 30 r30 [138])
>                      ] UNSPEC_INDEX_JMP))
>              (use (label_ref 121))
>              (clobber (reg/f:HI 30 r30 [138]))
>              (clobber (const_int 0 [0]))
>          ]) "gcc/gcc/testsuite/gcc.c-torture/compile/930120-1.c":55:5 779 {*tablejump_split}
>       (nil)
>   -> 121)
>
> With LRA, however, the pseudo reg remains unassigned, eventually causing an ICE in cselib_invalidate_regno.
>
> (jump_insn 120 538 121 26 (parallel [
>              (set (pc)
>                  (unspec:HI [
>                          (reg/f:HI 30 r30 [138])
>                      ] UNSPEC_INDEX_JMP))
>              (use (label_ref 121))
>              (clobber (reg:HI 279))
>              (clobber (const_int 0 [0]))
>          ]) "gcc/gcc/testsuite/gcc.c-torture/compile/930120-1.c":55:5 779 {*tablejump_split}
>       (nil)
>   -> 121)
>
> Is this something that LRA should be able to fix?

No, LRA can not do this but it keeps match_dup correct after any reloads 
and insn transformations.

I think it is a web optimization bug.  RA assumes the insn recognition 
should give the same insn code as it present in the insn.

In my opinion any optimization pass can assume this at the pass start 
and should keep this condition after its work finish.