RISC-V: Fix bug that XTheadMemPair extension caused fcsr not to be saved and restored before and after interrupt.

The t0 register is used as a temporary register for interrupts, so it needs
special treatment. It is necessary to avoid using "th.ldd" in the interrupt
program to stop the subsequent operation of the t0 register, so they need to
exchange positions in the function "riscv_for_each_saved_reg".

gcc/ChangeLog:

	* config/riscv/riscv.cc (riscv_for_each_saved_reg): Place the interrupt
	operation before the XTheadMemPair.
---
 gcc/config/riscv/riscv.cc                     | 56 +++++++++----------
 .../riscv/xtheadmempair-interrupt-fcsr.c      | 18 ++++++
 2 files changed, 46 insertions(+), 28 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/xtheadmempair-interrupt-fcsr.c


base-commit: e7f4040d9d6ec40c48ada940168885d7dde03af9

On Fri, Nov 10, 2023 at 8:14 AM Jin Ma <jinma@linux.alibaba.com> wrote:
>
> The t0 register is used as a temporary register for interrupts, so it needs
> special treatment. It is necessary to avoid using "th.ldd" in the interrupt
> program to stop the subsequent operation of the t0 register, so they need to
> exchange positions in the function "riscv_for_each_saved_reg".

RISCV_PROLOGUE_TEMP_REGNUM needs indeed to be treated special
in case of ISRs and fcsr. This patch just moves the TARGET_XTHEADMEMPAIR
block after the ISR/fcsr block.

Reviewed-by: Christoph Müllner <christoph.muellner@vrull.eu>

