Re: [PATCH] RISC-V: Save and restore FCSR in interrupt functions to avoid program errors.
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).
Re: [PATCH] RISC-V: Save and restore FCSR in interrupt functions to avoid program errors.
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.
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.
@@ -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
[PATCH] RISC-V: Save and restore FCSR in interrupt functions to avoid program errors.
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.
---
gcc/config/riscv/riscv.cc | 33 ++-
gcc/config/riscv/riscv.md | 13
.../gcc.target/riscv/interrupt-fcsr-1.c | 14
.../gcc.target/riscv/interrupt-fcsr-2.c | 14
.../gcc.target/riscv/interrupt-fcsr-3.c | 13
5 files changed, 86 insertions(+), 1 deletion(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/interrupt-fcsr-1.c
create mode 100644 gcc/testsuite/gcc.target/riscv/interrupt-fcsr-2.c
create mode 100644 gcc/testsuite/gcc.target/riscv/interrupt-fcsr-3.c
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;
}
@@ -5035,6 +5036,13 @@ riscv_compute_frame_info (void)
frame->save_libcall_adjustment = x_save_size;
}
+
+ if (TARGET_HARD_FLOAT
+ && cfun->machine->interrupt_handler_p
+ && frame->fmask)
+ /* In an interrupt function, we need extra space
+ for the initial saves of FCSR. */
+ x_save_size += riscv_stack_align (1 * UNITS_PER_WORD);
}
/* At the bottom of the frame are any outgoing stack arguments. */
@@ -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;
+ }
+
riscv_save_restore_reg (word_mode, regno, offset, fn);
}
diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index d8e935cb934..565e8cd27cd 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -78,6 +78,8 @@ (define_c_enum "unspecv" [
UNSPECV_GPR_RESTORE
;; Floating-point unspecs.
+ UNSPECV_FRCSR
+ UNSPECV_FSCSR
UNSPECV_FRFLAGS
UNSPECV_FSFLAGS
UNSPECV_FSNVSNAN
@@ -3056,6 +3058,17 @@ (define_insn "gpr_restore_return"
""
"")
+(define_insn "riscv_frcsr"
+ [(set (match_operand:SI 0 "register_operand" "=r")
+ (unspec_volatile [(const_int 0)] UNSPECV_FRCSR))]
+ "TARGET_HARD_FLOAT || TARGET_ZFINX"
+ "frcsr\t%0")
+
+(define_insn "riscv_fscsr"
+ [(unspec_volatile [(match_operand:SI 0 "csr_operand" "rK")] UNSPECV_FSCSR)]
+ "TARGET_HARD_FLOAT || TARGET_ZFINX"
+ "fscsr\t%0")
+
(define_insn "riscv_frflags"
[(set (match_operand:SI 0 "register_operand" "=r")
(unspec_volatile [(const_int 0)] UNSPECV_FRFLAGS))]
diff --git a/gcc/testsuite/gcc.target/riscv/interrupt-fcsr-1.c
b/gcc/testsuite/gcc.target/riscv/interrupt-fcsr-1.c
new file mode 100644
index 000..fe49007c936
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/interrupt-fcsr-1.c
@@ -0,0 +1,14 @@
+/* Verify that fcsr instructions emitted. */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+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 } } */
\ No newline at end of file
diff --git a/gcc/testsuite/gcc.target/riscv/interrupt-fcsr-2.c
b/gcc/testsuite/gcc.target/riscv/interrupt-fcsr-2.c
new file mode 100644
index 000..189d10654e9
--- /dev/null
+++
