Re: [PATCH] RISC-V: Save and restore FCSR in interrupt functions to avoid program errors.

[Palmer Dabbelt]

On Tue, 13 Jun 2023 10:41:00 PDT (-0700), gcc-patches@gcc.gnu.org wrote:


On 6/13/23 00:41, Jin Ma wrote:
gcc/ChangeLog:

        * config/riscv/riscv.cc (riscv_compute_frame_info): Allocate frame for 
FCSR.
        (riscv_for_each_saved_reg): Save and restore FCSR in interrupt 
functions.
        * config/riscv/riscv.md (riscv_frcsr): New patterns.
        (riscv_fscsr): Likewise.

gcc/testsuite/ChangeLog:

        * gcc.target/riscv/interrupt-fcsr-1.c: New test.
        * gcc.target/riscv/interrupt-fcsr-2.c: New test.
        * gcc.target/riscv/interrupt-fcsr-3.c: New test.
Looks pretty good.  Just a couple minor updates and I think we can push
this to the trunk.

We should update the C API doc as well, it's a bit vague as to whether 
the CSRs are saved: it just says the any used registers are saved, it's 
not clear if registers includes CSRs.

Unless I'm missing something, we also need to save/restore the V CSRs in 
interrupt functions as well?  They're treated the same way in the C API 
doc, so applying the same logic seems reasonable -- I'm not sure we 
really want to save/restore something like vstart, though...

I opened a PR for the API doc: 
https://github.com/riscv-non-isa/riscv-c-api-doc/pull/42

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index de30bf4e567..4ef9692b4db 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -4990,7 +4990,8 @@ riscv_compute_frame_info (void)
    if (cfun->machine->interrupt_handler_p)
      {
        HOST_WIDE_INT step1 = riscv_first_stack_step (frame, frame->total_size);
-      if (! POLY_SMALL_OPERAND_P ((frame->total_size - step1)))
+      if (! POLY_SMALL_OPERAND_P ((frame->total_size - step1))
+         || TARGET_HARD_FLOAT)
        interrupt_save_prologue_temp = true;
      }
There's a comment before this IF block indicating when we need to save
the prologue temporary register (specifically in interrupt functions
with large frames).  That comment needs to be updated so that it
mentions interrupt functions on TARGET_HARD_FLOAT.

I think we're also missing Zfinx here: there's no F registers to save, 
but we should still have the same side effects visible in the CSRs.




@@ -5282,6 +5290,29 @@ riscv_for_each_saved_reg (poly_int64 sp_offset, 
riscv_save_restore_fn fn,
            }
        }

+      if (regno == RISCV_PROLOGUE_TEMP_REGNUM
+         && TARGET_HARD_FLOAT
+         && cfun->machine->interrupt_handler_p
+         && cfun->machine->frame.fmask)
+       {
+         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 (gen_rtx_REG (SImode, 
RISCV_PROLOGUE_TEMP_REGNUM)));
+             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 (gen_rtx_REG (SImode, 
RISCV_PROLOGUE_TEMP_REGNUM)));
+             riscv_save_restore_reg (word_mode, regno, offset, fn);
+             offset -= fcsr_size;
+           }
+         continue;
+       }
Note there is a macro RISCV_PROLOGUE_TEMP(MODE) which will create the
REG expression for the prologue temporary in the given mode.  That way
you don't have to call gen_rtx_REG directly here.

Jeff

This got snipped, but the tests should only check for the CSR 
save/restore on F/D systems (from looking at them they'd fail on soft 
float targets).