| Message ID | 20240708083915.29943-1-gaofei@eswincomputing.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RISC-V] fix zcmp popretz [PR113715] | expand |
On 7/8/24 2:39 AM, Fei Gao wrote: > Root cause: > https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=b27d323a368033f0b37e93c57a57a35fd9997864 > Commit above tries in targetm.gen_epilogue () to detect if > there's li a0,0 insn at the end of insn chain, if so, cm.popret > is replaced by cm.popretz and li a0,0 insn is deleted. > > Insertion of the generated epilogue sequence > into the insn chain doesn't happen at this moment. > If later shrink-wrap decides NOT to insert the epilogue sequence at the end > of insn chain, then the li a0,0 insn has already been mistakeny removed. > > Fix this issue by removing generation of cm.popretz in epilogue, > leaving the assignment to a0 and use insn with cm.popret. > > That's likely going to result in some kind of code size regression, > but not a correctness regression. > > Optimization can be done in future. > > Signed-off-by: Fei Gao <gaofei@eswincomputing.com> > gcc/ChangeLog: > > * config/riscv/riscv.cc (riscv_zcmp_can_use_popretz): Removed. > (riscv_gen_multi_pop_insn): Remove generation of cm.popretz. > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/rv32e_zcmp.c: Adapt TC. > * gcc.target/riscv/rv32i_zcmp.c: Likewise. OK. Has the ESWIN team ever considered doing a bootstrap test of the zcmp* extensions? ie, turn them all on by default, then build GCC natively on an rv32 system (or qemu emulated rv32 system)? I've found that kind of test amazingly helpful through the years to thoroughly exercise certain changes. In fact, Raphael and I just did that kind of test on rv32 for stack-clash protection to verify some degree of correctness on rv32 (the vast majority of our efforts are focused on rv64). THe only issue we ran into was the linker running out of address space on the stage3 link, but if you turn off debug symbols it should bootstrap in rv32. Jeff
On 2024-07-09 01:49 Jeff Law <jeffreyalaw@gmail.com> wrote: >Has the ESWIN team ever considered doing a bootstrap test of the zcmp* >extensions? ie, turn them all on by default, then build GCC natively on >an rv32 system (or qemu emulated rv32 system)? > >I've found that kind of test amazingly helpful through the years to >thoroughly exercise certain changes. In fact, Raphael and I just did >that kind of test on rv32 for stack-clash protection to verify some >degree of correctness on rv32 (the vast majority of our efforts are >focused on rv64). THe only issue we ran into was the linker running out >of address space on the stage3 link, but if you turn off debug symbols >it should bootstrap in rv32. It's really a good idea to test the new feature. We haven't done that but will try it. What I'm doing is to add newlib board with zcmp enabled and watch if there's compile or run failures in regression test. Oberviously, bootstrap is more thorough and greater coverage in test. Thanks for your advice. BR, Fei > >Jeff
On 7/8/24 7:33 PM, Fei Gao wrote: > On 2024-07-09 01:49 Jeff Law <jeffreyalaw@gmail.com> wrote: > >> Has the ESWIN team ever considered doing a bootstrap test of the zcmp* >> extensions? ie, turn them all on by default, then build GCC natively on >> an rv32 system (or qemu emulated rv32 system)? >> >> I've found that kind of test amazingly helpful through the years to >> thoroughly exercise certain changes. In fact, Raphael and I just did >> that kind of test on rv32 for stack-clash protection to verify some >> degree of correctness on rv32 (the vast majority of our efforts are >> focused on rv64). THe only issue we ran into was the linker running out >> of address space on the stage3 link, but if you turn off debug symbols >> it should bootstrap in rv32. > > It's really a good idea to test the new feature. We haven't done that but will try it. If you need a RFS, let me know. I've got one as a docker container that you can run on an x86 host if you've got binfmt handlers installed. That's faster than full system emulation. It's even possible that my docker registry is publicly accessable. If you've got docker installed somewhere, try docker pull law-sandy.freeddns.org:49153/riscv32-builder You'll know you have the binfmt handlers setup when you can do something like this: > jlaw@x11-dpi:~/test/gcc$ docker run -it law-sandy.freeddns.org:49153/riscv32-builder /bin/bash > root@4708cec84476:/# I'm now in a riscv32 docker container and it mostly looks native. I had to take a few shortcuts to get this working, so if you look real closely you'll find that bash and vim are actually x86 binaries. But other than that, it's riscv32 and you should be able to bootstrap in it. Also note you can take advantage of as many cores are your x86 host has when building. > What I'm doing is to add newlib board with zcmp enabled and watch if there's > compile or run failures in regression test. What I'd suggest for this testing (and it has value as it's much faster) is to setup the binfmt handlers. > > Oberviously, bootstrap is more thorough and greater coverage in test. > Thanks for your advice. Happy to help. jeff
On 2024-07-09 01:49 Jeff Law <jeffreyalaw@gmail.com> wrote: > > > >On 7/8/24 2:39 AM, Fei Gao wrote: >> Root cause: >> https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=b27d323a368033f0b37e93c57a57a35fd9997864 >> Commit above tries in targetm.gen_epilogue () to detect if >> there's li a0,0 insn at the end of insn chain, if so, cm.popret >> is replaced by cm.popretz and li a0,0 insn is deleted. >> >> Insertion of the generated epilogue sequence >> into the insn chain doesn't happen at this moment. >> If later shrink-wrap decides NOT to insert the epilogue sequence at the end >> of insn chain, then the li a0,0 insn has already been mistakeny removed. >> >> Fix this issue by removing generation of cm.popretz in epilogue, >> leaving the assignment to a0 and use insn with cm.popret. >> >> That's likely going to result in some kind of code size regression, >> but not a correctness regression. >> >> Optimization can be done in future. >> >> Signed-off-by: Fei Gao <gaofei@eswincomputing.com> >> gcc/ChangeLog: >> >> * config/riscv/riscv.cc (riscv_zcmp_can_use_popretz): Removed. >> (riscv_gen_multi_pop_insn): Remove generation of cm.popretz. >> >> gcc/testsuite/ChangeLog: >> >> * gcc.target/riscv/rv32e_zcmp.c: Adapt TC. >> * gcc.target/riscv/rv32i_zcmp.c: Likewise. >OK. > Shall I back port this fix to releases/gcc-14? BR Fei > >Jeff
On 7/8/24 11:30 PM, Fei Gao wrote: > On 2024-07-09 01:49 Jeff Law <jeffreyalaw@gmail.com> wrote: >> >> >> >> On 7/8/24 2:39 AM, Fei Gao wrote: >>> Root cause: >>> https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=b27d323a368033f0b37e93c57a57a35fd9997864 >>> Commit above tries in targetm.gen_epilogue () to detect if >>> there's li a0,0 insn at the end of insn chain, if so, cm.popret >>> is replaced by cm.popretz and li a0,0 insn is deleted. >>> >>> Insertion of the generated epilogue sequence >>> into the insn chain doesn't happen at this moment. >>> If later shrink-wrap decides NOT to insert the epilogue sequence at the end >>> of insn chain, then the li a0,0 insn has already been mistakeny removed. >>> >>> Fix this issue by removing generation of cm.popretz in epilogue, >>> leaving the assignment to a0 and use insn with cm.popret. >>> >>> That's likely going to result in some kind of code size regression, >>> but not a correctness regression. >>> >>> Optimization can be done in future. >>> >>> Signed-off-by: Fei Gao <gaofei@eswincomputing.com> >>> gcc/ChangeLog: >>> >>> * config/riscv/riscv.cc (riscv_zcmp_can_use_popretz): Removed. >>> (riscv_gen_multi_pop_insn): Remove generation of cm.popretz. >>> >>> gcc/testsuite/ChangeLog: >>> >>> * gcc.target/riscv/rv32e_zcmp.c: Adapt TC. >>> * gcc.target/riscv/rv32i_zcmp.c: Likewise. >> OK. >> > > Shall I back port this fix to releases/gcc-14? Yea, that's probably wise given its a correctness issue. Thanks! jeff
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc index 38ed773c222..61fa74e9322 100644 --- a/gcc/config/riscv/riscv.cc +++ b/gcc/config/riscv/riscv.cc @@ -8167,52 +8167,6 @@ riscv_adjust_libcall_cfi_epilogue () return dwarf; } -/* return true if popretz pattern can be matched. - set (reg 10 a0) (const_int 0) - use (reg 10 a0) - NOTE_INSN_EPILOGUE_BEG */ -static rtx_insn * -riscv_zcmp_can_use_popretz (void) -{ - rtx_insn *insn = NULL, *use = NULL, *clear = NULL; - - /* sequence stack for NOTE_INSN_EPILOGUE_BEG*/ - struct sequence_stack *outer_seq = get_current_sequence ()->next; - if (!outer_seq) - return NULL; - insn = outer_seq->first; - if (!insn || !NOTE_P (insn) || NOTE_KIND (insn) != NOTE_INSN_EPILOGUE_BEG) - return NULL; - - /* sequence stack for the insn before NOTE_INSN_EPILOGUE_BEG*/ - outer_seq = outer_seq->next; - if (outer_seq) - insn = outer_seq->last; - - /* skip notes */ - while (insn && NOTE_P (insn)) - { - insn = PREV_INSN (insn); - } - use = insn; - - /* match use (reg 10 a0) */ - if (use == NULL || !INSN_P (use) || GET_CODE (PATTERN (use)) != USE - || !REG_P (XEXP (PATTERN (use), 0)) - || REGNO (XEXP (PATTERN (use), 0)) != A0_REGNUM) - return NULL; - - /* match set (reg 10 a0) (const_int 0 [0]) */ - clear = PREV_INSN (use); - if (clear != NULL && INSN_P (clear) && GET_CODE (PATTERN (clear)) == SET - && REG_P (SET_DEST (PATTERN (clear))) - && REGNO (SET_DEST (PATTERN (clear))) == A0_REGNUM - && SET_SRC (PATTERN (clear)) == const0_rtx) - return clear; - - return NULL; -} - static void riscv_gen_multi_pop_insn (bool use_multi_pop_normal, unsigned mask, unsigned multipop_size) @@ -8223,13 +8177,6 @@ riscv_gen_multi_pop_insn (bool use_multi_pop_normal, unsigned mask, if (!use_multi_pop_normal) insn = emit_insn ( riscv_gen_multi_push_pop_insn (POP_IDX, multipop_size, regs_count)); - else if (rtx_insn *clear_a0_insn = riscv_zcmp_can_use_popretz ()) - { - delete_insn (NEXT_INSN (clear_a0_insn)); - delete_insn (clear_a0_insn); - insn = emit_jump_insn ( - riscv_gen_multi_push_pop_insn (POPRETZ_IDX, multipop_size, regs_count)); - } else insn = emit_jump_insn ( riscv_gen_multi_push_pop_insn (POPRET_IDX, multipop_size, regs_count)); diff --git a/gcc/testsuite/gcc.target/riscv/rv32e_zcmp.c b/gcc/testsuite/gcc.target/riscv/rv32e_zcmp.c index 50e443573ad..0af4d7199f6 100644 --- a/gcc/testsuite/gcc.target/riscv/rv32e_zcmp.c +++ b/gcc/testsuite/gcc.target/riscv/rv32e_zcmp.c @@ -259,7 +259,8 @@ foo (void) **test_popretz: ** cm.push {ra}, -16 ** call f1 -** cm.popretz {ra}, 16 +** li a0,0 +** cm.popret {ra}, 16 */ long test_popretz () diff --git a/gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c b/gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c index 1e1a8be8705..723889f49df 100644 --- a/gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c +++ b/gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c @@ -259,7 +259,8 @@ foo (void) **test_popretz: ** cm.push {ra}, -16 ** call f1 -** cm.popretz {ra}, 16 +** li a0,0 +** cm.popret {ra}, 16 */ long test_popretz ()
Root cause: https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=b27d323a368033f0b37e93c57a57a35fd9997864 Commit above tries in targetm.gen_epilogue () to detect if there's li a0,0 insn at the end of insn chain, if so, cm.popret is replaced by cm.popretz and li a0,0 insn is deleted. Insertion of the generated epilogue sequence into the insn chain doesn't happen at this moment. If later shrink-wrap decides NOT to insert the epilogue sequence at the end of insn chain, then the li a0,0 insn has already been mistakeny removed. Fix this issue by removing generation of cm.popretz in epilogue, leaving the assignment to a0 and use insn with cm.popret. That's likely going to result in some kind of code size regression, but not a correctness regression. Optimization can be done in future. Signed-off-by: Fei Gao <gaofei@eswincomputing.com> gcc/ChangeLog: * config/riscv/riscv.cc (riscv_zcmp_can_use_popretz): Removed. (riscv_gen_multi_pop_insn): Remove generation of cm.popretz. gcc/testsuite/ChangeLog: * gcc.target/riscv/rv32e_zcmp.c: Adapt TC. * gcc.target/riscv/rv32i_zcmp.c: Likewise. --- gcc/config/riscv/riscv.cc | 53 --------------------- gcc/testsuite/gcc.target/riscv/rv32e_zcmp.c | 3 +- gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c | 3 +- 3 files changed, 4 insertions(+), 55 deletions(-)