diff mbox series

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

Message ID 20230613064126.1323-1-jinma@linux.alibaba.com
State Superseded
Delegated to: Jeff Law
Headers
Series RISC-V: Save and restore FCSR in interrupt functions to avoid program errors. |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Testing passed

Commit Message

Jin Ma June 13, 2023, 6:41 a.m. UTC 
  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

Comments

Jeff Law June 13, 2023, 5:41 p.m. UTC | #1 
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
Palmer Dabbelt June 13, 2023, 6:51 p.m. UTC | #2 
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).
diff mbox series

Patch

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 00000000000..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 00000000000..189d10654e9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/interrupt-fcsr-2.c
@@ -0,0 +1,14 @@ 
+/* Verify that fcsr instructions emitted.  */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+extern int foo (void);
+extern float interrupt_count;
+void __attribute__ ((interrupt))
+sub (void)
+{
+  interrupt_count++;
+}
+
+/* { dg-final { scan-assembler-times "frcsr\t" 1 } } */
+/* { dg-final { scan-assembler-times "fscsr\t" 1 } } */
diff --git a/gcc/testsuite/gcc.target/riscv/interrupt-fcsr-3.c b/gcc/testsuite/gcc.target/riscv/interrupt-fcsr-3.c
new file mode 100644
index 00000000000..28d021c7ae8
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/interrupt-fcsr-3.c
@@ -0,0 +1,13 @@ 
+/* Verify that fcsr instructions are not emitted.  */
+/* { dg-do compile } */
+/* { dg-options "" } */
+
+extern int foo (void);
+
+void __attribute__ ((interrupt))
+sub (void)
+{
+}
+
+/* { dg-final { scan-assembler-not "frcsr\t" } } */
+/* { dg-final { scan-assembler-not "fscsr\t" } } */