LRA for avr: Handling hard regs set directly at expand

LRA for avr: Handling hard regs set directly at expand

[SenthilKumar.Selvaraj]

Hi,

  The avr target has a bunch of patterns that directly set hard regs at expand time, like so

(define_expand "cpymemhi"
  [(parallel [(set (match_operand:BLK 0 "memory_operand" "")
                   (match_operand:BLK 1 "memory_operand" ""))
              (use (match_operand:HI 2 "const_int_operand" ""))
              (use (match_operand:HI 3 "const_int_operand" ""))])]
  ""
  {
    if (avr_emit_cpymemhi (operands))
      DONE;

    FAIL;
  })

where avr_emit_cpymemhi generates

(insn 14 13 15 4 (set (reg:HI 30 r30)
        (reg:HI 48 [ ivtmp.10 ])) "pr53505.c":21:22 -1
     (nil))
(insn 15 14 16 4 (set (reg:HI 26 r26)
        (reg/f:HI 38 virtual-stack-vars)) "pr53505.c":21:22 -1
     (nil))
(insn 16 15 17 4 (parallel [
            (set (mem:BLK (reg:HI 26 r26) [0  A8])
                (mem:BLK (reg:HI 30 r30) [0  A8]))
            (unspec [
                    (const_int 0 [0])
                ] UNSPEC_CPYMEM)
            (use (reg:QI 52))
            (clobber (reg:HI 26 r26))
            (clobber (reg:HI 30 r30))
            (clobber (reg:QI 0 r0))
            (clobber (reg:QI 52))
        ]) "pr53505.c":21:22 -1
     (nil))

Classic reload knows about these - find_reg masks out bad_spill_regs, and bad_spill_regs
when ORed with chain->live_throughout in order_regs_for_reload picks up r30.

LRA, however, appears to not consider that, and proceeds to use such regs as reload regs.
For the same source, it generates
<snip>
 Choosing alt 0 in insn 15:  (0) =r  (1) r {*movhi_split}
      Creating newreg=70, assigning class GENERAL_REGS to r70
   15: r26:HI=r70:HI
      REG_EQUAL r28:HI+0x1
    Inserting insn reload before:
   58: r70:HI=r28:HI+0x1

Choosing alt 3 in insn 58:  (0) d  (1) 0  (2) nYnn {*addhi3_split}
      Creating newreg=71 from oldreg=70, assigning class LD_REGS to r71
   58: r71:HI=r71:HI+0x1
    Inserting insn reload before:
   59: r71:HI=r28:HI
    Inserting insn reload after:
   60: r70:HI=r71:HI

********** Assignment #1: **********

	 Assigning to 71 (cl=LD_REGS, orig=70, freq=3000, tfirst=71, tfreq=3000)...
	   Assign 30 to reload r71 (freq=3000)
	Hard reg 26 is preferable by r70 with profit 1000
	Hard reg 30 is preferable by r70 with profit 1000
	 Assigning to 70 (cl=GENERAL_REGS, orig=70, freq=2000, tfirst=70, tfreq=2000)...
	   Assign 30 to reload r70 (freq=2000)


(insn 14 13 59 3 (set (reg:HI 30 r30)
        (reg:HI 18 r18 [orig:48 ivtmp.10 ] [48])) "pr53505.c":21:22 101 {*movhi_split}
     (nil))
(insn 59 14 58 3 (set (reg:HI 30 r30 [70])
        (reg/f:HI 28 r28)) "pr53505.c":21:22 101 {*movhi_split}
     (nil))
(insn 58 59 15 3 (set (reg:HI 30 r30 [70])
        (plus:HI (reg:HI 30 r30 [70])
            (const_int 1 [0x1]))) "pr53505.c":21:22 165 {*addhi3_split}
     (nil))
(insn 15 58 16 3 (set (reg:HI 26 r26)
        (reg:HI 30 r30 [70])) "pr53505.c":21:22 101 {*movhi_split}
     (expr_list:REG_EQUAL (plus:HI (reg/f:HI 28 r28)
            (const_int 1 [0x1]))
        (nil)))
(insn 16 15 17 3 (parallel [
            (set (mem:BLK (reg:HI 26 r26) [0  A8])
                (mem:BLK (reg:HI 30 r30) [0  A8]))
            (unspec [
                    (const_int 0 [0])
                ] UNSPEC_CPYMEM)
            (use (reg:QI 22 r22 [52]))
            (clobber (reg:HI 26 r26))
            (clobber (reg:HI 30 r30))
            (clobber (reg:QI 0 r0))
            (clobber (reg:QI 22 r22 [52]))
        ]) "pr53505.c":21:22 132 {cpymem_qi}
     (nil))

LRA generates insn 59 that clobbers r30 set in insn 14, causing an execution
failure down the line.

How should the avr backend deal with this?

Regards
Senthil

Re: LRA for avr: Handling hard regs set directly at expand

[Georg-Johann Lay]

Am 17.07.23 um 13:33 schrieb SenthilKumar.Selvaraj--- via Gcc:
> Hi,
> 
>    The avr target has a bunch of patterns that directly set hard regs at expand time, like so