>
> gcc/ChangeLog:
>
>         * config/riscv/riscv.cc (riscv_for_each_saved_reg): Place the interrupt
>         operation before the XTheadMemPair.
> ---
>  gcc/config/riscv/riscv.cc                     | 56 +++++++++----------
>  .../riscv/xtheadmempair-interrupt-fcsr.c      | 18 ++++++
>  2 files changed, 46 insertions(+), 28 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/riscv/xtheadmempair-interrupt-fcsr.c
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index e25692b86fc..fa2d4d4b779 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -6346,6 +6346,34 @@ riscv_for_each_saved_reg (poly_int64 sp_offset, riscv_save_restore_fn fn,
>           && riscv_is_eh_return_data_register (regno))
>         continue;
>
> +      /* In an interrupt function, save and restore some necessary CSRs in the stack
> +        to avoid changes in CSRs.  */
> +      if (regno == RISCV_PROLOGUE_TEMP_REGNUM
> +         && cfun->machine->interrupt_handler_p
> +         && ((TARGET_HARD_FLOAT  && cfun->machine->frame.fmask)
> +             || (TARGET_ZFINX
> +                 && (cfun->machine->frame.mask & ~(1 << RISCV_PROLOGUE_TEMP_REGNUM)))))
> +       {
> +         unsigned int fcsr_size = GET_MODE_SIZE (SImode);
> +         if (!epilogue)
> +           {
> +             riscv_save_restore_reg (word_mode, regno, offset, fn);
> +             offset -= fcsr_size;
> +             emit_insn (gen_riscv_frcsr (RISCV_PROLOGUE_TEMP (SImode)));
> +             riscv_save_restore_reg (SImode, RISCV_PROLOGUE_TEMP_REGNUM,
> +                                     offset, riscv_save_reg);
> +           }
> +         else
> +           {
> +             riscv_save_restore_reg (SImode, RISCV_PROLOGUE_TEMP_REGNUM,
> +                                     offset - fcsr_size, riscv_restore_reg);
> +             emit_insn (gen_riscv_fscsr (RISCV_PROLOGUE_TEMP (SImode)));
> +             riscv_save_restore_reg (word_mode, regno, offset, fn);
> +             offset -= fcsr_size;
> +           }
> +         continue;
> +       }
> +
>        if (TARGET_XTHEADMEMPAIR)
>         {
>           /* Get the next reg/offset pair.  */
> @@ -6376,34 +6404,6 @@ riscv_for_each_saved_reg (poly_int64 sp_offset, riscv_save_restore_fn fn,
>             }
>         }
>
> -      /* In an interrupt function, save and restore some necessary CSRs in the stack
> -        to avoid changes in CSRs.  */
> -      if (regno == RISCV_PROLOGUE_TEMP_REGNUM
> -         && cfun->machine->interrupt_handler_p
> -         && ((TARGET_HARD_FLOAT  && cfun->machine->frame.fmask)
> -             || (TARGET_ZFINX
> -                 && (cfun->machine->frame.mask & ~(1 << RISCV_PROLOGUE_TEMP_REGNUM)))))
> -       {
> -         unsigned int fcsr_size = GET_MODE_SIZE (SImode);
> -         if (!epilogue)
> -           {
> -             riscv_save_restore_reg (word_mode, regno, offset, fn);
> -             offset -= fcsr_size;
> -             emit_insn (gen_riscv_frcsr (RISCV_PROLOGUE_TEMP (SImode)));
> -             riscv_save_restore_reg (SImode, RISCV_PROLOGUE_TEMP_REGNUM,
> -                                     offset, riscv_save_reg);
> -           }
> -         else
> -           {
> -             riscv_save_restore_reg (SImode, RISCV_PROLOGUE_TEMP_REGNUM,
> -                                     offset - fcsr_size, riscv_restore_reg);
> -             emit_insn (gen_riscv_fscsr (RISCV_PROLOGUE_TEMP (SImode)));
> -             riscv_save_restore_reg (word_mode, regno, offset, fn);
> -             offset -= fcsr_size;
> -           }
> -         continue;
> -       }
> -
>        riscv_save_restore_reg (word_mode, regno, offset, fn);
>      }
>
> diff --git a/gcc/testsuite/gcc.target/riscv/xtheadmempair-interrupt-fcsr.c b/gcc/testsuite/gcc.target/riscv/xtheadmempair-interrupt-fcsr.c
> new file mode 100644
> index 00000000000..d06f05f5c7c
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/riscv/xtheadmempair-interrupt-fcsr.c
> @@ -0,0 +1,18 @@
> +/* Verify that fcsr instructions emitted.  */
> +/* { dg-do compile } */
> +/* { dg-require-effective-target hard_float } */
> +/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-g" "-Oz" "-Os" "-flto" } } */
> +/* { dg-options "-march=rv64gc_xtheadmempair -mtune=thead-c906 -funwind-tables" { target { rv64 } } } */
> +/* { dg-options "-march=rv32gc_xtheadmempair -mtune=thead-c906 -funwind-tables" { target { rv32 } } } */
> +
> +
> +extern int foo (void);
> +
> +void __attribute__ ((interrupt))
> +sub (void)
> +{
> +  foo ();
> +}
> +
> +/* { dg-final { scan-assembler-times "frcsr\t" 1 } } */
> +/* { dg-final { scan-assembler-times "fscsr\t" 1 } } */
>
> base-commit: e7f4040d9d6ec40c48ada940168885d7dde03af9
> --
> 2.17.1
>

LGTM

Christoph Müllner <christoph.muellner@vrull.eu>於 2023年11月10日 週五，20:55寫道：

