[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gcc
Subject:    Re: LRA for avr: Handling hard regs set directly at expand
From:       Georg-Johann Lay <avr () gjlay ! de>
Date:       2023-07-28 8:11:12
Message-ID: 595f2158-3d38-dcfd-fe62-b0983a90d038 () gjlay ! de
[Download RAW message or body]



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
[prev in list] [next in list] [prev in thread] [next in thread]