Re: LRA for avr: Clobber with match_dup

2023-07-20 Thread Vladimir Makarov via Gcc



On 7/20/23 07:17, senthilkumar.selva...@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)

(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.





Re: LRA for avr: Clobber with match_dup

2023-07-20 Thread Joern Rennecke
> 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.


LRA for avr: Clobber with match_dup

2023-07-20 Thread SenthilKumar.Selvaraj--- via Gcc
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)

(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