LRA for avr: Maintain live range info for pseudos assigned to FP?

LRA for avr: Maintain live range info for pseudos assigned to FP?

[SenthilKumar.Selvaraj]

Hi,

  One more execution failure for the avr target, this time from
  gcc.c-torture/execute/bitfld-3.c.

  Steps to reproduce

  Enable LRA in avr.cc by removing TARGET_LRA_P hook, build with

$  make all-host && make install-host

  and then

$ avr-gcc gcc/testsuite/gcc.c-torture/execute/bitfld-3.c -S -Os -mmcu=avr51 -fdump-rtl-all

  When lra_update_fp2sp_elimination runs and pseudos assigned to the
  FP have to be spilled to stack slots, they sometimes end up in a
  slot that already has a reg with an overlapping live range.  This is
  because lra_reg_info[regno].live_ranges is NULL for such spilled
  pseudos, and therefore when assign_stack_slot_num_and_sort_pseduos
  checks if lra_intersected_live_ranges_p, it always returns false.

  In the below reload dump, all the pseudos assigned to FP get
  allocated to slot 0. The live ranges for some of them (r1241 for
  e.g.) conflicts with r603 that was originally assigned to slot 0,
  but they still end up in the same slot, causing the execution failure.

<snip>
 r472: [86..87]
 r473: [0..85]
 ...
 r603: [254..655]
 ...
 r1241: [282..283]
 r1242: [268..269]
 r1243: [254..255]
 r1244: [238..239]
 r1245: [222..223]
 r1314: [88..89]
<snip>
Ranges after the compression:
 r591: [0..1]
 r603: [0..1]
 r604: [0..1]
 r605: [0..1]
 r606: [0..1]
 r607: [0..1]
 r623: [0..1]
 r624: [0..1]
 r635: [0..1]
 r636: [0..1]
 r637: [0..1]
 r638: [0..1]
 r639: [0..1]
 r668: [0..1]
 r669: [0..1]
 r670: [0..1]
 r671: [0..1]
 r672: [0..1]
	   Frame pointer can not be eliminated anymore
	   Spilling non-eliminable hard regs: 28 29
	 Spilling r472(28)
	 Spilling r473(28)
	 Spilling r589(29)
	 Spilling r590(28)
	 Spilling r704(29)
	 Spilling r1241(28)
	 Spilling r1242(28)
	 Spilling r1243(28)
	 Spilling r1244(28)
	 Spilling r1245(28)
	 Spilling r1314(28)
  Slot 0 regnos (width = 0):	 603	 1314	 1245	 1244	 1243	 1242	 1241	 704	 590	 589	 473	 472

  Live ranges for those pseudos is NULL because when
  lra_create_live_ranges_1 ran with all_p = false, they were not in
  memory (they were assigned to FP). Computing live range info for
  pseudos assigned to FP fixes the problem for avr,

  Is that the right fix for this problem? After applying the below
  patch, the reload dump looks like this

<snip>
Ranges after the compression:
 r472: [2..3]
 r473: [0..1]
 r589: [16..17]
 r590: [16..17]
 r591: [16..17]
 r603: [10..17]
 r604: [10..17]
 r605: [10..17]
 r606: [10..17]
 r607: [10..17]
 r623: [6..17]
 r624: [6..17]
 r635: [8..17]
 r636: [8..17]
 r637: [8..17]
 r638: [8..17]
 r639: [8..17]
 r668: [6..17]
 r669: [6..17]
 r670: [6..17]
 r671: [6..17]
 r672: [6..17]
 r704: [0..1]
 r1241: [14..15]
 r1242: [12..13]
 r1243: [10..11]
 r1244: [8..9]
 r1245: [6..7]
 r1314: [4..5]
	   Frame pointer can not be eliminated anymore
	   Spilling non-eliminable hard regs: 28 29
	 Spilling r472(28)
	 Spilling r473(28)
	 Spilling r589(29)
	 Spilling r590(28)
	 Spilling r704(29)
	 Spilling r1241(28)
	 Spilling r1242(28)
	 Spilling r1243(28)
	 Spilling r1244(28)
	 Spilling r1245(28)
	 Spilling r1314(28)
  Slot 0 regnos (width = 0):	 603	 1314	 1245	 1244	 473	 472
  Slot 1 regnos (width = 0):	 604	 704
  ...
  Slot 17 regnos (width = 0):	 591	 1243	 1242	 1241
  Slot 18 regnos (width = 0):	 589
  Slot 19 regnos (width = 0):	 590

Regards
Senthil

diff --git a/gcc/lra-lives.cc b/gcc/lra-lives.cc
index f60e564da82..e4289f13979 100644
--- a/gcc/lra-lives.cc
+++ b/gcc/lra-lives.cc
@@ -250,7 +250,17 @@ update_pseudo_point (int regno, int point, enum point_type type)
   if (HARD_REGISTER_NUM_P (regno))
     return;
 
