| Message ID | 20230429105959.23211-1-gaofei@eswincomputing.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2] RISC-V: decouple stack allocation for rv32e w/o save-restore. | expand |
On 4/29/23 04:59, Fei Gao wrote: > Currently in rv32e, stack allocation for GPR callee-saved registers is > always 12 bytes w/o save-restore. Actually, for the case without save-restore, > less stack memory can be reserved. This patch decouples stack allocation for > rv32e w/o save-restore and makes riscv_compute_frame_info more readable. > > output of testcase rv32e_stack.c > before patch: > addi sp,sp,-16 > sw ra,12(sp) > call getInt > sw a0,0(sp) > lw a0,0(sp) > call PrintInts > lw a5,0(sp) > mv a0,a5 > lw ra,12(sp) > addi sp,sp,16 > jr ra > > after patch: > addi sp,sp,-8 > sw ra,4(sp) > call getInt > sw a0,0(sp) > lw a0,0(sp) > call PrintInts > lw a5,0(sp) > mv a0,a5 > lw ra,4(sp) > addi sp,sp,8 > jr ra > > > gcc/ChangeLog: > > * config/riscv/riscv.cc (riscv_avoid_save_libcall): helper function for riscv_use_save_libcall. > (riscv_use_save_libcall): call riscv_avoid_save_libcall. > (riscv_compute_frame_info): restructure to decouple stack allocation for rv32e w/o save-restore. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/rv32e_stack.c: New test. Thanks. I rewrapped the ChangeLog and pushed this to the trunk. jeff
On Sat, 29 Apr 2023 08:38:06 PDT (-0700), jeffreyalaw@gmail.com wrote: > > > On 4/29/23 04:59, Fei Gao wrote: >> Currently in rv32e, stack allocation for GPR callee-saved registers is >> always 12 bytes w/o save-restore. Actually, for the case without save-restore, >> less stack memory can be reserved. This patch decouples stack allocation for >> rv32e w/o save-restore and makes riscv_compute_frame_info more readable. Are you guys using rv32e? It's not widely tested, at least by most upstream folks. If you're actively trying to ship it then we should probably add it to the various lists of targest that get tested, as I'd bet there's a lot of oddness floating around. >> output of testcase rv32e_stack.c >> before patch: >> addi sp,sp,-16 >> sw ra,12(sp) >> call getInt >> sw a0,0(sp) >> lw a0,0(sp) >> call PrintInts >> lw a5,0(sp) >> mv a0,a5 >> lw ra,12(sp) >> addi sp,sp,16 >> jr ra >> >> after patch: >> addi sp,sp,-8 >> sw ra,4(sp) >> call getInt >> sw a0,0(sp) >> lw a0,0(sp) >> call PrintInts >> lw a5,0(sp) >> mv a0,a5 >> lw ra,4(sp) >> addi sp,sp,8 >> jr ra >> >> >> gcc/ChangeLog: >> >> * config/riscv/riscv.cc (riscv_avoid_save_libcall): helper function for riscv_use_save_libcall. >> (riscv_use_save_libcall): call riscv_avoid_save_libcall. >> (riscv_compute_frame_info): restructure to decouple stack allocation for rv32e w/o save-restore. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.target/riscv/rv32e_stack.c: New test. > Thanks. I rewrapped the ChangeLog and pushed this to the trunk. Works for me, thanks for reviewing all this stuff -- we're all pretty buried ;) > > jeff
On 4/29/23 11:00, Palmer Dabbelt wrote: > On Sat, 29 Apr 2023 08:38:06 PDT (-0700), jeffreyalaw@gmail.com wrote: >> >> >> On 4/29/23 04:59, Fei Gao wrote: >>> Currently in rv32e, stack allocation for GPR callee-saved registers is >>> always 12 bytes w/o save-restore. Actually, for the case without >>> save-restore, >>> less stack memory can be reserved. This patch decouples stack >>> allocation for >>> rv32e w/o save-restore and makes riscv_compute_frame_info more readable. > > Are you guys using rv32e? It's not widely tested, at least by most > upstream folks. If you're actively trying to ship it then we should > probably add it to the various lists of targest that get tested, as I'd > bet there's a lot of oddness floating around. No interest at all in rv32 at Ventana. >> Thanks. I rewrapped the ChangeLog and pushed this to the trunk. > > Works for me, thanks for reviewing all this stuff -- we're all pretty > buried ;) Just standard procedure with the trunk re-opened. In the past I would have ignored anything in the risc-v space. I've traded that for ignoring x86 :-) Jeff
On Sat, 29 Apr 2023 10:44:08 PDT (-0700), jeffreyalaw@gmail.com wrote: > > > On 4/29/23 11:00, Palmer Dabbelt wrote: >> On Sat, 29 Apr 2023 08:38:06 PDT (-0700), jeffreyalaw@gmail.com wrote: >>> >>> >>> On 4/29/23 04:59, Fei Gao wrote: >>>> Currently in rv32e, stack allocation for GPR callee-saved registers is >>>> always 12 bytes w/o save-restore. Actually, for the case without >>>> save-restore, >>>> less stack memory can be reserved. This patch decouples stack >>>> allocation for >>>> rv32e w/o save-restore and makes riscv_compute_frame_info more readable. >> >> Are you guys using rv32e? It's not widely tested, at least by most >> upstream folks. If you're actively trying to ship it then we should >> probably add it to the various lists of targest that get tested, as I'd >> bet there's a lot of oddness floating around. > No interest at all in rv32 at Ventana. Makes sense, I was mostly wondering abotu the Eswin folks though. > > >>> Thanks. I rewrapped the ChangeLog and pushed this to the trunk. >> >> Works for me, thanks for reviewing all this stuff -- we're all pretty >> buried ;) > Just standard procedure with the trunk re-opened. In the past I would > have ignored anything in the risc-v space. I've traded that for > ignoring x86 :-) > > > > Jeff
SiFive has tests and delivers RV32E. On Sun, Apr 30, 2023 at 1:45 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Sat, 29 Apr 2023 10:44:08 PDT (-0700), jeffreyalaw@gmail.com wrote: > > > > > > On 4/29/23 11:00, Palmer Dabbelt wrote: > >> On Sat, 29 Apr 2023 08:38:06 PDT (-0700), jeffreyalaw@gmail.com wrote: > >>> > >>> > >>> On 4/29/23 04:59, Fei Gao wrote: > >>>> Currently in rv32e, stack allocation for GPR callee-saved registers is > >>>> always 12 bytes w/o save-restore. Actually, for the case without > >>>> save-restore, > >>>> less stack memory can be reserved. This patch decouples stack > >>>> allocation for > >>>> rv32e w/o save-restore and makes riscv_compute_frame_info more readable. > >> > >> Are you guys using rv32e? It's not widely tested, at least by most > >> upstream folks. If you're actively trying to ship it then we should > >> probably add it to the various lists of targest that get tested, as I'd > >> bet there's a lot of oddness floating around. > > No interest at all in rv32 at Ventana. > > Makes sense, I was mostly wondering abotu the Eswin folks though. > > > > > > >>> Thanks. I rewrapped the ChangeLog and pushed this to the trunk. > >> > >> Works for me, thanks for reviewing all this stuff -- we're all pretty > >> buried ;) > > Just standard procedure with the trunk re-opened. In the past I would > > have ignored anything in the risc-v space. I've traded that for > > ignoring x86 :-) > > > > > > > > Jeff
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc index 5d2550871c7..8b32977e296 100644 --- a/gcc/config/riscv/riscv.cc +++ b/gcc/config/riscv/riscv.cc @@ -4772,12 +4772,27 @@ riscv_save_reg_p (unsigned int regno) return false; } +/* Return TRUE if a libcall to save/restore GPRs should be + avoided. FALSE otherwise. */ +static bool +riscv_avoid_save_libcall (void) +{ + if (!TARGET_SAVE_RESTORE + || crtl->calls_eh_return + || frame_pointer_needed + || cfun->machine->interrupt_handler_p + || cfun->machine->varargs_size != 0 + || crtl->args.pretend_args_size != 0) + return true; + + return false; +} + /* Determine whether to call GPR save/restore routines. */ static bool riscv_use_save_libcall (const struct riscv_frame_info *frame) { - if (!TARGET_SAVE_RESTORE || crtl->calls_eh_return || frame_pointer_needed - || cfun->machine->interrupt_handler_p) + if (riscv_avoid_save_libcall ()) return false; return frame->save_libcall_adjustment != 0; @@ -4857,7 +4872,7 @@ riscv_compute_frame_info (void) struct riscv_frame_info *frame; poly_int64 offset; bool interrupt_save_prologue_temp = false; - unsigned int regno, i, num_x_saved = 0, num_f_saved = 0; + unsigned int regno, i, num_x_saved = 0, num_f_saved = 0, x_save_size = 0; frame = &cfun->machine->frame; @@ -4895,24 +4910,14 @@ riscv_compute_frame_info (void) frame->fmask |= 1 << (regno - FP_REG_FIRST), num_f_saved++; } - /* At the bottom of the frame are any outgoing stack arguments. */ - offset = riscv_stack_align (crtl->outgoing_args_size); - /* Next are local stack variables. */ - offset += riscv_stack_align (get_frame_size ()); - /* The virtual frame pointer points above the local variables. */ - frame->frame_pointer_offset = offset; - /* Next are the callee-saved FPRs. */ - if (frame->fmask) - offset += riscv_stack_align (num_f_saved * UNITS_PER_FP_REG); - frame->fp_sp_offset = offset - UNITS_PER_FP_REG; - /* Next are the callee-saved GPRs. */ if (frame->mask) { - unsigned x_save_size = riscv_stack_align (num_x_saved * UNITS_PER_WORD); + x_save_size = riscv_stack_align (num_x_saved * UNITS_PER_WORD); unsigned num_save_restore = 1 + riscv_save_libcall_count (frame->mask); /* Only use save/restore routines if they don't alter the stack size. */ - if (riscv_stack_align (num_save_restore * UNITS_PER_WORD) == x_save_size) + if (riscv_stack_align (num_save_restore * UNITS_PER_WORD) == x_save_size + && !riscv_avoid_save_libcall ()) { /* Libcall saves/restores 3 registers at once, so we need to allocate 12 bytes for callee-saved register. */ @@ -4921,9 +4926,21 @@ riscv_compute_frame_info (void) frame->save_libcall_adjustment = x_save_size; } - - offset += x_save_size; } + + /* At the bottom of the frame are any outgoing stack arguments. */ + offset = riscv_stack_align (crtl->outgoing_args_size); + /* Next are local stack variables. */ + offset += riscv_stack_align (get_frame_size ()); + /* The virtual frame pointer points above the local variables. */ + frame->frame_pointer_offset = offset; + /* Next are the callee-saved FPRs. */ + if (frame->fmask) + offset += riscv_stack_align (num_f_saved * UNITS_PER_FP_REG); + frame->fp_sp_offset = offset - UNITS_PER_FP_REG; + /* Next are the callee-saved GPRs. */ + if (frame->mask) + offset += x_save_size; frame->gp_sp_offset = offset - UNITS_PER_WORD; /* The hard frame pointer points above the callee-saved GPRs. */ frame->hard_frame_pointer_offset = offset; @@ -4935,11 +4952,8 @@ riscv_compute_frame_info (void) padding. */ frame->arg_pointer_offset = offset - crtl->args.pretend_args_size; frame->total_size = offset; - /* Next points the incoming stack pointer and any incoming arguments. */ - /* Only use save/restore routines when the GPRs are atop the frame. */ - if (known_ne (frame->hard_frame_pointer_offset, frame->total_size)) - frame->save_libcall_adjustment = 0; + /* Next points the incoming stack pointer and any incoming arguments. */ } /* Make sure that we're not trying to eliminate to the wrong hard frame diff --git a/gcc/testsuite/gcc.target/riscv/rv32e_stack.c b/gcc/testsuite/gcc.target/riscv/rv32e_stack.c new file mode 100644 index 00000000000..cec90ede4b5 --- /dev/null +++ b/gcc/testsuite/gcc.target/riscv/rv32e_stack.c @@ -0,0 +1,14 @@ +/* { dg-do compile} */ +/* { dg-options "-O0 -march=rv32e -mabi=ilp32e -fomit-frame-pointer" } */ + +int getInt(); +void PrintInts(int); + +int callPrintInts() +{ + int i = getInt(); + PrintInts(i); + return i; +} + +/* { dg-final { scan-assembler-not "addi sp,sp,-16" } } */
Currently in rv32e, stack allocation for GPR callee-saved registers is always 12 bytes w/o save-restore. Actually, for the case without save-restore, less stack memory can be reserved. This patch decouples stack allocation for rv32e w/o save-restore and makes riscv_compute_frame_info more readable. output of testcase rv32e_stack.c before patch: addi sp,sp,-16 sw ra,12(sp) call getInt sw a0,0(sp) lw a0,0(sp) call PrintInts lw a5,0(sp) mv a0,a5 lw ra,12(sp) addi sp,sp,16 jr ra after patch: addi sp,sp,-8 sw ra,4(sp) call getInt sw a0,0(sp) lw a0,0(sp) call PrintInts lw a5,0(sp) mv a0,a5 lw ra,4(sp) addi sp,sp,8 jr ra gcc/ChangeLog: * config/riscv/riscv.cc (riscv_avoid_save_libcall): helper function for riscv_use_save_libcall. (riscv_use_save_libcall): call riscv_avoid_save_libcall. (riscv_compute_frame_info): restructure to decouple stack allocation for rv32e w/o save-restore. gcc/testsuite/ChangeLog: * gcc.target/riscv/rv32e_stack.c: New test. --- gcc/config/riscv/riscv.cc | 58 ++++++++++++-------- gcc/testsuite/gcc.target/riscv/rv32e_stack.c | 14 +++++ 2 files changed, 50 insertions(+), 22 deletions(-) create mode 100644 gcc/testsuite/gcc.target/riscv/rv32e_stack.c