The correct approach would be to use usual predicates together with 
constraints that describe the register instead of hard regs, e.g. 
(match_operand:HI n "register_operand" "R18_2") for a 2-byte register 
that starts at R18 instead of (reg:HI 18).  I deprecated and removed 
constraints starting with "R" long ago in order to get "R" free for that 
purpose.

Some years ago I tried such constraints (and hence also zoo of new 
register classes that are required to accommodate them).  The resulting 
code quality was so bad that I quickly abandoned that approach, and IIRC 
there were also spill fails.  Appears that reload / ira was overwhelmed 
by the multitude of new reg classes and took sub-optimal decisions.

The way out was more of explicit hard regs in expand, together with 
awkward functionalities like avr_fix_operands (PR63633) and the 
functions that use it.  That way we get correct code without performance 
penalties in unrelated places.

Most of such insns are explicitly modelling hand-written asm functions 
in libgcc, because most of these functions have a footprint smaller than 
the default ABI.  And some functions have an interface not complying to 
default ABI.

For the case of cpymem etc from below, explicit hard registers were used 
because register allocator did a bad job when using constraints like "e" 
(X, Y, or Z).

Johann


> (define_expand "cpymemhi"
>    [(parallel [(set (match_operand:BLK 0 "memory_operand" "")
>                     (match_operand:BLK 1 "memory_operand" ""))
>                (use (match_operand:HI 2 "const_int_operand" ""))
>                (use (match_operand:HI 3 "const_int_operand" ""))])]
>    ""
>    {
>      if (avr_emit_cpymemhi (operands))
>        DONE;
> 
>      FAIL;
>    })
> 
> where avr_emit_cpymemhi generates
> 
> (insn 14 13 15 4 (set (reg:HI 30 r30)
>          (reg:HI 48 [ ivtmp.10 ])) "pr53505.c":21:22 -1
>       (nil))
> (insn 15 14 16 4 (set (reg:HI 26 r26)
>          (reg/f:HI 38 virtual-stack-vars)) "pr53505.c":21:22 -1
>       (nil))
> (insn 16 15 17 4 (parallel [
>              (set (mem:BLK (reg:HI 26 r26) [0  A8])
>                  (mem:BLK (reg:HI 30 r30) [0  A8]))
>              (unspec [
>                      (const_int 0 [0])
>                  ] UNSPEC_CPYMEM)
>              (use (reg:QI 52))
>              (clobber (reg:HI 26 r26))
>              (clobber (reg:HI 30 r30))
>              (clobber (reg:QI 0 r0))
>              (clobber (reg:QI 52))
>          ]) "pr53505.c":21:22 -1
>       (nil))
> 
> Classic reload knows about these - find_reg masks out bad_spill_regs, and bad_spill_regs
> when ORed with chain->live_throughout in order_regs_for_reload picks up r30.
> 
> LRA, however, appears to not consider that, and proceeds to use such regs as reload regs.
> For the same source, it generates
> <snip>
>   Choosing alt 0 in insn 15:  (0) =r  (1) r {*movhi_split}
>        Creating newreg=70, assigning class GENERAL_REGS to r70
>     15: r26:HI=r70:HI
>        REG_EQUAL r28:HI+0x1
>      Inserting insn reload before:
>     58: r70:HI=r28:HI+0x1
> 
> Choosing alt 3 in insn 58:  (0) d  (1) 0  (2) nYnn {*addhi3_split}
>        Creating newreg=71 from oldreg=70, assigning class LD_REGS to r71
>     58: r71:HI=r71:HI+0x1
>      Inserting insn reload before:
>     59: r71:HI=r28:HI
>      Inserting insn reload after:
>     60: r70:HI=r71:HI
> 
> ********** Assignment #1: **********
> 
> 	 Assigning to 71 (cl=LD_REGS, orig=70, freq=3000, tfirst=71, tfreq=3000)...
> 	   Assign 30 to reload r71 (freq=3000)
> 	Hard reg 26 is preferable by r70 with profit 1000
> 	Hard reg 30 is preferable by r70 with profit 1000
> 	 Assigning to 70 (cl=GENERAL_REGS, orig=70, freq=2000, tfirst=70, tfreq=2000)...
> 	   Assign 30 to reload r70 (freq=2000)
> 
> 
> (insn 14 13 59 3 (set (reg:HI 30 r30)
>          (reg:HI 18 r18 [orig:48 ivtmp.10 ] [48])) "pr53505.c":21:22 101 {*movhi_split}
>       (nil))
> (insn 59 14 58 3 (set (reg:HI 30 r30 [70])
>          (reg/f:HI 28 r28)) "pr53505.c":21:22 101 {*movhi_split}
>       (nil))
> (insn 58 59 15 3 (set (reg:HI 30 r30 [70])
>          (plus:HI (reg:HI 30 r30 [70])
>              (const_int 1 [0x1]))) "pr53505.c":21:22 165 {*addhi3_split}
>       (nil))
> (insn 15 58 16 3 (set (reg:HI 26 r26)
>          (reg:HI 30 r30 [70])) "pr53505.c":21:22 101 {*movhi_split}
>       (expr_list:REG_EQUAL (plus:HI (reg/f:HI 28 r28)
>              (const_int 1 [0x1]))
>          (nil)))
> (insn 16 15 17 3 (parallel [
>              (set (mem:BLK (reg:HI 26 r26) [0  A8])
>                  (mem:BLK (reg:HI 30 r30) [0  A8]))
>              (unspec [
>                      (const_int 0 [0])
>                  ] UNSPEC_CPYMEM)
>              (use (reg:QI 22 r22 [52]))
>              (clobber (reg:HI 26 r26))
>              (clobber (reg:HI 30 r30))
>              (clobber (reg:QI 0 r0))
>              (clobber (reg:QI 22 r22 [52]))
>          ]) "pr53505.c":21:22 132 {cpymem_qi}
>       (nil))
> 
> LRA generates insn 59 that clobbers r30 set in insn 14, causing an execution
> failure down the line.
> 
> How should the avr backend deal with this?
> 
> Regards
> Senthil
> 
> 