> On Fri, Nov 10, 2023 at 8:14 AM Jin Ma <jinma@linux.alibaba.com> wrote:
> >
> > The t0 register is used as a temporary register for interrupts, so it
> needs
> > special treatment. It is necessary to avoid using "th.ldd" in the
> interrupt
> > program to stop the subsequent operation of the t0 register, so they
> need to
> > exchange positions in the function "riscv_for_each_saved_reg".
>
> RISCV_PROLOGUE_TEMP_REGNUM needs indeed to be treated special
> in case of ISRs and fcsr. This patch just moves the TARGET_XTHEADMEMPAIR
> block after the ISR/fcsr block.
>
> Reviewed-by: Christoph Müllner <christoph.muellner@vrull.eu>
>
> >
> > gcc/ChangeLog:
> >
> >         * config/riscv/riscv.cc (riscv_for_each_saved_reg): Place the
> interrupt
> >         operation before the XTheadMemPair.
> > ---
> >  gcc/config/riscv/riscv.cc                     | 56 +++++++++----------
> >  .../riscv/xtheadmempair-interrupt-fcsr.c      | 18 ++++++
> >  2 files changed, 46 insertions(+), 28 deletions(-)
> >  create mode 100644
> gcc/testsuite/gcc.target/riscv/xtheadmempair-interrupt-fcsr.c
> >
> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> > index e25692b86fc..fa2d4d4b779 100644
> > --- a/gcc/config/riscv/riscv.cc
> > +++ b/gcc/config/riscv/riscv.cc
> > @@ -6346,6 +6346,34 @@ riscv_for_each_saved_reg (poly_int64 sp_offset,
> riscv_save_restore_fn fn,
> >           && riscv_is_eh_return_data_register (regno))
> >         continue;
> >
> > +      /* In an interrupt function, save and restore some necessary CSRs
> in the stack
> > +        to avoid changes in CSRs.  */
> > +      if (regno == RISCV_PROLOGUE_TEMP_REGNUM
> > +         && cfun->machine->interrupt_handler_p
> > +         && ((TARGET_HARD_FLOAT  && cfun->machine->frame.fmask)
> > +             || (TARGET_ZFINX
> > +                 && (cfun->machine->frame.mask & ~(1 <<
> RISCV_PROLOGUE_TEMP_REGNUM)))))
> > +       {
> > +         unsigned int fcsr_size = GET_MODE_SIZE (SImode);
> > +         if (!epilogue)
> > +           {
> > +             riscv_save_restore_reg (word_mode, regno, offset, fn);
> > +             offset -= fcsr_size;
> > +             emit_insn (gen_riscv_frcsr (RISCV_PROLOGUE_TEMP (SImode)));
> > +             riscv_save_restore_reg (SImode, RISCV_PROLOGUE_TEMP_REGNUM,
> > +                                     offset, riscv_save_reg);
> > +           }
> > +         else
> > +           {
> > +             riscv_save_restore_reg (SImode, RISCV_PROLOGUE_TEMP_REGNUM,
> > +                                     offset - fcsr_size,
> riscv_restore_reg);
> > +             emit_insn (gen_riscv_fscsr (RISCV_PROLOGUE_TEMP (SImode)));
> > +             riscv_save_restore_reg (word_mode, regno, offset, fn);
> > +             offset -= fcsr_size;
> > +           }
> > +         continue;
> > +       }
> > +
> >        if (TARGET_XTHEADMEMPAIR)
> >         {
> >           /* Get the next reg/offset pair.  */
> > @@ -6376,34 +6404,6 @@ riscv_for_each_saved_reg (poly_int64 sp_offset,
> riscv_save_restore_fn fn,
> >             }
> >         }
> >
> > -      /* In an interrupt function, save and restore some necessary CSRs
> in the stack
> > -        to avoid changes in CSRs.  */
> > -      if (regno == RISCV_PROLOGUE_TEMP_REGNUM
> > -         && cfun->machine->interrupt_handler_p
> > -         && ((TARGET_HARD_FLOAT  && cfun->machine->frame.fmask)
> > -             || (TARGET_ZFINX
> > -                 && (cfun->machine->frame.mask & ~(1 <<
> RISCV_PROLOGUE_TEMP_REGNUM)))))
> > -       {
> > -         unsigned int fcsr_size = GET_MODE_SIZE (SImode);
> > -         if (!epilogue)
> > -           {
> > -             riscv_save_restore_reg (word_mode, regno, offset, fn);
> > -             offset -= fcsr_size;
> > -             emit_insn (gen_riscv_frcsr (RISCV_PROLOGUE_TEMP (SImode)));
> > -             riscv_save_restore_reg (SImode, RISCV_PROLOGUE_TEMP_REGNUM,
> > -                                     offset, riscv_save_reg);
> > -           }
> > -         else
> > -           {
> > -             riscv_save_restore_reg (SImode, RISCV_PROLOGUE_TEMP_REGNUM,
> > -                                     offset - fcsr_size,
> riscv_restore_reg);
> > -             emit_insn (gen_riscv_fscsr (RISCV_PROLOGUE_TEMP (SImode)));
> > -             riscv_save_restore_reg (word_mode, regno, offset, fn);
> > -             offset -= fcsr_size;
> > -           }
> > -         continue;
> > -       }
> > -
> >        riscv_save_restore_reg (word_mode, regno, offset, fn);
> >      }
> >
> > diff --git
> a/gcc/testsuite/gcc.target/riscv/xtheadmempair-interrupt-fcsr.c
> b/gcc/testsuite/gcc.target/riscv/xtheadmempair-interrupt-fcsr.c
> > new file mode 100644
> > index 00000000000..d06f05f5c7c
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/riscv/xtheadmempair-interrupt-fcsr.c
> > @@ -0,0 +1,18 @@
> > +/* Verify that fcsr instructions emitted.  */
> > +/* { dg-do compile } */
> > +/* { dg-require-effective-target hard_float } */
> > +/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-g" "-Oz" "-Os" "-flto" } }
> */
> > +/* { dg-options "-march=rv64gc_xtheadmempair -mtune=thead-c906
> -funwind-tables" { target { rv64 } } } */
> > +/* { dg-options "-march=rv32gc_xtheadmempair -mtune=thead-c906
> -funwind-tables" { target { rv32 } } } */
> > +
> > +
> > +extern int foo (void);
> > +
> > +void __attribute__ ((interrupt))
> > +sub (void)
> > +{
> > +  foo ();
> > +}
> > +
> > +/* { dg-final { scan-assembler-times "frcsr\t" 1 } } */
> > +/* { dg-final { scan-assembler-times "fscsr\t" 1 } } */
> >
> > base-commit: e7f4040d9d6ec40c48ada940168885d7dde03af9
> > --
> > 2.17.1
> >
>

