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

[<SenthilKumar.Selvaraj@microchip.com>]

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)
> >          {
> > 
> >