* Re: LRA for avr: Clobber with match_dup
@ 2023-07-20 17:39 Joern Rennecke
0 siblings, 0 replies; 3+ messages in thread
From: Joern Rennecke @ 2023-07-20 17:39 UTC (permalink / raw)
To: SenthilKumar.Selvaraj, GCC
> Making operand 0 read/write in define_insn_and_split prevents the web pass from creating a new pseudo
> for the clobber, avoiding the whole problem.
That is the right solution. If you clobber a match_dup of an input
operand, that makes it and input-and-output operand, so you should
mark it as such.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: LRA for avr: Clobber with match_dup
2023-07-20 11:17 SenthilKumar.Selvaraj
@ 2023-07-20 19:38 ` Vladimir Makarov
0 siblings, 0 replies; 3+ messages in thread
From: Vladimir Makarov @ 2023-07-20 19:38 UTC (permalink / raw)
To: SenthilKumar.Selvaraj, gcc; +Cc: hubicka
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* LRA for avr: Clobber with match_dup
@ 2023-07-20 11:17 SenthilKumar.Selvaraj
2023-07-20 19:38 ` Vladimir Makarov
0 siblings, 1 reply; 3+ messages in thread
From: SenthilKumar.Selvaraj @ 2023-07-20 11:17 UTC (permalink / raw)
To: gcc; +Cc: hubicka, vmakarov
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.
(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?
Making operand 0 read/write in define_insn_and_split prevents the web pass from creating a new pseudo
for the clobber, avoiding the whole problem.
(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")])
However, https://gcc.gnu.org/onlinedocs/gccint/Side-Effects.html has this to say for clobber
"There is one other known use for clobbering a pseudo register in a parallel: when one of the input operands of the insn is also
clobbered by the insn. In this case, using the same pseudo register in the clobber and elsewhere in the insn produces the expected
results."
so I'm not sure if that's the right way to fix it. I could add a matching constraint (i.e "=0,0,0"), but until reload runs,
the clobber reg would be different from input reg, so that seems wrong to me too.
Any ideas?
Regards
Senthil
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-07-20 19:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-07-20 17:39 LRA for avr: Clobber with match_dup Joern Rennecke
-- strict thread matches above, loose matches on Subject: below --
2023-07-20 11:17 SenthilKumar.Selvaraj
2023-07-20 19:38 ` Vladimir Makarov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).