On Fri, Nov 10, 2023 at 2:20 PM Kito Cheng <kito.cheng@gmail.com> wrote:
>
> LGTM

Committed after shortening the commit message's heading.

>
> Christoph Müllner <christoph.muellner@vrull.eu>於 2023年11月10日 週五，20:55寫道：
>>
>> On Fri, Nov 10, 2023 at 8:14 AM Jin Ma <jinma@linux.alibaba.com> wrote:
>> >
>> > The t0 register is used as a temporary register for interrupts, so it needs
>> > special treatment. It is necessary to avoid using "th.ldd" in the interrupt
>> > program to stop the subsequent operation of the t0 register, so they need to
>> > exchange positions in the function "riscv_for_each_saved_reg".
>>
>> RISCV_PROLOGUE_TEMP_REGNUM needs indeed to be treated special
>> in case of ISRs and fcsr. This patch just moves the TARGET_XTHEADMEMPAIR
>> block after the ISR/fcsr block.
>>
>> Reviewed-by: Christoph Müllner <christoph.muellner@vrull.eu>
>>
>> >
>> > gcc/ChangeLog:
>> >
>> >         * config/riscv/riscv.cc (riscv_for_each_saved_reg): Place the interrupt
>> >         operation before the XTheadMemPair.
>> > ---
>> >  gcc/config/riscv/riscv.cc                     | 56 +++++++++----------
>> >  .../riscv/xtheadmempair-interrupt-fcsr.c      | 18 ++++++
>> >  2 files changed, 46 insertions(+), 28 deletions(-)
>> >  create mode 100644 gcc/testsuite/gcc.target/riscv/xtheadmempair-interrupt-fcsr.c
>> >
>> > diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> > index e25692b86fc..fa2d4d4b779 100644
>> > --- a/gcc/config/riscv/riscv.cc
>> > +++ b/gcc/config/riscv/riscv.cc
>> > @@ -6346,6 +6346,34 @@ riscv_for_each_saved_reg (poly_int64 sp_offset, riscv_save_restore_fn fn,
>> >           && riscv_is_eh_return_data_register (regno))
>> >         continue;
>> >
>> > +      /* In an interrupt function, save and restore some necessary CSRs in the stack
>> > +        to avoid changes in CSRs.  */
>> > +      if (regno == RISCV_PROLOGUE_TEMP_REGNUM
>> > +         && cfun->machine->interrupt_handler_p
>> > +         && ((TARGET_HARD_FLOAT  && cfun->machine->frame.fmask)
>> > +             || (TARGET_ZFINX
>> > +                 && (cfun->machine->frame.mask & ~(1 << RISCV_PROLOGUE_TEMP_REGNUM)))))
>> > +       {
>> > +         unsigned int fcsr_size = GET_MODE_SIZE (SImode);
>> > +         if (!epilogue)
>> > +           {
>> > +             riscv_save_restore_reg (word_mode, regno, offset, fn);
>> > +             offset -= fcsr_size;
>> > +             emit_insn (gen_riscv_frcsr (RISCV_PROLOGUE_TEMP (SImode)));
>> > +             riscv_save_restore_reg (SImode, RISCV_PROLOGUE_TEMP_REGNUM,
>> > +                                     offset, riscv_save_reg);
>> > +           }
>> > +         else
>> > +           {
>> > +             riscv_save_restore_reg (SImode, RISCV_PROLOGUE_TEMP_REGNUM,
>> > +                                     offset - fcsr_size, riscv_restore_reg);
>> > +             emit_insn (gen_riscv_fscsr (RISCV_PROLOGUE_TEMP (SImode)));
>> > +             riscv_save_restore_reg (word_mode, regno, offset, fn);
>> > +             offset -= fcsr_size;
>> > +           }
>> > +         continue;
>> > +       }
>> > +
>> >        if (TARGET_XTHEADMEMPAIR)
>> >         {
>> >           /* Get the next reg/offset pair.  */
>> > @@ -6376,34 +6404,6 @@ riscv_for_each_saved_reg (poly_int64 sp_offset, riscv_save_restore_fn fn,
>> >             }
>> >         }
>> >
>> > -      /* In an interrupt function, save and restore some necessary CSRs in the stack
>> > -        to avoid changes in CSRs.  */
>> > -      if (regno == RISCV_PROLOGUE_TEMP_REGNUM
>> > -         && cfun->machine->interrupt_handler_p
>> > -         && ((TARGET_HARD_FLOAT  && cfun->machine->frame.fmask)
>> > -             || (TARGET_ZFINX
>> > -                 && (cfun->machine->frame.mask & ~(1 << RISCV_PROLOGUE_TEMP_REGNUM)))))
>> > -       {
>> > -         unsigned int fcsr_size = GET_MODE_SIZE (SImode);
>> > -         if (!epilogue)
>> > -           {
>> > -             riscv_save_restore_reg (word_mode, regno, offset, fn);
>> > -             offset -= fcsr_size;
>> > -             emit_insn (gen_riscv_frcsr (RISCV_PROLOGUE_TEMP (SImode)));
>> > -             riscv_save_restore_reg (SImode, RISCV_PROLOGUE_TEMP_REGNUM,
>> > -                                     offset, riscv_save_reg);
>> > -           }
>> > -         else
>> > -           {
>> > -             riscv_save_restore_reg (SImode, RISCV_PROLOGUE_TEMP_REGNUM,
>> > -                                     offset - fcsr_size, riscv_restore_reg);
>> > -             emit_insn (gen_riscv_fscsr (RISCV_PROLOGUE_TEMP (SImode)));
>> > -             riscv_save_restore_reg (word_mode, regno, offset, fn);
>> > -             offset -= fcsr_size;
>> > -           }
>> > -         continue;
>> > -       }
>> > -
>> >        riscv_save_restore_reg (word_mode, regno, offset, fn);
>> >      }
>> >
>> > diff --git a/gcc/testsuite/gcc.target/riscv/xtheadmempair-interrupt-fcsr.c b/gcc/testsuite/gcc.target/riscv/xtheadmempair-interrupt-fcsr.c
>> > new file mode 100644
>> > index 00000000000..d06f05f5c7c
>> > --- /dev/null
>> > +++ b/gcc/testsuite/gcc.target/riscv/xtheadmempair-interrupt-fcsr.c
>> > @@ -0,0 +1,18 @@
>> > +/* Verify that fcsr instructions emitted.  */
>> > +/* { dg-do compile } */
>> > +/* { dg-require-effective-target hard_float } */
>> > +/* { dg-skip-if "" { *-*-* } { "-O0" "-O1" "-g" "-Oz" "-Os" "-flto" } } */
>> > +/* { dg-options "-march=rv64gc_xtheadmempair -mtune=thead-c906 -funwind-tables" { target { rv64 } } } */
>> > +/* { dg-options "-march=rv32gc_xtheadmempair -mtune=thead-c906 -funwind-tables" { target { rv32 } } } */
>> > +
>> > +
>> > +extern int foo (void);
>> > +
>> > +void __attribute__ ((interrupt))
>> > +sub (void)
>> > +{
>> > +  foo ();
>> > +}
>> > +
>> > +/* { dg-final { scan-assembler-times "frcsr\t" 1 } } */
>> > +/* { dg-final { scan-assembler-times "fscsr\t" 1 } } */
>> >
>> > base-commit: e7f4040d9d6ec40c48ada940168885d7dde03af9
>> > --
>> > 2.17.1
>> >