diff mbox series

[v2,02/11] riscv: Restructure callee-saved register save/restore code

Message ID 20221219010838.3878675-3-christoph.muellner@vrull.eu
State Accepted
Headers
Series RISC-V: Add XThead* extension support |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Christoph Müllner Dec. 19, 2022, 1:08 a.m. UTC 
  From: Christoph Müllner <christoph.muellner@vrull.eu>

This patch restructures the loop over the GP registers
which saves/restores then as part of the prologue/epilogue.
No functional change is intended by this patch, but it
offers the possibility to use load-pair/store-pair instructions.

gcc/ChangeLog:

	* config/riscv/riscv.cc (riscv_next_saved_reg): New function.
	(riscv_is_eh_return_data_register): New function.
	(riscv_for_each_saved_reg): Restructure loop.

Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
---
 gcc/config/riscv/riscv.cc | 94 +++++++++++++++++++++++++++------------
 1 file changed, 66 insertions(+), 28 deletions(-)

Comments

Kito Cheng Dec. 19, 2022, 6:30 a.m. UTC | #1 
just one more nit: Use INVALID_REGNUM as sentinel value for
riscv_next_saved_reg, otherwise LGTM, and feel free to commit that
separately :)

On Mon, Dec 19, 2022 at 9:08 AM Christoph Muellner
<christoph.muellner@vrull.eu> wrote:
>
> From: Christoph Müllner <christoph.muellner@vrull.eu>
>
> This patch restructures the loop over the GP registers
> which saves/restores then as part of the prologue/epilogue.
> No functional change is intended by this patch, but it
> offers the possibility to use load-pair/store-pair instructions.
>
> gcc/ChangeLog:
>
>         * config/riscv/riscv.cc (riscv_next_saved_reg): New function.
>         (riscv_is_eh_return_data_register): New function.
>         (riscv_for_each_saved_reg): Restructure loop.
>
> Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> ---
>  gcc/config/riscv/riscv.cc | 94 +++++++++++++++++++++++++++------------
>  1 file changed, 66 insertions(+), 28 deletions(-)
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 6dd2ab2d11e..a8d5e1dac7f 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -4835,6 +4835,49 @@ riscv_save_restore_reg (machine_mode mode, int regno,
>    fn (gen_rtx_REG (mode, regno), mem);
>  }
>
> +/* Return the next register up from REGNO up to LIMIT for the callee
> +   to save or restore.  OFFSET will be adjusted accordingly.
> +   If INC is set, then REGNO will be incremented first.  */
> +
> +static unsigned int
> +riscv_next_saved_reg (unsigned int regno, unsigned int limit,
> +                     HOST_WIDE_INT *offset, bool inc = true)
> +{
> +  if (inc)
> +    regno++;
> +
> +  while (regno <= limit)
> +    {
> +      if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
> +       {
> +         *offset = *offset - UNITS_PER_WORD;
> +         break;
> +       }
> +
> +      regno++;
> +    }
> +  return regno;
> +}
> +
> +/* Return TRUE if provided REGNO is eh return data register.  */
> +
> +static bool
> +riscv_is_eh_return_data_register (unsigned int regno)
> +{
> +  unsigned int i, regnum;
> +
> +  if (!crtl->calls_eh_return)
> +    return false;
> +
> +  for (i = 0; (regnum = EH_RETURN_DATA_REGNO (i)) != INVALID_REGNUM; i++)
> +    if (regno == regnum)
> +      {
> +       return true;
> +      }
> +
> +  return false;
> +}
> +
>  /* Call FN for each register that is saved by the current function.
>     SP_OFFSET is the offset of the current stack pointer from the start
>     of the frame.  */
> @@ -4844,36 +4887,31 @@ riscv_for_each_saved_reg (poly_int64 sp_offset, riscv_save_restore_fn fn,
>                           bool epilogue, bool maybe_eh_return)
>  {
>    HOST_WIDE_INT offset;
> +  unsigned int regno;
> +  unsigned int start = GP_REG_FIRST;
> +  unsigned int limit = GP_REG_LAST;
>
>    /* Save the link register and s-registers. */
> -  offset = (cfun->machine->frame.gp_sp_offset - sp_offset).to_constant ();
> -  for (unsigned int regno = GP_REG_FIRST; regno <= GP_REG_LAST; regno++)
> -    if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
> -      {
> -       bool handle_reg = !cfun->machine->reg_is_wrapped_separately[regno];
> -
> -       /* If this is a normal return in a function that calls the eh_return
> -          builtin, then do not restore the eh return data registers as that
> -          would clobber the return value.  But we do still need to save them
> -          in the prologue, and restore them for an exception return, so we
> -          need special handling here.  */
> -       if (epilogue && !maybe_eh_return && crtl->calls_eh_return)
> -         {
> -           unsigned int i, regnum;
> -
> -           for (i = 0; (regnum = EH_RETURN_DATA_REGNO (i)) != INVALID_REGNUM;
> -                i++)
> -             if (regno == regnum)
> -               {
> -                 handle_reg = FALSE;
> -                 break;
> -               }
> -         }
> -
> -       if (handle_reg)
> -         riscv_save_restore_reg (word_mode, regno, offset, fn);
> -       offset -= UNITS_PER_WORD;
> -      }
> +  offset = (cfun->machine->frame.gp_sp_offset - sp_offset).to_constant ()
> +          + UNITS_PER_WORD;
> +  for (regno = riscv_next_saved_reg (start, limit, &offset, false);
> +       regno <= limit;
> +       regno = riscv_next_saved_reg (regno, limit, &offset))
> +    {
> +      if (cfun->machine->reg_is_wrapped_separately[regno])
> +       continue;
> +
> +      /* If this is a normal return in a function that calls the eh_return
> +        builtin, then do not restore the eh return data registers as that
> +        would clobber the return value.  But we do still need to save them
> +        in the prologue, and restore them for an exception return, so we
> +        need special handling here.  */
> +      if (epilogue && !maybe_eh_return
> +         && riscv_is_eh_return_data_register (regno))
> +       continue;
> +
> +      riscv_save_restore_reg (word_mode, regno, offset, fn);
> +    }
>
>    /* This loop must iterate over the same space as its companion in
>       riscv_compute_frame_info.  */
> --
> 2.38.1
>
Christoph Müllner Dec. 19, 2022, 9:21 a.m. UTC | #2 
On Mon, Dec 19, 2022 at 7:30 AM Kito Cheng <kito.cheng@sifive.com> wrote:

> just one more nit: Use INVALID_REGNUM as sentinel value for
> riscv_next_saved_reg, otherwise LGTM, and feel free to commit that
> separately :)
>