Re: LRA for avr: Handling hard regs set directly at expand

[SenthilKumar.Selvaraj]

On Thu, 2023-07-27 at 15:11 +0200, Georg-Johann Lay wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 17.07.23 um 13:33 schrieb SenthilKumar.Selvaraj--- via Gcc:
> > Hi,
> > 
> >    The avr target has a bunch of patterns that directly set hard regs at expand time, like so
> 
> The correct approach would be to use usual predicates together with
> constraints that describe the register instead of hard regs, e.g.
> (match_operand:HI n "register_operand" "R18_2") for a 2-byte register
> that starts at R18 instead of (reg:HI 18).  I deprecated and removed
> constraints starting with "R" long ago in order to get "R" free for that
> purpose.
> 
> Some years ago I tried such constraints (and hence also zoo of new
> register classes that are required to accommodate them).  The resulting
> code quality was so bad that I quickly abandoned that approach, and IIRC
> there were also spill fails.  Appears that reload / ira was overwhelmed
> by the multitude of new reg classes and took sub-optimal decisions.
> 
> The way out was more of explicit hard regs in expand, together with
> awkward functionalities like avr_fix_operands (PR63633) and the
> functions that use it.  That way we get correct code without performance
> penalties in unrelated places.
> 
> Most of such insns are explicitly modelling hand-written asm functions
> in libgcc, because most of these functions have a footprint smaller than
> the default ABI.  And some functions have an interface not complying to
> default ABI.
> 
> For the case of cpymem etc from below, explicit hard registers were used
> because register allocator did a bad job when using constraints like "e"
> (X, Y, or Z).

I guessed that much. Yes, using constraints works - I used "x" and "z" that
directly correspond to REG_X and REG_Z (ignore the weird operand numbering).