-  if (complete_info_p || lra_get_regno_hard_regno (regno) < 0)
+  /* Pseudos assigned to the FP register could potentially get spilled
+     to stack slots when lra_update_fp2sp_elimination runs, so keep
+     their live range info up to date, even if they aren't in memory
+     right now. */
+  int hard_regno = lra_get_regno_hard_regno (regno);
+  HARD_REG_SET set;
+  CLEAR_HARD_REG_SET(set);
+  add_to_hard_reg_set (&set, Pmode, HARD_FRAME_POINTER_REGNUM);
+
+  if (complete_info_p || hard_regno < 0
+     || overlaps_hard_reg_set_p (set, PSEUDO_REGNO_MODE (regno), hard_regno))
     {
       if (type == DEF_POINT)
        {

Re: LRA for avr: Maintain live range info for pseudos assigned to FP?

[Vladimir Makarov]

On 9/7/23 07:21, SenthilKumar.Selvaraj@microchip.com wrote:
> Hi,
>
>    One more execution failure for the avr target, this time from
>    gcc.c-torture/execute/bitfld-3.c.
>
>    Steps to reproduce
>
>    Enable LRA in avr.cc by removing TARGET_LRA_P hook, build with
>
> $  make all-host && make install-host
>
>    and then
>
> $ avr-gcc gcc/testsuite/gcc.c-torture/execute/bitfld-3.c -S -Os -mmcu=avr51 -fdump-rtl-all
>
>    When lra_update_fp2sp_elimination runs and pseudos assigned to the
>    FP have to be spilled to stack slots, they sometimes end up in a
>    slot that already has a reg with an overlapping live range.  This is
>    because lra_reg_info[regno].live_ranges is NULL for such spilled
>    pseudos, and therefore when assign_stack_slot_num_and_sort_pseduos
>    checks if lra_intersected_live_ranges_p, it always returns false.
>
>    In the below reload dump, all the pseudos assigned to FP get
>    allocated to slot 0. The live ranges for some of them (r1241 for
>    e.g.) conflicts with r603 that was originally assigned to slot 0,
>    but they still end up in the same slot, causing the execution failure.
>
Sorry for the delay with the answer, Senthil.  Avr is unusual target and 
needs some changes in LRA but the changes improves LRA portability.  So 
thank you for your work on porting LRA to AVR.

The patch is ok for me.  The only comment is that making calculation of 
the set only once would be nice. Live range calculation in LRA can take 
a lot of time, code of update_pseudo_point is hot and the worst the set 
will be really used rarely but it is calculated every time.

You can commit the current patch and I'll do it by myself or, if you 
want, you can modify the patch by yourself and submit it for review and 
I'll review as soon as possible.  Either way works for me.

>
> diff --git a/gcc/lra-lives.cc b/gcc/lra-lives.cc
> index f60e564da82..e4289f13979 100644
> --- a/gcc/lra-lives.cc
> +++ b/gcc/lra-lives.cc
> @@ -250,7 +250,17 @@ update_pseudo_point (int regno, int point, enum point_type type)
>     if (HARD_REGISTER_NUM_P (regno))
>       return;
>   
> -  if (complete_info_p || lra_get_regno_hard_regno (regno) < 0)
> +  /* Pseudos assigned to the FP register could potentially get spilled
> +     to stack slots when lra_update_fp2sp_elimination runs, so keep
> +     their live range info up to date, even if they aren't in memory
> +     right now. */
> +  int hard_regno = lra_get_regno_hard_regno (regno);
> +  HARD_REG_SET set;
> +  CLEAR_HARD_REG_SET(set);
> +  add_to_hard_reg_set (&set, Pmode, HARD_FRAME_POINTER_REGNUM);
> +
> +  if (complete_info_p || hard_regno < 0
> +     || overlaps_hard_reg_set_p (set, PSEUDO_REGNO_MODE (regno), hard_regno))
>       {
>         if (type == DEF_POINT)
>          {
>
>

Re: LRA for avr: Maintain live range info for pseudos assigned to FP?

[SenthilKumar.Selvaraj]

On Thu, 2023-10-05 at 15:33 -0400, Vladimir Makarov wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 9/7/23 07:21, SenthilKumar.Selvaraj@microchip.com wrote:
> > Hi,
> > 
> >    One more execution failure for the avr target, this time from
> >    gcc.c-torture/execute/bitfld-3.c.
> > 
> >    Steps to reproduce
> > 
> >    Enable LRA in avr.cc by removing TARGET_LRA_P hook, build with
> > 
> > $  make all-host && make install-host
> > 
> >    and then
> > 
> > $ avr-gcc gcc/testsuite/gcc.c-torture/execute/bitfld-3.c -S -Os -mmcu=avr51 -fdump-rtl-all
> > 
> >    When lra_update_fp2sp_elimination runs and pseudos assigned to the
> >    FP have to be spilled to stack slots, they sometimes end up in a
> >    slot that already has a reg with an overlapping live range.  This is
> >    because lra_reg_info[regno].live_ranges is NULL for such spilled
> >    pseudos, and therefore when assign_stack_slot_num_and_sort_pseduos
> >    checks if lra_intersected_live_ranges_p, it always returns false.
> > 
> >    In the below reload dump, all the pseudos assigned to FP get
> >    allocated to slot 0. The live ranges for some of them (r1241 for
> >    e.g.) conflicts with r603 that was originally assigned to slot 0,
> >    but they still end up in the same slot, causing the execution failure.
> > 
> Sorry for the delay with the answer, Senthil.  Avr is unusual target and
> needs some changes in LRA but the changes improves LRA portability.  So
> thank you for your work on porting LRA to AVR.
> 
> The patch is ok for me.  The only comment is that making calculation of
> the set only once would be nice. Live range calculation in LRA can take
> a lot of time, code of update_pseudo_point is hot and the worst the set
> will be really used rarely but it is calculated every time.
> 
> You can commit the current patch and I'll do it by myself or, if you
> want, you can modify the patch by yourself and submit it for review and
> I'll review as soon as possible.  Either way works for me.

Apologies for the extreme delay in responding - had to sort out some medical issues.

Is it ok if I commit the patch now? I have one more patch in ira.cc, after
which I'm hoping the regression results would be good enough to switch the 
avr target to LRA.

Regards
Senthil

> 
> > diff --git a/gcc/lra-lives.cc b/gcc/lra-lives.cc
> > index f60e564da82..e4289f13979 100644
> > --- a/gcc/lra-lives.cc
> > +++ b/gcc/lra-lives.cc
> > @@ -250,7 +250,17 @@ update_pseudo_point (int regno, int point, enum point_type type)
> >     if (HARD_REGISTER_NUM_P (regno))
> >       return;
> > 
> > -  if (complete_info_p || lra_get_regno_hard_regno (regno) < 0)
> > +  /* Pseudos assigned to the FP register could potentially get spilled
> > +     to stack slots when lra_update_fp2sp_elimination runs, so keep
> > +     their live range info up to date, even if they aren't in memory
> > +     right now. */
> > +  int hard_regno = lra_get_regno_hard_regno (regno);
> > +  HARD_REG_SET set;
> > +  CLEAR_HARD_REG_SET(set);
> > +  add_to_hard_reg_set (&set, Pmode, HARD_FRAME_POINTER_REGNUM);
> > +
> > +  if (complete_info_p || hard_regno < 0
> > +     || overlaps_hard_reg_set_p (set, PSEUDO_REGNO_MODE (regno), hard_regno))
> >       {
> >         if (type == DEF_POINT)
> >          {
> > 
> > 

Re: LRA for avr: Maintain live range info for pseudos assigned to FP?

[Georg-Johann Lay]

Am 20.11.23 um 08:14 schrieb SenthilKumar.Selvaraj--- via Gcc:
> On Thu, 2023-10-05 at 15:33 -0400, Vladimir Makarov wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 9/7/23 07:21, SenthilKumar.Selvaraj@microchip.com wrote:
>>> Hi,
>>>
>>>     One more execution failure for the avr target, this time from
>>>     gcc.c-torture/execute/bitfld-3.c.
>>>
>>>     Steps to reproduce
>>>
>>>     Enable LRA in avr.cc by removing TARGET_LRA_P hook, build with
>>>
>>> $  make all-host && make install-host
>>>
>>>     and then
>>>
>>> $ avr-gcc gcc/testsuite/gcc.c-torture/execute/bitfld-3.c -S -Os -mmcu=avr51 -fdump-rtl-all
>>>
>>>     When lra_update_fp2sp_elimination runs and pseudos assigned to the
>>>     FP have to be spilled to stack slots, they sometimes end up in a
>>>     slot that already has a reg with an overlapping live range.  This is
>>>     because lra_reg_info[regno].live_ranges is NULL for such spilled
>>>     pseudos, and therefore when assign_stack_slot_num_and_sort_pseduos
>>>     checks if lra_intersected_live_ranges_p, it always returns false.
>>>
>>>     In the below reload dump, all the pseudos assigned to FP get
>>>     allocated to slot 0. The live ranges for some of them (r1241 for
>>>     e.g.) conflicts with r603 that was originally assigned to slot 0,
>>>     but they still end up in the same slot, causing the execution failure.
>>>
>> Sorry for the delay with the answer, Senthil.  Avr is unusual target and
>> needs some changes in LRA but the changes improves LRA portability.  So
>> thank you for your work on porting LRA to AVR.
>>
>> The patch is ok for me.  The only comment is that making calculation of
>> the set only once would be nice. Live range calculation in LRA can take
>> a lot of time, code of update_pseudo_point is hot and the worst the set
>> will be really used rarely but it is calculated every time.
>>
>> You can commit the current patch and I'll do it by myself or, if you
>> want, you can modify the patch by yourself and submit it for review and
>> I'll review as soon as possible.  Either way works for me.
> 
> Apologies for the extreme delay in responding - had to sort out some medical issues.
> 
> Is it ok if I commit the patch now? I have one more patch in ira.cc, after
> which I'm hoping the regression results would be good enough to switch the
> avr target to LRA.
> 
> Regards
> Senthil

I have two questions:

1) Is there a command line option to switch back to IRA?

2) Will the X register be used for memory accesses? I am asking because
as far as I understand, there is no replacement for 
LEGITIMIZE_RELOAD_ADDRESS.

Regards,

Johann