Message ID | 20231225084521.78251-1-kito.cheng@sifive.com |
---|---|
State | New |
Headers | show |
Series | RISC-V: Fix misaligned stack offset for interrupt function | expand |
On 12/25/23 01:45, Kito Cheng wrote: > `interrupt` function will backup fcsr register, but it fixed to SImode, > it's not big issue since fcsr only used 8 bits so far, however the > offset should still using UNITS_PER_WORD to prevent the stack offset > become non 8 byte aligned, it will cause problem for RV64. > > gcc/ChangeLog: > > * config/riscv/riscv.cc (riscv_for_each_saved_reg): Adjust the > offset of fcsr. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/interrupt-misaligned.c: New. OK jeff
On 2023-12-25 16:45 Kito Cheng <kito.cheng@sifive.com> wrote: >+++ b/gcc/testsuite/gcc.target/riscv/interrupt-misaligned.c >@@ -0,0 +1,29 @@ >+/* { dg-do compile } */ >+/* { dg-options "-O2 -march=rv64gc -mabi=lp64d -fno-schedule-insns -fno-schedule-insns2" } */ >+/* { dg-skip-if "" { *-*-* } { "-flto -fno-fat-lto-objects" } } */ >+ >+/* Make sure no stack offset are misaligned. >+** interrupt: >+** ... >+** sd\tt0,40\(sp\) >+** frcsr\tt0 >+** sw\tt0,32\(sp\) >+** sd\tt1,24\(sp\) >+** fsd\tft0,8\(sp\) >+** ... >+** lw\tt0,32\(sp\) >+** fscsr\tt0 >+** ld\tt0,40\(sp\) >+** ld\tt1,24\(sp\) >+** fld\tft0,8\(sp\) >+** ... >+*/ Hi Kito The fix is fine but maybe using s0 instead of t0 is better: 1. simpler codes. 2. less stack size current implementaion: >+** sd\tt0,40\(sp\) >+** frcsr\tt0 >+** sw\tt0,32\(sp\) //save content of frcsr in stack use s0: >+** sd\tt0,40\(sp\) >+** frcsr\ts0 //save content of frcsr in s0 instead of stack. If s0 is used as callee saved register, it will be saved again later by legacy codes . Also adding this change in riscv_expand_prologue & epilogue would be consistent with current stack allocation logic. I can try it if you think necessary. BR Fei >+ >+ >+void interrupt(void) __attribute__((interrupt)); >+void interrupt(void) >+{ >+ asm volatile ("# clobber!":::"t0", "t1", "ft0"); >+} >+ >+/* { dg-final { check-function-bodies "**" "" } } */ >-- >2.40.1
Hi Fei: > The fix is fine but maybe using s0 instead of t0 is better: > 1. simpler codes. > 2. less stack size > > current implementaion: > >+** sd\tt0,40\(sp\) > >+** frcsr\tt0 > >+** sw\tt0,32\(sp\) //save content of frcsr in stack > > use s0: > >+** sd\tt0,40\(sp\) > >+** frcsr\ts0 //save content of frcsr in s0 instead of stack. If s0 is used as callee saved register, it will be saved again later by legacy codes . > > Also adding this change in riscv_expand_prologue & epilogue would be consistent with current stack allocation logic. > > I can try it if you think necessary. Yeah, that can optimize further, but I guess the ideal strategy is check which register will save/restore at prologue/epilogue - and use that no matter if it's s* or t* register, anyway, I think it's interesting stuff to improve, so it would be appreciated if you can try :)
Committed to trunk, thanks!
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc index d867c0a03f0..c2b24d3db5a 100644 --- a/gcc/config/riscv/riscv.cc +++ b/gcc/config/riscv/riscv.cc @@ -6790,7 +6790,9 @@ riscv_for_each_saved_reg (poly_int64 sp_offset, riscv_save_restore_fn fn, || (TARGET_ZFINX && (cfun->machine->frame.mask & ~(1 << RISCV_PROLOGUE_TEMP_REGNUM))))) { - unsigned int fcsr_size = GET_MODE_SIZE (SImode); + /* Always assume FCSR occupy UNITS_PER_WORD to prevent stack + offset misaligned later. */ + unsigned int fcsr_size = UNITS_PER_WORD; if (!epilogue) { riscv_save_restore_reg (word_mode, regno, offset, fn); diff --git a/gcc/testsuite/gcc.target/riscv/interrupt-misaligned.c b/gcc/testsuite/gcc.target/riscv/interrupt-misaligned.c new file mode 100644 index 00000000000..b5f8e6c2bbe --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/interrupt-misaligned.c @@ -0,0 +1,29 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -march=rv64gc -mabi=lp64d -fno-schedule-insns -fno-schedule-insns2" } */ +/* { dg-skip-if "" { *-*-* } { "-flto -fno-fat-lto-objects" } } */ + +/* Make sure no stack offset are misaligned. +** interrupt: +** ... +** sd\tt0,40\(sp\) +** frcsr\tt0 +** sw\tt0,32\(sp\) +** sd\tt1,24\(sp\) +** fsd\tft0,8\(sp\) +** ... +** lw\tt0,32\(sp\) +** fscsr\tt0 +** ld\tt0,40\(sp\) +** ld\tt1,24\(sp\) +** fld\tft0,8\(sp\) +** ... +*/ + + +void interrupt(void) __attribute__((interrupt)); +void interrupt(void) +{ + asm volatile ("# clobber!":::"t0", "t1", "ft0"); +} + +/* { dg-final { check-function-bodies "**" "" } } */