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

[Palmer Dabbelt <palmer@dabbelt.com>]

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