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

[Kito Cheng <kito.cheng@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5870 bytes --]

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