diff --git a/gcc/config/avr/avr.md b/gcc/config/avr/avr.md
index be0f8dcbe0e..6c6c4e4e212 100644
--- a/gcc/config/avr/avr.md
+++ b/gcc/config/avr/avr.md
@@ -1148,20 +1148,20 @@
 ;; "cpymem_qi"
 ;; "cpymem_hi"
 (define_insn_and_split "cpymem_<mode>"
-  [(set (mem:BLK (reg:HI REG_X))
-        (mem:BLK (reg:HI REG_Z)))
+  [(set (mem:BLK (match_operand:HI 3 "register_operand" "+x"))
+        (mem:BLK (match_operand:HI 4 "register_operand" "+z")))
    (unspec [(match_operand:QI 0 "const_int_operand" "n")]
            UNSPEC_CPYMEM)
    (use (match_operand:QIHI 1 "register_operand" "<CPYMEM_r_d>"))
-   (clobber (reg:HI REG_X))
-   (clobber (reg:HI REG_Z))
+   (clobber (match_dup 3))
+   (clobber (match_dup 4))
    (clobber (reg:QI LPM_REGNO))
    (clobber (match_operand:QIHI 2 "register_operand" "=1"))]
   ""
   "#"
   "&& reload_completed"
-  [(parallel [(set (mem:BLK (reg:HI REG_X))
-                   (mem:BLK (reg:HI REG_Z)))
+  [(parallel [(set (mem:BLK (match_dup 3))
+                   (mem:BLK (match_dup 4)))
               (unspec [(match_dup 0)]
                       UNSPEC_CPYMEM)
               (use (match_dup 1))

I know you did these changes a long time ago, but do you happen to have any
test cases lying around that I can use to see if LRA does a better job than
classic reload?

Vladimir, given that classic reload handled such hardcoded hard regs just 
fine, should LRA also be able to deal with them the same way? Or is this
something that LRA is not going to support?

Regards
Senthil

> 
> Johann
> 
> 
> > (define_expand "cpymemhi"
> >    [(parallel [(set (match_operand:BLK 0 "memory_operand" "")
> >                     (match_operand:BLK 1 "memory_operand" ""))
> >                (use (match_operand:HI 2 "const_int_operand" ""))
> >                (use (match_operand:HI 3 "const_int_operand" ""))])]
> >    ""
> >    {
> >      if (avr_emit_cpymemhi (operands))
> >        DONE;
> > 
> >      FAIL;
> >    })
> > 
> > where avr_emit_cpymemhi generates
> > 
> > (insn 14 13 15 4 (set (reg:HI 30 r30)
> >          (reg:HI 48 [ ivtmp.10 ])) "pr53505.c":21:22 -1
> >       (nil))
> > (insn 15 14 16 4 (set (reg:HI 26 r26)
> >          (reg/f:HI 38 virtual-stack-vars)) "pr53505.c":21:22 -1
> >       (nil))
> > (insn 16 15 17 4 (parallel [
> >              (set (mem:BLK (reg:HI 26 r26) [0  A8])
> >                  (mem:BLK (reg:HI 30 r30) [0  A8]))
> >              (unspec [
> >                      (const_int 0 [0])
> >                  ] UNSPEC_CPYMEM)
> >              (use (reg:QI 52))
> >              (clobber (reg:HI 26 r26))
> >              (clobber (reg:HI 30 r30))
> >              (clobber (reg:QI 0 r0))
> >              (clobber (reg:QI 52))
> >          ]) "pr53505.c":21:22 -1
> >       (nil))
> > 
> > Classic reload knows about these - find_reg masks out bad_spill_regs, and bad_spill_regs
> > when ORed with chain->live_throughout in order_regs_for_reload picks up r30.
> > 
> > LRA, however, appears to not consider that, and proceeds to use such regs as reload regs.
> > For the same source, it generates
> > <snip>
> >   Choosing alt 0 in insn 15:  (0) =r  (1) r {*movhi_split}
> >        Creating newreg=70, assigning class GENERAL_REGS to r70
> >     15: r26:HI=r70:HI
> >        REG_EQUAL r28:HI+0x1
> >      Inserting insn reload before:
> >     58: r70:HI=r28:HI+0x1
> > 
> > Choosing alt 3 in insn 58:  (0) d  (1) 0  (2) nYnn {*addhi3_split}
> >        Creating newreg=71 from oldreg=70, assigning class LD_REGS to r71
> >     58: r71:HI=r71:HI+0x1
> >      Inserting insn reload before:
> >     59: r71:HI=r28:HI
> >      Inserting insn reload after:
> >     60: r70:HI=r71:HI
> > 
> > ********** Assignment #1: **********
> > 
> >        Assigning to 71 (cl=LD_REGS, orig=70, freq=3000, tfirst=71, tfreq=3000)...
> >          Assign 30 to reload r71 (freq=3000)
> >       Hard reg 26 is preferable by r70 with profit 1000
> >       Hard reg 30 is preferable by r70 with profit 1000
> >        Assigning to 70 (cl=GENERAL_REGS, orig=70, freq=2000, tfirst=70, tfreq=2000)...
> >          Assign 30 to reload r70 (freq=2000)
> > 
> > 
> > (insn 14 13 59 3 (set (reg:HI 30 r30)
> >          (reg:HI 18 r18 [orig:48 ivtmp.10 ] [48])) "pr53505.c":21:22 101 {*movhi_split}
> >       (nil))
> > (insn 59 14 58 3 (set (reg:HI 30 r30 [70])
> >          (reg/f:HI 28 r28)) "pr53505.c":21:22 101 {*movhi_split}
> >       (nil))
> > (insn 58 59 15 3 (set (reg:HI 30 r30 [70])
> >          (plus:HI (reg:HI 30 r30 [70])
> >              (const_int 1 [0x1]))) "pr53505.c":21:22 165 {*addhi3_split}
> >       (nil))
> > (insn 15 58 16 3 (set (reg:HI 26 r26)
> >          (reg:HI 30 r30 [70])) "pr53505.c":21:22 101 {*movhi_split}
> >       (expr_list:REG_EQUAL (plus:HI (reg/f:HI 28 r28)
> >              (const_int 1 [0x1]))
> >          (nil)))
> > (insn 16 15 17 3 (parallel [
> >              (set (mem:BLK (reg:HI 26 r26) [0  A8])
> >                  (mem:BLK (reg:HI 30 r30) [0  A8]))
> >              (unspec [
> >                      (const_int 0 [0])
> >                  ] UNSPEC_CPYMEM)
> >              (use (reg:QI 22 r22 [52]))
> >              (clobber (reg:HI 26 r26))
> >              (clobber (reg:HI 30 r30))
> >              (clobber (reg:QI 0 r0))
> >              (clobber (reg:QI 22 r22 [52]))
> >          ]) "pr53505.c":21:22 132 {cpymem_qi}
> >       (nil))
> > 
> > LRA generates insn 59 that clobbers r30 set in insn 14, causing an execution
> > failure down the line.
> > 
> > How should the avr backend deal with this?
> > 
> > Regards
> > Senthil
> > 
> > 

Re: LRA for avr: Handling hard regs set directly at expand

[Georg-Johann Lay]

Am 28.07.23 um 07:04 schrieb SenthilKumar.Selvaraj@microchip.com:
> On Thu, 2023-07-27 at 15:11 +0200, Georg-Johann Lay wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> Am 17.07.23 um 13:33 schrieb SenthilKumar.Selvaraj--- via Gcc:
>>> Hi,
>>>
>>>     The avr target has a bunch of patterns that directly set hard regs at expand time, like so
>>
>> The correct approach would be to use usual predicates together with
>> constraints that describe the register instead of hard regs, e.g.
>> (match_operand:HI n "register_operand" "R18_2") for a 2-byte register
>> that starts at R18 instead of (reg:HI 18).  I deprecated and removed
>> constraints starting with "R" long ago in order to get "R" free for that
>> purpose.
>>
>> Some years ago I tried such constraints (and hence also zoo of new
>> register classes that are required to accommodate them).  The resulting
>> code quality was so bad that I quickly abandoned that approach, and IIRC
>> there were also spill fails.  Appears that reload / ira was overwhelmed
>> by the multitude of new reg classes and took sub-optimal decisions.
>>
>> The way out was more of explicit hard regs in expand, together with
>> awkward functionalities like avr_fix_operands (PR63633) and the
>> functions that use it.  That way we get correct code without performance
>> penalties in unrelated places.
>>
>> Most of such insns are explicitly modelling hand-written asm functions
>> in libgcc, because most of these functions have a footprint smaller than
>> the default ABI.  And some functions have an interface not complying to
>> default ABI.
>>
>> For the case of cpymem etc from below, explicit hard registers were used
>> because register allocator did a bad job when using constraints like "e"
>> (X, Y, or Z).
> 
> I guessed that much. Yes, using constraints works - I used "x" and "z" that
> directly correspond to REG_X and REG_Z (ignore the weird operand numbering).
> 
> diff --git a/gcc/config/avr/avr.md b/gcc/config/avr/avr.md
> index be0f8dcbe0e..6c6c4e4e212 100644
> --- a/gcc/config/avr/avr.md
> +++ b/gcc/config/avr/avr.md
> @@ -1148,20 +1148,20 @@
>   ;; "cpymem_qi"
>   ;; "cpymem_hi"
>   (define_insn_and_split "cpymem_<mode>"
> -  [(set (mem:BLK (reg:HI REG_X))
> -        (mem:BLK (reg:HI REG_Z)))
> +  [(set (mem:BLK (match_operand:HI 3 "register_operand" "+x"))
> +        (mem:BLK (match_operand:HI 4 "register_operand" "+z")))
>      (unspec [(match_operand:QI 0 "const_int_operand" "n")]
>              UNSPEC_CPYMEM)
>      (use (match_operand:QIHI 1 "register_operand" "<CPYMEM_r_d>"))
> -   (clobber (reg:HI REG_X))
> -   (clobber (reg:HI REG_Z))
> +   (clobber (match_dup 3))
> +   (clobber (match_dup 4))

With some other insns I encountered problem with clobber (match_dup), 
similar to PR50788, and thus used clobber of operand with constraint as 
proposed in that PR.  See also comments for insn "*add.for.eqne.<mode>".

>      (clobber (reg:QI LPM_REGNO))
>      (clobber (match_operand:QIHI 2 "register_operand" "=1"))]
>     ""
>     "#"
>     "&& reload_completed"
> -  [(parallel [(set (mem:BLK (reg:HI REG_X))
> -                   (mem:BLK (reg:HI REG_Z)))
> +  [(parallel [(set (mem:BLK (match_dup 3))
> +                   (mem:BLK (match_dup 4)))

This split happens when reload_completed, thus using hard regs should be 
okay (match_dup will be X or Z anyway).

>                 (unspec [(match_dup 0)]
>                         UNSPEC_CPYMEM)
>                 (use (match_dup 1))
> 
> I know you did these changes a long time ago, but do you happen to have any
> test cases lying around that I can use to see if LRA does a better job than
> classic reload?

No.  But the test cases I used were straight forward lines of code.

Johann

> Vladimir, given that classic reload handled such hardcoded hard regs just
> fine, should LRA also be able to deal with them the same way? Or is this
> something that LRA is not going to support?
> 
> Regards
> Senthil

Re: LRA for avr: Handling hard regs set directly at expand

[Vladimir Makarov]

On 7/17/23 07:33, SenthilKumar.Selvaraj@microchip.com wrote:
> Hi,
>
>    The avr target has a bunch of patterns that directly set hard regs at expand time, like so
>
> (define_expand "cpymemhi"
>    [(parallel [(set (match_operand:BLK 0 "memory_operand" "")
>                     (match_operand:BLK 1 "memory_operand" ""))
>                (use (match_operand:HI 2 "const_int_operand" ""))
>                (use (match_operand:HI 3 "const_int_operand" ""))])]
>    ""
>    {
>      if (avr_emit_cpymemhi (operands))
>        DONE;
>
>      FAIL;
>    })
>
> where avr_emit_cpymemhi generates
>
> (insn 14 13 15 4 (set (reg:HI 30 r30)
>          (reg:HI 48 [ ivtmp.10 ])) "pr53505.c":21:22 -1
>       (nil))
> (insn 15 14 16 4 (set (reg:HI 26 r26)
>          (reg/f:HI 38 virtual-stack-vars)) "pr53505.c":21:22 -1
>       (nil))
> (insn 16 15 17 4 (parallel [
>              (set (mem:BLK (reg:HI 26 r26) [0  A8])
>                  (mem:BLK (reg:HI 30 r30) [0  A8]))
>              (unspec [
>                      (const_int 0 [0])
>                  ] UNSPEC_CPYMEM)
>              (use (reg:QI 52))
>              (clobber (reg:HI 26 r26))
>              (clobber (reg:HI 30 r30))
>              (clobber (reg:QI 0 r0))
>              (clobber (reg:QI 52))
>          ]) "pr53505.c":21:22 -1
>       (nil))
>
> Classic reload knows about these - find_reg masks out bad_spill_regs, and bad_spill_regs
> when ORed with chain->live_throughout in order_regs_for_reload picks up r30.
>
> LRA, however, appears to not consider that, and proceeds to use such regs as reload regs.
> For the same source, it generates
> <snip>
>   Choosing alt 0 in insn 15:  (0) =r  (1) r {*movhi_split}
>        Creating newreg=70, assigning class GENERAL_REGS to r70
>     15: r26:HI=r70:HI
>        REG_EQUAL r28:HI+0x1
>      Inserting insn reload before:
>     58: r70:HI=r28:HI+0x1
>
> Choosing alt 3 in insn 58:  (0) d  (1) 0  (2) nYnn {*addhi3_split}
>        Creating newreg=71 from oldreg=70, assigning class LD_REGS to r71
>     58: r71:HI=r71:HI+0x1
>      Inserting insn reload before:
>     59: r71:HI=r28:HI
>      Inserting insn reload after:
>     60: r70:HI=r71:HI
>
> ********** Assignment #1: **********
>
> 	 Assigning to 71 (cl=LD_REGS, orig=70, freq=3000, tfirst=71, tfreq=3000)...
> 	   Assign 30 to reload r71 (freq=3000)
> 	Hard reg 26 is preferable by r70 with profit 1000
> 	Hard reg 30 is preferable by r70 with profit 1000
> 	 Assigning to 70 (cl=GENERAL_REGS, orig=70, freq=2000, tfirst=70, tfreq=2000)...
> 	   Assign 30 to reload r70 (freq=2000)
>
>
> (insn 14 13 59 3 (set (reg:HI 30 r30)
>          (reg:HI 18 r18 [orig:48 ivtmp.10 ] [48])) "pr53505.c":21:22 101 {*movhi_split}
>       (nil))
> (insn 59 14 58 3 (set (reg:HI 30 r30 [70])
>          (reg/f:HI 28 r28)) "pr53505.c":21:22 101 {*movhi_split}
>       (nil))
> (insn 58 59 15 3 (set (reg:HI 30 r30 [70])
>          (plus:HI (reg:HI 30 r30 [70])
>              (const_int 1 [0x1]))) "pr53505.c":21:22 165 {*addhi3_split}
>       (nil))
> (insn 15 58 16 3 (set (reg:HI 26 r26)
>          (reg:HI 30 r30 [70])) "pr53505.c":21:22 101 {*movhi_split}
>       (expr_list:REG_EQUAL (plus:HI (reg/f:HI 28 r28)
>              (const_int 1 [0x1]))
>          (nil)))
> (insn 16 15 17 3 (parallel [
>              (set (mem:BLK (reg:HI 26 r26) [0  A8])
>                  (mem:BLK (reg:HI 30 r30) [0  A8]))
>              (unspec [
>                      (const_int 0 [0])
>                  ] UNSPEC_CPYMEM)
>              (use (reg:QI 22 r22 [52]))
>              (clobber (reg:HI 26 r26))
>              (clobber (reg:HI 30 r30))
>              (clobber (reg:QI 0 r0))
>              (clobber (reg:QI 22 r22 [52]))
>          ]) "pr53505.c":21:22 132 {cpymem_qi}
>       (nil))
>
> LRA generates insn 59 that clobbers r30 set in insn 14, causing an execution
> failure down the line.
>
> How should the avr backend deal with this?
>
Sorry for the big delay with the answer.  I was on vacation.

There are probably some ways to fix it by changing patterns as other 
people suggested but I'd like to see the current patterns work for LRA 
as well.

Could you send me the test case on which I could reproduce the problem 
and work on implementing such functionality.

Re: LRA for avr: Handling hard regs set directly at expand

[SenthilKumar.Selvaraj]

On Wed, 2023-08-02 at 12:54 -0400, Vladimir Makarov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 7/17/23 07:33, SenthilKumar.Selvaraj@microchip.com wrote:
> > Hi,
> > 
> >    The avr target has a bunch of patterns that directly set hard regs at expand time, like so
> > 
> > (define_expand "cpymemhi"
> >    [(parallel [(set (match_operand:BLK 0 "memory_operand" "")
> >                     (match_operand:BLK 1 "memory_operand" ""))
> >                (use (match_operand:HI 2 "const_int_operand" ""))
> >                (use (match_operand:HI 3 "const_int_operand" ""))])]
> >    ""
> >    {
> >      if (avr_emit_cpymemhi (operands))
> >        DONE;
> > 
> >      FAIL;
> >    })
> > 
> > where avr_emit_cpymemhi generates
> > 
> > (insn 14 13 15 4 (set (reg:HI 30 r30)
> >          (reg:HI 48 [ ivtmp.10 ])) "pr53505.c":21:22 -1
> >       (nil))
> > (insn 15 14 16 4 (set (reg:HI 26 r26)
> >          (reg/f:HI 38 virtual-stack-vars)) "pr53505.c":21:22 -1
> >       (nil))
> > (insn 16 15 17 4 (parallel [
> >              (set (mem:BLK (reg:HI 26 r26) [0  A8])
> >                  (mem:BLK (reg:HI 30 r30) [0  A8]))
> >              (unspec [
> >                      (const_int 0 [0])
> >                  ] UNSPEC_CPYMEM)
> >              (use (reg:QI 52))
> >              (clobber (reg:HI 26 r26))
> >              (clobber (reg:HI 30 r30))
> >              (clobber (reg:QI 0 r0))
> >              (clobber (reg:QI 52))
> >          ]) "pr53505.c":21:22 -1
> >       (nil))
> > 
> > Classic reload knows about these - find_reg masks out bad_spill_regs, and bad_spill_regs
> > when ORed with chain->live_throughout in order_regs_for_reload picks up r30.
> > 
> > LRA, however, appears to not consider that, and proceeds to use such regs as reload regs.
> > For the same source, it generates
> > <snip>
> >   Choosing alt 0 in insn 15:  (0) =r  (1) r {*movhi_split}
> >        Creating newreg=70, assigning class GENERAL_REGS to r70
> >     15: r26:HI=r70:HI
> >        REG_EQUAL r28:HI+0x1
> >      Inserting insn reload before:
> >     58: r70:HI=r28:HI+0x1
> > 
> > Choosing alt 3 in insn 58:  (0) d  (1) 0  (2) nYnn {*addhi3_split}
> >        Creating newreg=71 from oldreg=70, assigning class LD_REGS to r71
> >     58: r71:HI=r71:HI+0x1
> >      Inserting insn reload before:
> >     59: r71:HI=r28:HI
> >      Inserting insn reload after:
> >     60: r70:HI=r71:HI
> > 
> > ********** Assignment #1: **********
> > 
> >        Assigning to 71 (cl=LD_REGS, orig=70, freq=3000, tfirst=71, tfreq=3000)...
> >          Assign 30 to reload r71 (freq=3000)
> >       Hard reg 26 is preferable by r70 with profit 1000
> >       Hard reg 30 is preferable by r70 with profit 1000
> >        Assigning to 70 (cl=GENERAL_REGS, orig=70, freq=2000, tfirst=70, tfreq=2000)...
> >          Assign 30 to reload r70 (freq=2000)
> > 
> > 
> > (insn 14 13 59 3 (set (reg:HI 30 r30)
> >          (reg:HI 18 r18 [orig:48 ivtmp.10 ] [48])) "pr53505.c":21:22 101 {*movhi_split}
> >       (nil))
> > (insn 59 14 58 3 (set (reg:HI 30 r30 [70])
> >          (reg/f:HI 28 r28)) "pr53505.c":21:22 101 {*movhi_split}
> >       (nil))
> > (insn 58 59 15 3 (set (reg:HI 30 r30 [70])
> >          (plus:HI (reg:HI 30 r30 [70])
> >              (const_int 1 [0x1]))) "pr53505.c":21:22 165 {*addhi3_split}
> >       (nil))
> > (insn 15 58 16 3 (set (reg:HI 26 r26)
> >          (reg:HI 30 r30 [70])) "pr53505.c":21:22 101 {*movhi_split}
> >       (expr_list:REG_EQUAL (plus:HI (reg/f:HI 28 r28)
> >              (const_int 1 [0x1]))
> >          (nil)))
> > (insn 16 15 17 3 (parallel [
> >              (set (mem:BLK (reg:HI 26 r26) [0  A8])
> >                  (mem:BLK (reg:HI 30 r30) [0  A8]))
> >              (unspec [
> >                      (const_int 0 [0])
> >                  ] UNSPEC_CPYMEM)
> >              (use (reg:QI 22 r22 [52]))
> >              (clobber (reg:HI 26 r26))
> >              (clobber (reg:HI 30 r30))
> >              (clobber (reg:QI 0 r0))
> >              (clobber (reg:QI 22 r22 [52]))
> >          ]) "pr53505.c":21:22 132 {cpymem_qi}
> >       (nil))
> > 
> > LRA generates insn 59 that clobbers r30 set in insn 14, causing an execution
> > failure down the line.
> > 
> > How should the avr backend deal with this?
> > 
> Sorry for the big delay with the answer.  I was on vacation.
> 
> There are probably some ways to fix it by changing patterns as other
> people suggested but I'd like to see the current patterns work for LRA
> as well.
> 
> Could you send me the test case on which I could reproduce the problem
> and work on implementing such functionality.
> 
> 
Thanks for taking your time to look at this.

To reproduce the behavior, apply the below patch on master

diff --git gcc/config/avr/avr.cc gcc/config/avr/avr.cc
index 25f3f4c22e0..a9ab8259339 100644
--- gcc/config/avr/avr.cc
+++ gcc/config/avr/avr.cc
@@ -1574,6 +1574,9 @@ avr_allocate_stack_slots_for_args (void)
 static bool
 avr_can_eliminate (const int from ATTRIBUTE_UNUSED, const int to)
 {
+  if (lra_in_progress && to == STACK_POINTER_REGNUM)
+         return false;
+
   return ((frame_pointer_needed && to == FRAME_POINTER_REGNUM)
           || !frame_pointer_needed);
 }
@@ -15246,9 +15249,6 @@ avr_float_lib_compare_returns_bool (machine_mode mode, enum rtx_code)
 #undef  TARGET_CONVERT_TO_TYPE
 #define TARGET_CONVERT_TO_TYPE avr_convert_to_type
 
-#undef TARGET_LRA_P
-#define TARGET_LRA_P hook_bool_void_false
-
 #undef  TARGET_ADDR_SPACE_SUBSET_P
 #define TARGET_ADDR_SPACE_SUBSET_P avr_addr_space_subset_p
 
diff --git gcc/config/avr/avr.h gcc/config/avr/avr.h
index 8e7e00db13b..f82f429b94a 100644
--- gcc/config/avr/avr.h
+++ gcc/config/avr/avr.h
@@ -313,8 +313,7 @@ enum reg_class {
 #define ELIMINABLE_REGS {                                      \
     { ARG_POINTER_REGNUM, STACK_POINTER_REGNUM },               \
     { ARG_POINTER_REGNUM, FRAME_POINTER_REGNUM },               \
-    { FRAME_POINTER_REGNUM, STACK_POINTER_REGNUM },             \
-    { FRAME_POINTER_REGNUM + 1, STACK_POINTER_REGNUM + 1 } }
+    { FRAME_POINTER_REGNUM, STACK_POINTER_REGNUM } }
 
 #define INITIAL_ELIMINATION_OFFSET(FROM, TO, OFFSET)                   \
   OFFSET = avr_initial_elimination_offset (FROM, TO)

Then configure and build for avr without attempting to build libgcc

$ configure --target=avr --prefix=<prefix_dir> --enable-languages=c && make all-host && make install-host

And finally to reproduce the failure

$ cat test.c
#include <stdbool.h>

struct A
{
  unsigned int a;
  unsigned char c1, c2;
  bool b1 : 1;
};

void
foo (const struct A *x, int y)
{
  int s = 0, i;
  for (i = 0; i < y; ++i)
    {
      const struct A a = x[i];
      s += a.b1 ? 1 : 0;
    }
  if (s != 0)
    __builtin_abort ();
}

$ avr-gcc -mmcu=avr51 -Os test.c -S -o - -fdump-rtl-all
You can find the below insn sequence in test.c.*.reload

(insn 13 12 14 3 (set (reg:QI 22 r22 [52])
        (const_int 5 [0x5])) "test.c":16:22 86 {movqi_insn_split}
     (expr_list:REG_EQUAL (const_int 5 [0x5])
        (nil)))
(insn 14 13 59 3 (set (reg:HI 30 r30)
        (reg:HI 18 r18 [orig:48 ivtmp.9 ] [48])) "test.c":16:22 101 {*movhi_split}
     (nil))
(insn 59 14 58 3 (set (reg:HI 30 r30 [70])
        (reg/f:HI 28 r28)) "test.c":16:22 101 {*movhi_split}
     (nil))
(insn 58 59 15 3 (set (reg:HI 30 r30 [70])
        (plus:HI (reg:HI 30 r30 [70])
            (const_int 1 [0x1]))) "test.c":16:22 165 {*addhi3_split}
     (nil))
(insn 15 58 16 3 (set (reg:HI 26 r26)
        (reg:HI 30 r30 [70])) "test.c":16:22 101 {*movhi_split}
     (expr_list:REG_EQUAL (plus:HI (reg/f:HI 28 r28)
            (const_int 1 [0x1]))
        (nil)))
(insn 16 15 17 3 (parallel [
            (set (mem:BLK (reg:HI 26 r26) [0  A8])
                (mem:BLK (reg:HI 30 r30) [0  A8]))
            (unspec [
                    (const_int 0 [0])
                ] UNSPEC_CPYMEM)
            (use (reg:QI 22 r22 [52]))
            (clobber (reg:HI 26 r26))
            (clobber (reg:HI 30 r30))
            (clobber (reg:QI 0 r0))
            (clobber (reg:QI 22 r22 [52]))
        ]) "test.c":16:22 132 {cpymem_qi}
     (nil))

Classic reload produces

(insn 13 12 14 3 (set (reg:QI 22 r22 [52])
        (const_int 5 [0x5])) "test.c":16:22 86 {movqi_insn_split}
     (expr_list:REG_EQUAL (const_int 5 [0x5])
        (nil)))
(insn 14 13 59 3 (set (reg:HI 30 r30)
        (reg:HI 18 r18 [orig:48 ivtmp.9 ] [48])) "test.c":16:22 101 {*movhi_split}
     (nil))
(insn 59 14 15 3 (set (reg:HI 26 r26)
        (reg/f:HI 28 r28)) "test.c":16:22 101 {*movhi_split}
     (nil))
(insn 15 59 16 3 (set (reg:HI 26 r26)
        (plus:HI (reg:HI 26 r26)
            (const_int 1 [0x1]))) "test.c":16:22 165 {*addhi3_split}
     (expr_list:REG_EQUAL (plus:HI (reg/f:HI 28 r28)
            (const_int 1 [0x1]))
        (nil)))
(insn 16 15 17 3 (parallel [
            (set (mem:BLK (reg:HI 26 r26) [0  A8])
                (mem:BLK (reg:HI 30 r30) [0  A8]))
            (unspec [
                    (const_int 0 [0])
                ] UNSPEC_CPYMEM)
            (use (reg:QI 22 r22 [52]))
            (clobber (reg:HI 26 r26))
            (clobber (reg:HI 30 r30))
            (clobber (reg:QI 0 r0))
            (clobber (reg:QI 22 r22 [52]))
        ]) "test.c":16:22 132 {cpymem_qi}
     (nil))

Regards
Senthil