Would this change below be ok?

@@ -5540,7 +5540,7 @@ riscv_next_saved_reg (unsigned int regno, unsigned
int limit,
   if (inc)
     regno++;

-  while (regno <= limit)
+  while (regno <= limit && regno != INVALID_REGNUM)
     {
       if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
        {

Thanks,
Christoph



>
> On Mon, Dec 19, 2022 at 9:08 AM Christoph Muellner
> <christoph.muellner@vrull.eu> wrote:
> >
> > From: Christoph Müllner <christoph.muellner@vrull.eu>
> >
> > This patch restructures the loop over the GP registers
> > which saves/restores then as part of the prologue/epilogue.
> > No functional change is intended by this patch, but it
> > offers the possibility to use load-pair/store-pair instructions.
> >
> > gcc/ChangeLog:
> >
> >         * config/riscv/riscv.cc (riscv_next_saved_reg): New function.
> >         (riscv_is_eh_return_data_register): New function.
> >         (riscv_for_each_saved_reg): Restructure loop.
> >
> > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> > ---
> >  gcc/config/riscv/riscv.cc | 94 +++++++++++++++++++++++++++------------
> >  1 file changed, 66 insertions(+), 28 deletions(-)
> >
> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> > index 6dd2ab2d11e..a8d5e1dac7f 100644
> > --- a/gcc/config/riscv/riscv.cc
> > +++ b/gcc/config/riscv/riscv.cc
> > @@ -4835,6 +4835,49 @@ riscv_save_restore_reg (machine_mode mode, int
> regno,
> >    fn (gen_rtx_REG (mode, regno), mem);
> >  }
> >
> > +/* Return the next register up from REGNO up to LIMIT for the callee
> > +   to save or restore.  OFFSET will be adjusted accordingly.
> > +   If INC is set, then REGNO will be incremented first.  */
> > +
> > +static unsigned int
> > +riscv_next_saved_reg (unsigned int regno, unsigned int limit,
> > +                     HOST_WIDE_INT *offset, bool inc = true)
> > +{
> > +  if (inc)
> > +    regno++;
> > +
> > +  while (regno <= limit)
> > +    {
> > +      if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
> > +       {
> > +         *offset = *offset - UNITS_PER_WORD;
> > +         break;
> > +       }
> > +
> > +      regno++;
> > +    }
> > +  return regno;
> > +}
> > +
> > +/* Return TRUE if provided REGNO is eh return data register.  */
> > +
> > +static bool
> > +riscv_is_eh_return_data_register (unsigned int regno)
> > +{
> > +  unsigned int i, regnum;
> > +
> > +  if (!crtl->calls_eh_return)
> > +    return false;
> > +
> > +  for (i = 0; (regnum = EH_RETURN_DATA_REGNO (i)) != INVALID_REGNUM;
> i++)
> > +    if (regno == regnum)
> > +      {
> > +       return true;
> > +      }
> > +
> > +  return false;
> > +}
> > +
> >  /* Call FN for each register that is saved by the current function.
> >     SP_OFFSET is the offset of the current stack pointer from the start
> >     of the frame.  */
> > @@ -4844,36 +4887,31 @@ riscv_for_each_saved_reg (poly_int64 sp_offset,
> riscv_save_restore_fn fn,
> >                           bool epilogue, bool maybe_eh_return)
> >  {
> >    HOST_WIDE_INT offset;
> > +  unsigned int regno;
> > +  unsigned int start = GP_REG_FIRST;
> > +  unsigned int limit = GP_REG_LAST;
> >
> >    /* Save the link register and s-registers. */
> > -  offset = (cfun->machine->frame.gp_sp_offset - sp_offset).to_constant
> ();
> > -  for (unsigned int regno = GP_REG_FIRST; regno <= GP_REG_LAST; regno++)
> > -    if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
> > -      {
> > -       bool handle_reg =
> !cfun->machine->reg_is_wrapped_separately[regno];
> > -
> > -       /* If this is a normal return in a function that calls the
> eh_return
> > -          builtin, then do not restore the eh return data registers as
> that
> > -          would clobber the return value.  But we do still need to save
> them
> > -          in the prologue, and restore them for an exception return, so
> we
> > -          need special handling here.  */
> > -       if (epilogue && !maybe_eh_return && crtl->calls_eh_return)
> > -         {
> > -           unsigned int i, regnum;
> > -
> > -           for (i = 0; (regnum = EH_RETURN_DATA_REGNO (i)) !=
> INVALID_REGNUM;
> > -                i++)
> > -             if (regno == regnum)
> > -               {
> > -                 handle_reg = FALSE;
> > -                 break;
> > -               }
> > -         }
> > -
> > -       if (handle_reg)
> > -         riscv_save_restore_reg (word_mode, regno, offset, fn);
> > -       offset -= UNITS_PER_WORD;
> > -      }
> > +  offset = (cfun->machine->frame.gp_sp_offset - sp_offset).to_constant
> ()
> > +          + UNITS_PER_WORD;
> > +  for (regno = riscv_next_saved_reg (start, limit, &offset, false);
> > +       regno <= limit;
> > +       regno = riscv_next_saved_reg (regno, limit, &offset))
> > +    {
> > +      if (cfun->machine->reg_is_wrapped_separately[regno])
> > +       continue;
> > +
> > +      /* If this is a normal return in a function that calls the
> eh_return
> > +        builtin, then do not restore the eh return data registers as
> that
> > +        would clobber the return value.  But we do still need to save
> them
> > +        in the prologue, and restore them for an exception return, so we
> > +        need special handling here.  */
> > +      if (epilogue && !maybe_eh_return
> > +         && riscv_is_eh_return_data_register (regno))
> > +       continue;
> > +
> > +      riscv_save_restore_reg (word_mode, regno, offset, fn);
> > +    }
> >
> >    /* This loop must iterate over the same space as its companion in
> >       riscv_compute_frame_info.  */
> > --
> > 2.38.1
> >
>
Kito Cheng Dec. 19, 2022, 9:26 a.m. UTC | #3 
Something like this:

static unsigned int
riscv_next_saved_reg (unsigned int regno, unsigned int limit,
                     HOST_WIDE_INT *offset, bool inc = true)
{
  if (inc)
    regno++;

  while (regno <= limit)
    {
      if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
       {
         *offset = *offset - UNITS_PER_WORD;
         break;
       }

      regno++;
    }
  if (regno >= limit)
    return INVALID_REGNUM;
  else
    return regno;
}
...

  for (regno = riscv_next_saved_reg (start, limit, &offset, false);
       regno != INVALID_REGNUM;
       regno = riscv_next_saved_reg (regno, limit, &offset))
    {
...

On Mon, Dec 19, 2022 at 5:21 PM Christoph Müllner
<christoph.muellner@vrull.eu> wrote:
>
>
>
> On Mon, Dec 19, 2022 at 7:30 AM Kito Cheng <kito.cheng@sifive.com> wrote:
>>
>> just one more nit: Use INVALID_REGNUM as sentinel value for
>> riscv_next_saved_reg, otherwise LGTM, and feel free to commit that
>> separately :)
>
>
> Would this change below be ok?
>
> @@ -5540,7 +5540,7 @@ riscv_next_saved_reg (unsigned int regno, unsigned int limit,
>    if (inc)
>      regno++;
>
> -  while (regno <= limit)
> +  while (regno <= limit && regno != INVALID_REGNUM)
>      {
>        if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
>         {
>
> Thanks,
> Christoph
>
>
>>
>>
>> On Mon, Dec 19, 2022 at 9:08 AM Christoph Muellner
>> <christoph.muellner@vrull.eu> wrote:
>> >
>> > From: Christoph Müllner <christoph.muellner@vrull.eu>
>> >
>> > This patch restructures the loop over the GP registers
>> > which saves/restores then as part of the prologue/epilogue.
>> > No functional change is intended by this patch, but it
>> > offers the possibility to use load-pair/store-pair instructions.
>> >
>> > gcc/ChangeLog:
>> >
>> >         * config/riscv/riscv.cc (riscv_next_saved_reg): New function.
>> >         (riscv_is_eh_return_data_register): New function.
>> >         (riscv_for_each_saved_reg): Restructure loop.
>> >
>> > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
>> > ---
>> >  gcc/config/riscv/riscv.cc | 94 +++++++++++++++++++++++++++------------
>> >  1 file changed, 66 insertions(+), 28 deletions(-)
>> >
>> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> > index 6dd2ab2d11e..a8d5e1dac7f 100644
>> > --- a/gcc/config/riscv/riscv.cc
>> > +++ b/gcc/config/riscv/riscv.cc
>> > @@ -4835,6 +4835,49 @@ riscv_save_restore_reg (machine_mode mode, int regno,
>> >    fn (gen_rtx_REG (mode, regno), mem);
>> >  }
>> >
>> > +/* Return the next register up from REGNO up to LIMIT for the callee
>> > +   to save or restore.  OFFSET will be adjusted accordingly.
>> > +   If INC is set, then REGNO will be incremented first.  */
>> > +
>> > +static unsigned int
>> > +riscv_next_saved_reg (unsigned int regno, unsigned int limit,
>> > +                     HOST_WIDE_INT *offset, bool inc = true)
>> > +{
>> > +  if (inc)
>> > +    regno++;
>> > +
>> > +  while (regno <= limit)
>> > +    {
>> > +      if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
>> > +       {
>> > +         *offset = *offset - UNITS_PER_WORD;
>> > +         break;
>> > +       }
>> > +
>> > +      regno++;
>> > +    }
>> > +  return regno;
>> > +}
>> > +
>> > +/* Return TRUE if provided REGNO is eh return data register.  */
>> > +
>> > +static bool
>> > +riscv_is_eh_return_data_register (unsigned int regno)
>> > +{
>> > +  unsigned int i, regnum;
>> > +
>> > +  if (!crtl->calls_eh_return)
>> > +    return false;
>> > +
>> > +  for (i = 0; (regnum = EH_RETURN_DATA_REGNO (i)) != INVALID_REGNUM; i++)
>> > +    if (regno == regnum)
>> > +      {
>> > +       return true;
>> > +      }
>> > +
>> > +  return false;
>> > +}
>> > +
>> >  /* Call FN for each register that is saved by the current function.
>> >     SP_OFFSET is the offset of the current stack pointer from the start
>> >     of the frame.  */
>> > @@ -4844,36 +4887,31 @@ riscv_for_each_saved_reg (poly_int64 sp_offset, riscv_save_restore_fn fn,
>> >                           bool epilogue, bool maybe_eh_return)
>> >  {
>> >    HOST_WIDE_INT offset;
>> > +  unsigned int regno;
>> > +  unsigned int start = GP_REG_FIRST;
>> > +  unsigned int limit = GP_REG_LAST;
>> >
>> >    /* Save the link register and s-registers. */
>> > -  offset = (cfun->machine->frame.gp_sp_offset - sp_offset).to_constant ();
>> > -  for (unsigned int regno = GP_REG_FIRST; regno <= GP_REG_LAST; regno++)
>> > -    if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
>> > -      {
>> > -       bool handle_reg = !cfun->machine->reg_is_wrapped_separately[regno];
>> > -
>> > -       /* If this is a normal return in a function that calls the eh_return
>> > -          builtin, then do not restore the eh return data registers as that
>> > -          would clobber the return value.  But we do still need to save them
>> > -          in the prologue, and restore them for an exception return, so we
>> > -          need special handling here.  */
>> > -       if (epilogue && !maybe_eh_return && crtl->calls_eh_return)
>> > -         {
>> > -           unsigned int i, regnum;
>> > -
>> > -           for (i = 0; (regnum = EH_RETURN_DATA_REGNO (i)) != INVALID_REGNUM;
>> > -                i++)
>> > -             if (regno == regnum)
>> > -               {
>> > -                 handle_reg = FALSE;
>> > -                 break;
>> > -               }
>> > -         }
>> > -
>> > -       if (handle_reg)
>> > -         riscv_save_restore_reg (word_mode, regno, offset, fn);
>> > -       offset -= UNITS_PER_WORD;
>> > -      }
>> > +  offset = (cfun->machine->frame.gp_sp_offset - sp_offset).to_constant ()
>> > +          + UNITS_PER_WORD;
>> > +  for (regno = riscv_next_saved_reg (start, limit, &offset, false);
>> > +       regno <= limit;
>> > +       regno = riscv_next_saved_reg (regno, limit, &offset))
>> > +    {
>> > +      if (cfun->machine->reg_is_wrapped_separately[regno])
>> > +       continue;
>> > +
>> > +      /* If this is a normal return in a function that calls the eh_return
>> > +        builtin, then do not restore the eh return data registers as that
>> > +        would clobber the return value.  But we do still need to save them
>> > +        in the prologue, and restore them for an exception return, so we
>> > +        need special handling here.  */
>> > +      if (epilogue && !maybe_eh_return
>> > +         && riscv_is_eh_return_data_register (regno))
>> > +       continue;
>> > +
>> > +      riscv_save_restore_reg (word_mode, regno, offset, fn);
>> > +    }
>> >
>> >    /* This loop must iterate over the same space as its companion in
>> >       riscv_compute_frame_info.  */
>> > --
>> > 2.38.1
>> >
Christoph Müllner Dec. 19, 2022, 9:45 a.m. UTC | #4 
On Mon, Dec 19, 2022 at 10:26 AM Kito Cheng <kito.cheng@sifive.com> wrote:

> Something like this:
>
> static unsigned int
> riscv_next_saved_reg (unsigned int regno, unsigned int limit,
>                      HOST_WIDE_INT *offset, bool inc = true)
> {
>   if (inc)
>     regno++;
>
>   while (regno <= limit)
>     {
>       if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
>        {
>          *offset = *offset - UNITS_PER_WORD;
>          break;
>        }
>
>       regno++;
>     }
>   if (regno >= limit)
>     return INVALID_REGNUM;
>   else
>     return regno;
> }
> ...
>
>   for (regno = riscv_next_saved_reg (start, limit, &offset, false);
>        regno != INVALID_REGNUM;
>        regno = riscv_next_saved_reg (regno, limit, &offset))
>     {
> ...
>
>
Ok, I see.
I changed it as follows (it will be retested before committing):

@@ -5531,7 +5531,8 @@ riscv_save_restore_reg (machine_mode mode, int regno,

 /* Return the next register up from REGNO up to LIMIT for the callee
    to save or restore.  OFFSET will be adjusted accordingly.
-   If INC is set, then REGNO will be incremented first.  */
+   If INC is set, then REGNO will be incremented first.
+   Returns INVALID_REGNUM if there is no such next register.  */

 static unsigned int
 riscv_next_saved_reg (unsigned int regno, unsigned int limit,
@@ -5545,12 +5546,12 @@ riscv_next_saved_reg (unsigned int regno, unsigned
int limit,
       if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
        {
          *offset = *offset - UNITS_PER_WORD;
-         break;
+         return regno;
        }

       regno++;
     }
-  return regno;
+  return INVALID_REGNUM;
 }

 /* Return TRUE if provided REGNO is eh return data register.  */
@@ -5589,7 +5590,7 @@ riscv_for_each_saved_reg (poly_int64 sp_offset,
riscv_save_restore_fn fn,
   offset = (cfun->machine->frame.gp_sp_offset - sp_offset).to_constant ()
           + UNITS_PER_WORD;
   for (regno = riscv_next_saved_reg (start, limit, &offset, false);
-       regno <= limit;
+       regno != INVALID_REGNUM;

Thanks!



> On Mon, Dec 19, 2022 at 5:21 PM Christoph Müllner
> <christoph.muellner@vrull.eu> wrote:
> >
> >
> >
> > On Mon, Dec 19, 2022 at 7:30 AM Kito Cheng <kito.cheng@sifive.com>
> wrote:
> >>
> >> just one more nit: Use INVALID_REGNUM as sentinel value for
> >> riscv_next_saved_reg, otherwise LGTM, and feel free to commit that
> >> separately :)
> >
> >
> > Would this change below be ok?
> >
> > @@ -5540,7 +5540,7 @@ riscv_next_saved_reg (unsigned int regno, unsigned
> int limit,
> >    if (inc)
> >      regno++;
> >
> > -  while (regno <= limit)
> > +  while (regno <= limit && regno != INVALID_REGNUM)
> >      {
> >        if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
> >         {
> >
> > Thanks,
> > Christoph
> >
> >
> >>
> >>
> >> On Mon, Dec 19, 2022 at 9:08 AM Christoph Muellner
> >> <christoph.muellner@vrull.eu> wrote:
> >> >
> >> > From: Christoph Müllner <christoph.muellner@vrull.eu>
> >> >
> >> > This patch restructures the loop over the GP registers
> >> > which saves/restores then as part of the prologue/epilogue.
> >> > No functional change is intended by this patch, but it
> >> > offers the possibility to use load-pair/store-pair instructions.
> >> >
> >> > gcc/ChangeLog:
> >> >
> >> >         * config/riscv/riscv.cc (riscv_next_saved_reg): New function.
> >> >         (riscv_is_eh_return_data_register): New function.
> >> >         (riscv_for_each_saved_reg): Restructure loop.
> >> >
> >> > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> >> > ---
> >> >  gcc/config/riscv/riscv.cc | 94
> +++++++++++++++++++++++++++------------
> >> >  1 file changed, 66 insertions(+), 28 deletions(-)
> >> >
> >> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> >> > index 6dd2ab2d11e..a8d5e1dac7f 100644
> >> > --- a/gcc/config/riscv/riscv.cc
> >> > +++ b/gcc/config/riscv/riscv.cc
> >> > @@ -4835,6 +4835,49 @@ riscv_save_restore_reg (machine_mode mode, int
> regno,
> >> >    fn (gen_rtx_REG (mode, regno), mem);
> >> >  }
> >> >
> >> > +/* Return the next register up from REGNO up to LIMIT for the callee
> >> > +   to save or restore.  OFFSET will be adjusted accordingly.
> >> > +   If INC is set, then REGNO will be incremented first.  */
> >> > +
> >> > +static unsigned int
> >> > +riscv_next_saved_reg (unsigned int regno, unsigned int limit,
> >> > +                     HOST_WIDE_INT *offset, bool inc = true)
> >> > +{
> >> > +  if (inc)
> >> > +    regno++;
> >> > +
> >> > +  while (regno <= limit)
> >> > +    {
> >> > +      if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
> >> > +       {
> >> > +         *offset = *offset - UNITS_PER_WORD;
> >> > +         break;
> >> > +       }
> >> > +
> >> > +      regno++;
> >> > +    }
> >> > +  return regno;
> >> > +}
> >> > +
> >> > +/* Return TRUE if provided REGNO is eh return data register.  */
> >> > +
> >> > +static bool
> >> > +riscv_is_eh_return_data_register (unsigned int regno)
> >> > +{
> >> > +  unsigned int i, regnum;
> >> > +
> >> > +  if (!crtl->calls_eh_return)
> >> > +    return false;
> >> > +
> >> > +  for (i = 0; (regnum = EH_RETURN_DATA_REGNO (i)) != INVALID_REGNUM;
> i++)
> >> > +    if (regno == regnum)
> >> > +      {
> >> > +       return true;
> >> > +      }
> >> > +
> >> > +  return false;
> >> > +}
> >> > +
> >> >  /* Call FN for each register that is saved by the current function.
> >> >     SP_OFFSET is the offset of the current stack pointer from the
> start
> >> >     of the frame.  */
> >> > @@ -4844,36 +4887,31 @@ riscv_for_each_saved_reg (poly_int64
> sp_offset, riscv_save_restore_fn fn,
> >> >                           bool epilogue, bool maybe_eh_return)
> >> >  {
> >> >    HOST_WIDE_INT offset;
> >> > +  unsigned int regno;
> >> > +  unsigned int start = GP_REG_FIRST;
> >> > +  unsigned int limit = GP_REG_LAST;
> >> >
> >> >    /* Save the link register and s-registers. */
> >> > -  offset = (cfun->machine->frame.gp_sp_offset -
> sp_offset).to_constant ();
> >> > -  for (unsigned int regno = GP_REG_FIRST; regno <= GP_REG_LAST;
> regno++)
> >> > -    if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
> >> > -      {
> >> > -       bool handle_reg =
> !cfun->machine->reg_is_wrapped_separately[regno];
> >> > -
> >> > -       /* If this is a normal return in a function that calls the
> eh_return
> >> > -          builtin, then do not restore the eh return data registers
> as that
> >> > -          would clobber the return value.  But we do still need to
> save them
> >> > -          in the prologue, and restore them for an exception return,
> so we
> >> > -          need special handling here.  */
> >> > -       if (epilogue && !maybe_eh_return && crtl->calls_eh_return)
> >> > -         {
> >> > -           unsigned int i, regnum;
> >> > -
> >> > -           for (i = 0; (regnum = EH_RETURN_DATA_REGNO (i)) !=
> INVALID_REGNUM;
> >> > -                i++)
> >> > -             if (regno == regnum)
> >> > -               {
> >> > -                 handle_reg = FALSE;
> >> > -                 break;
> >> > -               }
> >> > -         }
> >> > -
> >> > -       if (handle_reg)
> >> > -         riscv_save_restore_reg (word_mode, regno, offset, fn);
> >> > -       offset -= UNITS_PER_WORD;
> >> > -      }
> >> > +  offset = (cfun->machine->frame.gp_sp_offset -
> sp_offset).to_constant ()
> >> > +          + UNITS_PER_WORD;
> >> > +  for (regno = riscv_next_saved_reg (start, limit, &offset, false);
> >> > +       regno <= limit;
> >> > +       regno = riscv_next_saved_reg (regno, limit, &offset))
> >> > +    {
> >> > +      if (cfun->machine->reg_is_wrapped_separately[regno])
> >> > +       continue;
> >> > +
> >> > +      /* If this is a normal return in a function that calls the
> eh_return
> >> > +        builtin, then do not restore the eh return data registers as
> that
> >> > +        would clobber the return value.  But we do still need to
> save them
> >> > +        in the prologue, and restore them for an exception return,
> so we
> >> > +        need special handling here.  */
> >> > +      if (epilogue && !maybe_eh_return
> >> > +         && riscv_is_eh_return_data_register (regno))
> >> > +       continue;
> >> > +
> >> > +      riscv_save_restore_reg (word_mode, regno, offset, fn);
> >> > +    }
> >> >
> >> >    /* This loop must iterate over the same space as its companion in
> >> >       riscv_compute_frame_info.  */
> >> > --
> >> > 2.38.1
> >> >
>
Philipp Tomsich Dec. 27, 2022, 7:51 p.m. UTC | #5 
Applied to master (with the change from the reviews), thanks!

Philipp.

On Mon, 19 Dec 2022 at 07:30, Kito Cheng <kito.cheng@sifive.com> wrote:

> just one more nit: Use INVALID_REGNUM as sentinel value for
> riscv_next_saved_reg, otherwise LGTM, and feel free to commit that
> separately :)
>
> On Mon, Dec 19, 2022 at 9:08 AM Christoph Muellner
> <christoph.muellner@vrull.eu> wrote:
> >
> > From: Christoph Müllner <christoph.muellner@vrull.eu>
> >
> > This patch restructures the loop over the GP registers
> > which saves/restores then as part of the prologue/epilogue.
> > No functional change is intended by this patch, but it
> > offers the possibility to use load-pair/store-pair instructions.
> >
> > gcc/ChangeLog:
> >
> >         * config/riscv/riscv.cc (riscv_next_saved_reg): New function.
> >         (riscv_is_eh_return_data_register): New function.
> >         (riscv_for_each_saved_reg): Restructure loop.
> >
> > Signed-off-by: Christoph Müllner <christoph.muellner@vrull.eu>
> > ---
> >  gcc/config/riscv/riscv.cc | 94 +++++++++++++++++++++++++++------------
> >  1 file changed, 66 insertions(+), 28 deletions(-)
> >
> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> > index 6dd2ab2d11e..a8d5e1dac7f 100644
> > --- a/gcc/config/riscv/riscv.cc
> > +++ b/gcc/config/riscv/riscv.cc
> > @@ -4835,6 +4835,49 @@ riscv_save_restore_reg (machine_mode mode, int
> regno,
> >    fn (gen_rtx_REG (mode, regno), mem);
> >  }
> >
> > +/* Return the next register up from REGNO up to LIMIT for the callee
> > +   to save or restore.  OFFSET will be adjusted accordingly.
> > +   If INC is set, then REGNO will be incremented first.  */
> > +
> > +static unsigned int
> > +riscv_next_saved_reg (unsigned int regno, unsigned int limit,
> > +                     HOST_WIDE_INT *offset, bool inc = true)
> > +{
> > +  if (inc)
> > +    regno++;
> > +
> > +  while (regno <= limit)
> > +    {
> > +      if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
> > +       {
> > +         *offset = *offset - UNITS_PER_WORD;
> > +         break;
> > +       }
> > +
> > +      regno++;
> > +    }
> > +  return regno;
> > +}
> > +
> > +/* Return TRUE if provided REGNO is eh return data register.  */
> > +
> > +static bool
> > +riscv_is_eh_return_data_register (unsigned int regno)
> > +{
> > +  unsigned int i, regnum;
> > +
> > +  if (!crtl->calls_eh_return)
> > +    return false;
> > +
> > +  for (i = 0; (regnum = EH_RETURN_DATA_REGNO (i)) != INVALID_REGNUM;
> i++)
> > +    if (regno == regnum)
> > +      {
> > +       return true;
> > +      }
> > +
> > +  return false;
> > +}
> > +
> >  /* Call FN for each register that is saved by the current function.
> >     SP_OFFSET is the offset of the current stack pointer from the start
> >     of the frame.  */
> > @@ -4844,36 +4887,31 @@ riscv_for_each_saved_reg (poly_int64 sp_offset,
> riscv_save_restore_fn fn,
> >                           bool epilogue, bool maybe_eh_return)
> >  {
> >    HOST_WIDE_INT offset;
> > +  unsigned int regno;
> > +  unsigned int start = GP_REG_FIRST;
> > +  unsigned int limit = GP_REG_LAST;
> >
> >    /* Save the link register and s-registers. */
> > -  offset = (cfun->machine->frame.gp_sp_offset - sp_offset).to_constant
> ();
> > -  for (unsigned int regno = GP_REG_FIRST; regno <= GP_REG_LAST; regno++)
> > -    if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
> > -      {
> > -       bool handle_reg =
> !cfun->machine->reg_is_wrapped_separately[regno];
> > -
> > -       /* If this is a normal return in a function that calls the
> eh_return
> > -          builtin, then do not restore the eh return data registers as
> that
> > -          would clobber the return value.  But we do still need to save
> them
> > -          in the prologue, and restore them for an exception return, so
> we
> > -          need special handling here.  */
> > -       if (epilogue && !maybe_eh_return && crtl->calls_eh_return)
> > -         {
> > -           unsigned int i, regnum;
> > -
> > -           for (i = 0; (regnum = EH_RETURN_DATA_REGNO (i)) !=
> INVALID_REGNUM;
> > -                i++)
> > -             if (regno == regnum)
> > -               {
> > -                 handle_reg = FALSE;
> > -                 break;
> > -               }
> > -         }
> > -
> > -       if (handle_reg)
> > -         riscv_save_restore_reg (word_mode, regno, offset, fn);
> > -       offset -= UNITS_PER_WORD;
> > -      }
> > +  offset = (cfun->machine->frame.gp_sp_offset - sp_offset).to_constant
> ()
> > +          + UNITS_PER_WORD;
> > +  for (regno = riscv_next_saved_reg (start, limit, &offset, false);
> > +       regno <= limit;
> > +       regno = riscv_next_saved_reg (regno, limit, &offset))
> > +    {
> > +      if (cfun->machine->reg_is_wrapped_separately[regno])
> > +       continue;
> > +
> > +      /* If this is a normal return in a function that calls the
> eh_return
> > +        builtin, then do not restore the eh return data registers as
> that
> > +        would clobber the return value.  But we do still need to save
> them
> > +        in the prologue, and restore them for an exception return, so we
> > +        need special handling here.  */
> > +      if (epilogue && !maybe_eh_return
> > +         && riscv_is_eh_return_data_register (regno))
> > +       continue;
> > +
> > +      riscv_save_restore_reg (word_mode, regno, offset, fn);
> > +    }
> >
> >    /* This loop must iterate over the same space as its companion in
> >       riscv_compute_frame_info.  */
> > --
> > 2.38.1
> >
>
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 6dd2ab2d11e..a8d5e1dac7f 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -4835,6 +4835,49 @@  riscv_save_restore_reg (machine_mode mode, int regno,
   fn (gen_rtx_REG (mode, regno), mem);
 }
 
+/* Return the next register up from REGNO up to LIMIT for the callee
+   to save or restore.  OFFSET will be adjusted accordingly.
+   If INC is set, then REGNO will be incremented first.  */
+
+static unsigned int
+riscv_next_saved_reg (unsigned int regno, unsigned int limit,
+		      HOST_WIDE_INT *offset, bool inc = true)
+{
+  if (inc)
+    regno++;
+
+  while (regno <= limit)
+    {
+      if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
+	{
+	  *offset = *offset - UNITS_PER_WORD;
+	  break;
+	}
+
+      regno++;
+    }
+  return regno;
+}
+
+/* Return TRUE if provided REGNO is eh return data register.  */
+
+static bool
+riscv_is_eh_return_data_register (unsigned int regno)
+{
+  unsigned int i, regnum;
+
+  if (!crtl->calls_eh_return)
+    return false;
+
+  for (i = 0; (regnum = EH_RETURN_DATA_REGNO (i)) != INVALID_REGNUM; i++)
+    if (regno == regnum)
+      {
+	return true;
+      }
+
+  return false;
+}
+
 /* Call FN for each register that is saved by the current function.
    SP_OFFSET is the offset of the current stack pointer from the start
    of the frame.  */
@@ -4844,36 +4887,31 @@  riscv_for_each_saved_reg (poly_int64 sp_offset, riscv_save_restore_fn fn,
 			  bool epilogue, bool maybe_eh_return)
 {
   HOST_WIDE_INT offset;
+  unsigned int regno;
+  unsigned int start = GP_REG_FIRST;
+  unsigned int limit = GP_REG_LAST;
 
   /* Save the link register and s-registers. */
-  offset = (cfun->machine->frame.gp_sp_offset - sp_offset).to_constant ();
-  for (unsigned int regno = GP_REG_FIRST; regno <= GP_REG_LAST; regno++)
-    if (BITSET_P (cfun->machine->frame.mask, regno - GP_REG_FIRST))
-      {
-	bool handle_reg = !cfun->machine->reg_is_wrapped_separately[regno];
-
-	/* If this is a normal return in a function that calls the eh_return
-	   builtin, then do not restore the eh return data registers as that
-	   would clobber the return value.  But we do still need to save them
-	   in the prologue, and restore them for an exception return, so we
-	   need special handling here.  */
-	if (epilogue && !maybe_eh_return && crtl->calls_eh_return)
-	  {
-	    unsigned int i, regnum;
-
-	    for (i = 0; (regnum = EH_RETURN_DATA_REGNO (i)) != INVALID_REGNUM;
-		 i++)
-	      if (regno == regnum)
-		{
-		  handle_reg = FALSE;
-		  break;
-		}
-	  }
-
-	if (handle_reg)
-	  riscv_save_restore_reg (word_mode, regno, offset, fn);
-	offset -= UNITS_PER_WORD;
-      }
+  offset = (cfun->machine->frame.gp_sp_offset - sp_offset).to_constant ()
+	   + UNITS_PER_WORD;
+  for (regno = riscv_next_saved_reg (start, limit, &offset, false);
+       regno <= limit;
+       regno = riscv_next_saved_reg (regno, limit, &offset))
+    {
+      if (cfun->machine->reg_is_wrapped_separately[regno])
+	continue;
+
+      /* If this is a normal return in a function that calls the eh_return
+	 builtin, then do not restore the eh return data registers as that
+	 would clobber the return value.  But we do still need to save them
+	 in the prologue, and restore them for an exception return, so we
+	 need special handling here.  */
+      if (epilogue && !maybe_eh_return
+	  && riscv_is_eh_return_data_register (regno))
+	continue;
+
+      riscv_save_restore_reg (word_mode, regno, offset, fn);
+    }
 
   /* This loop must iterate over the same space as its companion in
      riscv_compute_frame_info.  */