* [PATCH 0/2] fix RISC-V zcmp popretz [PR113715] @ 2024-06-05 1:50 Fei Gao 2024-06-05 1:50 ` [PATCH 1/2] target hooks: allow post processing after epilogue inserted Fei Gao ` (2 more replies) 0 siblings, 3 replies; 10+ messages in thread From: Fei Gao @ 2024-06-05 1:50 UTC (permalink / raw) To: gcc-patches; +Cc: kito.cheng, palmer, jeffreyalaw, gaofei The 1st patch adds a hook to allow post processing after epilogue inserted. The 2nd one implement the RISC-V hook to solve PR113715. Fei Gao (2): target hooks: allow post processing after epilogue inserted. [RISC-V]: fix zcmp popretz [PR113715]. gcc/config/riscv/riscv.cc | 191 ++++++++++++++------ gcc/doc/tm.texi | 5 + gcc/doc/tm.texi.in | 2 + gcc/function.cc | 2 + gcc/hooks.cc | 7 + gcc/hooks.h | 1 + gcc/target.def | 8 + gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c | 56 ++++++ 8 files changed, 219 insertions(+), 53 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] target hooks: allow post processing after epilogue inserted. 2024-06-05 1:50 [PATCH 0/2] fix RISC-V zcmp popretz [PR113715] Fei Gao @ 2024-06-05 1:50 ` Fei Gao 2024-06-05 1:50 ` [PATCH 2/2] [RISC-V]: fix zcmp popretz [PR113715] Fei Gao 2024-06-05 6:36 ` [PATCH 0/2] fix RISC-V " Kito Cheng 2 siblings, 0 replies; 10+ messages in thread From: Fei Gao @ 2024-06-05 1:50 UTC (permalink / raw) To: gcc-patches; +Cc: kito.cheng, palmer, jeffreyalaw, gaofei Define TARGET_POST_EPILOGUE_PROC if you have additional processing after epilogue is inserted into a basic block. gcc/ChangeLog: * doc/tm.texi: Regenerate. * doc/tm.texi.in: Document TARGET_POST_EPILOGUE_PROC. * function.cc (thread_prologue_and_epilogue_insns): Allow targets to have additional processingafter epilogue is inserted into a basic block. * hooks.cc (hook_void_rtx_insn): Define default handler. * hooks.h (hook_void_rtx_insn): Declare. * target.def: New hook. Signed-off-by: Fei Gao <gaofei@eswincomputing.com> --- gcc/doc/tm.texi | 5 +++++ gcc/doc/tm.texi.in | 2 ++ gcc/function.cc | 2 ++ gcc/hooks.cc | 7 +++++++ gcc/hooks.h | 1 + gcc/target.def | 8 ++++++++ 6 files changed, 25 insertions(+) diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi index cd50078227d..666a08c0406 100644 --- a/gcc/doc/tm.texi +++ b/gcc/doc/tm.texi @@ -5299,6 +5299,11 @@ This hook should add additional registers that are computed by the prologue to the hard regset for shrink-wrapping optimization purposes. @end deftypefn +@deftypefn {Target Hook} void TARGET_POST_EPILOGUE_PROC (rtx_insn *@var{}) +Define this hook if you have additional processing after epilogue is +inserted into a basic block. +@end deftypefn + @deftypefn {Target Hook} bool TARGET_WARN_FUNC_RETURN (tree) True if a function's return statements should be checked for matching the function's return type. This includes checking for falling off the end diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in index 058bd56487a..218e5d9dc20 100644 --- a/gcc/doc/tm.texi.in +++ b/gcc/doc/tm.texi.in @@ -3754,6 +3754,8 @@ the function prologue. Normally, the profiling code comes after. @hook TARGET_SET_UP_BY_PROLOGUE +@hook TARGET_POST_EPILOGUE_PROC + @hook TARGET_WARN_FUNC_RETURN @node Shrink-wrapping separate components diff --git a/gcc/function.cc b/gcc/function.cc index 4edd4da1247..6f3027972ee 100644 --- a/gcc/function.cc +++ b/gcc/function.cc @@ -6258,6 +6258,8 @@ thread_prologue_and_epilogue_insns (void) } } + targetm.post_epilogue_proc (epilogue_seq); + /* Threading the prologue and epilogue changes the artificial refs in the entry and exit blocks, and may invalidate DF info for tail calls. */ if (optimize diff --git a/gcc/hooks.cc b/gcc/hooks.cc index 28769074222..40844dd3593 100644 --- a/gcc/hooks.cc +++ b/gcc/hooks.cc @@ -501,6 +501,13 @@ hook_bool_rtx_insn_int_false (rtx_insn *, int) return false; } +/* Generic hook that takes a rtx_insn * and returns void. */ + +void +hook_void_rtx_insn (rtx_insn *) +{ +} + /* Generic hook that takes a rtx_insn * and an int and returns void. */ void diff --git a/gcc/hooks.h b/gcc/hooks.h index 924748420e6..4b2f47c61c1 100644 --- a/gcc/hooks.h +++ b/gcc/hooks.h @@ -60,6 +60,7 @@ extern bool hook_bool_const_tree_hwi_hwi_const_tree_true (const_tree, extern bool hook_bool_rtx_insn_true (rtx_insn *); extern bool hook_bool_rtx_false (rtx); extern bool hook_bool_rtx_insn_int_false (rtx_insn *, int); +extern void hook_void_rtx_insn (rtx_insn *); extern bool hook_bool_uintp_uintp_false (unsigned int *, unsigned int *); extern bool hook_bool_reg_class_t_false (reg_class_t regclass); extern bool hook_bool_mode_mode_reg_class_t_true (machine_mode, machine_mode, diff --git a/gcc/target.def b/gcc/target.def index c27df8095be..cd23f569e0b 100644 --- a/gcc/target.def +++ b/gcc/target.def @@ -6840,6 +6840,14 @@ to the hard regset for shrink-wrapping optimization purposes.", void, (struct hard_reg_set_container *), NULL) +/* Post epilogue processing. */ +DEFHOOK +(post_epilogue_proc, + "Define this hook if you have additional processing after epilogue is\n\ +inserted into a basic block.", + void, (rtx_insn *), + hook_void_rtx_insn) + /* For targets that have attributes that can affect whether a function's return statements need checking. For instance a 'naked' function attribute. */ -- 2.17.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] [RISC-V]: fix zcmp popretz [PR113715]. 2024-06-05 1:50 [PATCH 0/2] fix RISC-V zcmp popretz [PR113715] Fei Gao 2024-06-05 1:50 ` [PATCH 1/2] target hooks: allow post processing after epilogue inserted Fei Gao @ 2024-06-05 1:50 ` Fei Gao 2024-06-05 6:36 ` [PATCH 0/2] fix RISC-V " Kito Cheng 2 siblings, 0 replies; 10+ messages in thread From: Fei Gao @ 2024-06-05 1:50 UTC (permalink / raw) To: gcc-patches; +Cc: kito.cheng, palmer, jeffreyalaw, gaofei Before this patch, when generating epilogue with zcmp enabled, the compiler tries to check if return value is 0. If so, the cm.popret insn in epilogue sequence and the return value a0=0 insn before the epilogue sequence will be replaced with a cm.popretz insn. However, if shrink wrap is active, the epilogue may not be inserted at the end of function, causing return value a0=0 insn missing. This patch solves the issue by trying to generate cm.popretz insn after shrink wrap completes insertion of epilogue. TC: main function in gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c before patch: main: ... bltu a5,a4,.L6 .L7: cm.push {ra}, -16 call abort .L6: lui a5,%hi(.LC0) lhu a5,%lo(.LC0)(a5) sh a5,%lo(w)(a2) lhu a5,%lo(w)(a2) xori a5,a5,64 and a5,a5,a3 bltu a5,a4,.L7 ret after patch: main: ... bltu a5,a4,.L6 .L7: cm.push {ra}, -16 call abort .L6: lui a5,%hi(.LC0) lhu a5,%lo(.LC0)(a5) sh a5,%lo(w)(a2) lhu a5,%lo(w)(a2) xori a5,a5,64 and a5,a5,a3 bltu a5,a4,.L7 li a0,0 # missing before patch! ret Passed riscv regression tests on rv64gc. gcc/ChangeLog: * config/riscv/riscv.cc (riscv_zcmp_can_use_popretz): Modify to recognize popret with return value pattern. (riscv_gen_multi_pop_insn): Remove popretz generation. (gen_popretz_from_popret_insn): Generate popretz pattern based on popret insn. (riscv_popret_insn_p): Return true if INSN is a popret insn. (riscv_try_to_gen_popretz): Try to generate popretz insn if possible. (riscv_post_epilogue_proc): Implement TARGET_POST_EPILOGUE_PROC. (TARGET_POST_EPILOGUE_PROC): Define RISC-V hook. gcc/testsuite/ChangeLog: * gcc.target/riscv/rv32i_zcmp.c: New case. Signed-off-by: Fei Gao <gaofei@eswincomputing.com> --- gcc/config/riscv/riscv.cc | 191 ++++++++++++++------ gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c | 56 ++++++ 2 files changed, 194 insertions(+), 53 deletions(-) diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc index 10af38a5a81..975dd9d15d0 100644 --- a/gcc/config/riscv/riscv.cc +++ b/gcc/config/riscv/riscv.cc @@ -8151,52 +8151,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) @@ -8207,13 +8161,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)); @@ -8223,6 +8170,141 @@ riscv_gen_multi_pop_insn (bool use_multi_pop_normal, unsigned mask, REG_NOTES (insn) = dwarf; } +/* Generate popretz pattern based on POPRET insn. */ + +static rtx +gen_popretz_from_popret_insn (rtx_insn *popret) +{ + rtx pat = PATTERN (popret); + unsigned regs_count = XVECLEN (pat, 0) - 3; + rtx set_sp = XVECEXP (pat, 0, 0); + HOST_WIDE_INT multipop_size = INTVAL (XEXP (SET_SRC (set_sp), 1)); + return riscv_gen_multi_push_pop_insn (POPRETZ_IDX, multipop_size, regs_count); +} + +/* Return true if INSN is a popret insn. */ + +static bool +riscv_popret_insn_p (rtx_insn *insn) +{ + if (!INSN_P (insn)) + return false; + + recog_memoized (insn); + int insn_code = INSN_CODE (insn); + + return insn_code == CODE_FOR_gpr_multi_popret_up_to_ra_si + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s0_si + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s1_si + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s2_si + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s3_si + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s4_si + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s5_si + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s6_si + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s7_si + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s8_si + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s9_si + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s11_si + || insn_code == CODE_FOR_gpr_multi_popret_up_to_ra_di + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s0_di + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s1_di + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s2_di + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s3_di + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s4_di + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s5_di + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s6_di + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s7_di + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s8_di + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s9_di + || insn_code == CODE_FOR_gpr_multi_popret_up_to_s11_di; +} + +/* Return true if the following pattern recognized. + (insn ... (set (reg/i:SI 10 a0) + (const_int 0 [0])) ...) + (insn ... (use (reg/i:SI 10 a0)) ...) + skip notes + (note ... NOTE_INSN_EPILOGUE_BEG) + ... + (jump_insn/f ... {gpr_multi_popret_up_to_*} */ + +static bool +riscv_zcmp_can_use_popretz (rtx_insn *epilogue, rtx_insn **clear, + rtx_insn **use, rtx_insn **popret) +{ + rtx_insn *insn = epilogue; + + if (!TARGET_ZCMP) + return false; + + /* skip notes */ + while (insn && NOTE_P (insn)) + insn = PREV_INSN (insn); + + /* match use (reg 10 a0) */ + if (insn == NULL || !INSN_P (insn) || GET_CODE (PATTERN (insn)) != USE + || !REG_P (XEXP (PATTERN (insn), 0)) + || REGNO (XEXP (PATTERN (insn), 0)) != A0_REGNUM) + return false; + *use = insn; + if (dump_file) + fprintf (dump_file, "Found use a0 insn in insn %d.\n", INSN_UID (insn)); + + insn = PREV_INSN (insn); + /* match set (reg 10 a0) (const_int 0 [0]) */ + if (insn == NULL || !INSN_P (insn) || GET_CODE (PATTERN (insn)) != SET + || !REG_P (SET_DEST (PATTERN (insn))) + || REGNO (SET_DEST (PATTERN (insn))) != A0_REGNUM + || SET_SRC (PATTERN (insn)) != const0_rtx) + return false; + *clear = insn; + if (dump_file) + fprintf (dump_file, "Found clear a0 in insn %d.\n", INSN_UID (insn)); + + for (insn = epilogue; insn; insn = NEXT_INSN (insn)) + { + if (riscv_popret_insn_p (insn)) + { + *popret = insn; + if (dump_file) + fprintf (dump_file, "Found popret in insn %d.\n", INSN_UID (insn)); + return true; + } + } + + return false; +} + +/* Try to generate popretz insn if possible, + EPILOGUE points to the 1st insn of epilogue. */ + +void static riscv_try_to_gen_popretz (rtx_insn *epilogue) +{ + rtx_insn *clear_a0_insn = NULL, *use_a0 = NULL, *popret = NULL; + if (!riscv_zcmp_can_use_popretz (epilogue, &clear_a0_insn, &use_a0, &popret)) + return; + + rtx pat_popretz = gen_popretz_from_popret_insn (popret); + rtx_insn *popretz + = emit_jump_insn_after_setloc (pat_popretz, PREV_INSN (popret), + INSN_LOCATION (popret)); + REG_NOTES (popretz) = REG_NOTES (popret); + RTX_FRAME_RELATED_P (popretz) = 1; + delete_insn (clear_a0_insn); + delete_insn (use_a0); + delete_insn (popret); + + if (dump_file) + fprintf (dump_file, "popretz successfully generated.\n"); +} + +/* Implement TARGET_POST_EPILOGUE_PROC. */ + +void static riscv_post_epilogue_proc (rtx_insn *epilogue) +{ + riscv_try_to_gen_popretz (epilogue); +} + /* Expand an "epilogue", "sibcall_epilogue", or "eh_return_internal" pattern; style says which. */ @@ -11723,6 +11805,9 @@ riscv_expand_usadd (rtx dest, rtx x, rtx y) #define TARGET_SHRINK_WRAP_SET_HANDLED_COMPONENTS \ riscv_set_handled_components +#undef TARGET_POST_EPILOGUE_PROC +#define TARGET_POST_EPILOGUE_PROC riscv_post_epilogue_proc + /* The generic ELF target does not always have TLS support. */ #ifdef HAVE_AS_TLS #undef TARGET_HAVE_TLS diff --git a/gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c b/gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c index 1e1a8be8705..64da00bc094 100644 --- a/gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c +++ b/gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c @@ -267,3 +267,59 @@ test_popretz () f1 (); return 0; } + +int +f_1 (void) +{ + return __builtin_issignaling (__builtin_nansf16b ("")); +} + +int +f_2 (void) +{ + return __builtin_issignaling (((__bf16) __builtin_nanf (""))); +} + +int +f_3 (void) +{ + return __builtin_issignaling (0.0bf16); +} + +int +f_4 (__bf16 x) +{ + return __builtin_issignaling (x); +} + +__bf16 w; + +/* +**main: +** ... +** li a0,0 +** ... +*/ +int +main () +{ + if (!f_1 () || f_2 () || f_3 ()) + __builtin_abort (); + + asm volatile("" : : : "memory"); + + if (f_4 (w) || !f_4 (__builtin_nansf16b ("0x123")) || f_4 (42.0bf16) + || f_4 (((__bf16) __builtin_nanf ("0x234"))) + || f_4 (((__bf16) __builtin_inff ())) + || f_4 (-((__bf16) __builtin_inff ())) || f_4 (-42.0bf16) + || f_4 (-0.0bf16) || f_4 (0.0bf16)) + __builtin_abort (); + + w = __builtin_nansf16b (""); + asm volatile("" : : : "memory"); + + if (!f_4 (w)) + __builtin_abort (); + + return 0; +} -- 2.17.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] fix RISC-V zcmp popretz [PR113715] 2024-06-05 1:50 [PATCH 0/2] fix RISC-V zcmp popretz [PR113715] Fei Gao 2024-06-05 1:50 ` [PATCH 1/2] target hooks: allow post processing after epilogue inserted Fei Gao 2024-06-05 1:50 ` [PATCH 2/2] [RISC-V]: fix zcmp popretz [PR113715] Fei Gao @ 2024-06-05 6:36 ` Kito Cheng 2024-06-05 7:47 ` Fei Gao 2024-06-05 13:51 ` Jeff Law 2 siblings, 2 replies; 10+ messages in thread From: Kito Cheng @ 2024-06-05 6:36 UTC (permalink / raw) To: Fei Gao; +Cc: gcc-patches, palmer, jeffreyalaw Thanks for fixing this issue, and I am wondering doest it possible to fix that without introduce target hook? I ask that because...GCC 14 also has this bug, but I am not sure it's OK to introduce new target hook for release branch? or would you suggest we just revert patch to fix that on GCC 14? On Wed, Jun 5, 2024 at 9:50 AM Fei Gao <gaofei@eswincomputing.com> wrote: > > The 1st patch adds a hook to allow post processing after epilogue inserted. > The 2nd one implement the RISC-V hook to solve PR113715. > > Fei Gao (2): > target hooks: allow post processing after epilogue inserted. > [RISC-V]: fix zcmp popretz [PR113715]. > > gcc/config/riscv/riscv.cc | 191 ++++++++++++++------ > gcc/doc/tm.texi | 5 + > gcc/doc/tm.texi.in | 2 + > gcc/function.cc | 2 + > gcc/hooks.cc | 7 + > gcc/hooks.h | 1 + > gcc/target.def | 8 + > gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c | 56 ++++++ > 8 files changed, 219 insertions(+), 53 deletions(-) > > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH 0/2] fix RISC-V zcmp popretz [PR113715] 2024-06-05 6:36 ` [PATCH 0/2] fix RISC-V " Kito Cheng @ 2024-06-05 7:47 ` Fei Gao 2024-06-05 13:58 ` Jeff Law 2024-06-05 13:51 ` Jeff Law 1 sibling, 1 reply; 10+ messages in thread From: Fei Gao @ 2024-06-05 7:47 UTC (permalink / raw) To: Kito Cheng; +Cc: gcc-patches, Palmer Dabbelt, jeffreyalaw On 2024-06-05 14:36 Kito Cheng <kito.cheng@gmail.com> wrote: > >Thanks for fixing this issue, and I am wondering doest it possible to >fix that without introduce target hook? I ask that because...GCC 14 >also has this bug, but I am not sure it's OK to introduce new target >hook for release branch? or would you suggest we just revert patch to >fix that on GCC 14? If hook is unacceptable in GCC14, I suggest to revert on GCC 14 the following commit. https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=b27d323a368033f0b37e93c57a57a35fd9997864 I started fixing this issue by adding changes in mach pass but abandoned it due to the following reasons: 1. more codes to detect location of epilogue in the whole insn list. 2. due to impact by scheduling pass, clear a0 and use a0 insns get reordered, resulting in more codes. 3. data flow analysis is needed, but insn does't have bb info any more, so rescan actually does nothing, which I guess there's some hidden issue in riscv_remove_unneeded_save_restore_calls using dfa. So I came up this hook based patch in prologue and epilogue pass to make the optimization happen as earlier as possible. It ends up with simplicity and clear logic. BR Fei > >On Wed, Jun 5, 2024 at 9:50 AM Fei Gao <gaofei@eswincomputing.com> wrote: >> >> The 1st patch adds a hook to allow post processing after epilogue inserted. >> The 2nd one implement the RISC-V hook to solve PR113715. >> >> Fei Gao (2): >> target hooks: allow post processing after epilogue inserted. >> [RISC-V]: fix zcmp popretz [PR113715]. >> >> gcc/config/riscv/riscv.cc | 191 ++++++++++++++------ >> gcc/doc/tm.texi | 5 + >> gcc/doc/tm.texi.in | 2 + >> gcc/function.cc | 2 + >> gcc/hooks.cc | 7 + >> gcc/hooks.h | 1 + >> gcc/target.def | 8 + >> gcc/testsuite/gcc.target/riscv/rv32i_zcmp.c | 56 ++++++ >> 8 files changed, 219 insertions(+), 53 deletions(-) >> >> -- >> 2.17.1 >> ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] fix RISC-V zcmp popretz [PR113715] 2024-06-05 7:47 ` Fei Gao @ 2024-06-05 13:58 ` Jeff Law 2024-06-06 2:42 ` Fei Gao 0 siblings, 1 reply; 10+ messages in thread From: Jeff Law @ 2024-06-05 13:58 UTC (permalink / raw) To: Fei Gao, Kito Cheng; +Cc: gcc-patches, Palmer Dabbelt On 6/5/24 1:47 AM, Fei Gao wrote: > > On 2024-06-05 14:36 Kito Cheng <kito.cheng@gmail.com> wrote: >> >> Thanks for fixing this issue, and I am wondering doest it possible to >> fix that without introduce target hook? I ask that because...GCC 14 >> also has this bug, but I am not sure it's OK to introduce new target >> hook for release branch? or would you suggest we just revert patch to >> fix that on GCC 14? > > If hook is unacceptable in GCC14, I suggest to revert on GCC 14 the following commit. > https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=b27d323a368033f0b37e93c57a57a35fd9997864 > > I started fixing this issue by adding changes in mach pass but abandoned it > due to the following reasons: > 1. more codes to detect location of epilogue in the whole insn list. > 2. due to impact by scheduling pass, clear a0 and use a0 insns get reordered, resulting in more > codes. > 3. data flow analysis is needed, but insn does't have bb info any more, so rescan actually does > nothing, which I guess there's some hidden issue in riscv_remove_unneeded_save_restore_calls > using dfa. > > So I came up this hook based patch in prologue and epilogue pass to make the optimization > happen as earlier as possible. It ends up with simplicity and clear logic. But let's back up and get a good explanation of what the problem is. Based on patch 2/2 it looks like we have lost an assignment to the return register. To someone not familiar with this code, it sounds to me like we've made a mistake earlier and we're now defining a hook that lets us go back and fix that earlier mistake. I'm probably wrong, but so far that's what it sounds like. So let's get a good explanation of the problem and perhaps we'll find a better way to solve it. jeff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH 0/2] fix RISC-V zcmp popretz [PR113715] 2024-06-05 13:58 ` Jeff Law @ 2024-06-06 2:42 ` Fei Gao 2024-06-08 20:36 ` Jeff Law 0 siblings, 1 reply; 10+ messages in thread From: Fei Gao @ 2024-06-06 2:42 UTC (permalink / raw) To: jeffreyalaw, Kito Cheng; +Cc: gcc-patches, Palmer Dabbelt, gaofei On 2024-06-05 21:58 Jeff Law <jeffreyalaw@gmail.com> wrote: > > > >On 6/5/24 1:47 AM, Fei Gao wrote: >> >> On 2024-06-05 14:36 Kito Cheng <kito.cheng@gmail.com> wrote: >>> >>> Thanks for fixing this issue, and I am wondering doest it possible to >>> fix that without introduce target hook? I ask that because...GCC 14 >>> also has this bug, but I am not sure it's OK to introduce new target >>> hook for release branch? or would you suggest we just revert patch to >>> fix that on GCC 14? >> >> If hook is unacceptable in GCC14, I suggest to revert on GCC 14 the following commit. >> https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=b27d323a368033f0b37e93c57a57a35fd9997864 >> >> I started fixing this issue by adding changes in mach pass but abandoned it >> due to the following reasons: >> 1. more codes to detect location of epilogue in the whole insn list. >> 2. due to impact by scheduling pass, clear a0 and use a0 insns get reordered, resulting in more >> codes. >> 3. data flow analysis is needed, but insn does't have bb info any more, so rescan actually does >> nothing, which I guess there's some hidden issue in riscv_remove_unneeded_save_restore_calls >> using dfa. >> >> So I came up this hook based patch in prologue and epilogue pass to make the optimization >> happen as earlier as possible. It ends up with simplicity and clear logic. >But let's back up and get a good explanation of what the problem is. >Based on patch 2/2 it looks like we have lost an assignment to the >return register. > >To someone not familiar with this code, it sounds to me like we've made >a mistake earlier and we're now defining a hook that lets us go back and >fix that earlier mistake. I'm probably wrong, but so far that's what >it sounds like. Hi Jeff You're right. Let me rephrase patch 2/2 with more details. Search /* feigao to location the point I'm tring to explain. code snippets from gcc/function.cc void thread_prologue_and_epilogue_insns (void) { ... /*feigao: targetm.gen_epilogue () is called here to generate epilogue sequence. 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. issue of the commit above: 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. */ rtx_insn *epilogue_seq = make_epilogue_seq (); /* Try to perform a kind of shrink-wrapping, making sure the prologue/epilogue is emitted only around those parts of the function that require it. */ /* feigao: Make a simple_return for those exits that run without prologue and force nonfallthru by e->flags &= ~EDGE_FALLTHRU */ try_shrink_wrapping (&entry_edge, prologue_seq); ... edge exit_fallthru_edge = find_fallthru_edge (EXIT_BLOCK_PTR_FOR_FN (cfun)->preds); /* feigao: fasle here in simple_return case, so no insertion of epilogue, but the li a0,0 insn has already been mistakeny removed. */ if (exit_fallthru_edge) { if (epilogue_seq) { insert_insn_on_edge (epilogue_seq, exit_fallthru_edge); commit_edge_insertions (); /* The epilogue insns we inserted may cause the exit edge to no longer be fallthru. */ FOR_EACH_EDGE (e, ei, EXIT_BLOCK_PTR_FOR_FN (cfun)->preds) { if (((e->flags & EDGE_FALLTHRU) != 0) && returnjump_p (BB_END (e->src))) e->flags &= ~EDGE_FALLTHRU; } find_sub_basic_blocks (BLOCK_FOR_INSN (epilogue_seq)); } ... if (epilogue_seq) { rtx_insn *insn, *next; /* Similarly, move any line notes that appear after the epilogue. There is no need, however, to be quite so anal about the existence of such a note. Also possibly move NOTE_INSN_FUNCTION_BEG notes, as those can be relevant for debug info generation. */ for (insn = epilogue_seq; insn; insn = next) { next = NEXT_INSN (insn); if (NOTE_P (insn) && (NOTE_KIND (insn) == NOTE_INSN_FUNCTION_BEG)) reorder_insns (insn, insn, PREV_INSN (epilogue_seq)); } } /* feigao: new hook */ targetm.post_epilogue_proc (epilogue_seq); ... BR, Fei > >So let's get a good explanation of the problem and perhaps we'll find a >better way to solve it. > >jeff > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] fix RISC-V zcmp popretz [PR113715] 2024-06-06 2:42 ` Fei Gao @ 2024-06-08 20:36 ` Jeff Law 2024-06-28 10:46 ` Fei Gao 0 siblings, 1 reply; 10+ messages in thread From: Jeff Law @ 2024-06-08 20:36 UTC (permalink / raw) To: Fei Gao, Kito Cheng; +Cc: gcc-patches, Palmer Dabbelt On 6/5/24 8:42 PM, Fei Gao wrote: >> But let's back up and get a good explanation of what the problem is. >> Based on patch 2/2 it looks like we have lost an assignment to the >> return register. >> >> To someone not familiar with this code, it sounds to me like we've made >> a mistake earlier and we're now defining a hook that lets us go back and >> fix that earlier mistake. I'm probably wrong, but so far that's what >> it sounds like. > Hi Jeff > > You're right. Let me rephrase patch 2/2 with more details. Search /* feigao to location the point I'm > tring to explain. > > code snippets from gcc/function.cc > void > thread_prologue_and_epilogue_insns (void) > { > ... > /*feigao: > targetm.gen_epilogue () is called here to generate epilogue sequence. > 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. So that seems like the critical issue. Generation of the prologue/epilogue really shouldn't be changing other instructions in the instruction stream. I'm not immediately aware of another target that does that, an it seems like a rather risky thing to do. It looks like the cm.popretz's RTL exposes the assignment to a0 and there's a DCE pass that runs after insertion of the prologue/epilogue. So I would suggest leaving the assignment to a0 in the RTL chain and see if the later DCE pass after prologue generation eliminates the redundant assignment. That seems a lot cleaner. Jeff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Re: [PATCH 0/2] fix RISC-V zcmp popretz [PR113715] 2024-06-08 20:36 ` Jeff Law @ 2024-06-28 10:46 ` Fei Gao 0 siblings, 0 replies; 10+ messages in thread From: Fei Gao @ 2024-06-28 10:46 UTC (permalink / raw) To: jeffreyalaw, Kito Cheng; +Cc: gcc-patches, Palmer Dabbelt On 2024-06-09 04:36 Jeff Law <jeffreyalaw@gmail.com> wrote: > > > >On 6/5/24 8:42 PM, Fei Gao wrote: > >>> But let's back up and get a good explanation of what the problem is. >>> Based on patch 2/2 it looks like we have lost an assignment to the >>> return register. >>> >>> To someone not familiar with this code, it sounds to me like we've made >>> a mistake earlier and we're now defining a hook that lets us go back and >>> fix that earlier mistake. I'm probably wrong, but so far that's what >>> it sounds like. >> Hi Jeff >> >> You're right. Let me rephrase patch 2/2 with more details. Search /* feigao to location the point I'm >> tring to explain. >> >> code snippets from gcc/function.cc >> void >> thread_prologue_and_epilogue_insns (void) >> { >> ... >> /*feigao: >> targetm.gen_epilogue () is called here to generate epilogue sequence. >> 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 Hi Jeff I should have made it clear that there're li a0,0 and use a0 insns instead of just li a0, 0 here. >> is replaced by cm.popretz and li a0,0 insn is deleted. >So that seems like the critical issue. Generation of the >prologue/epilogue really shouldn't be changing other instructions in the >instruction stream. I'm not immediately aware of another target that >does that, an it seems like a rather risky thing to do. > > >It looks like the cm.popretz's RTL exposes the assignment to a0 and >there's a DCE pass that runs after insertion of the prologue/epilogue. >So I would suggest leaving the assignment to a0 in the RTL chain and see >if the later DCE pass after prologue generation eliminates the redundant >assignment. That seems a lot cleaner. The DCE pass after prologue generation may not help here for the following reasons: 1. The use a0 insn is not deletable, and then li a0,0 that defines a0 cannot be deleted. 2. We need to detect pattern (clear a0, use a0 and cm.popret) before generating cm.popretz. I don't think DCE is a good place to put this piece of codes. And I insist prologue and epilogue pass is a better place to do it with simplicity and clear logic as I explained earlier to Kito. Hook was added here safely without any impact on other targets. Please let me know your idea. Thanks. Fei > > > >Jeff ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 0/2] fix RISC-V zcmp popretz [PR113715] 2024-06-05 6:36 ` [PATCH 0/2] fix RISC-V " Kito Cheng 2024-06-05 7:47 ` Fei Gao @ 2024-06-05 13:51 ` Jeff Law 1 sibling, 0 replies; 10+ messages in thread From: Jeff Law @ 2024-06-05 13:51 UTC (permalink / raw) To: Kito Cheng, Fei Gao; +Cc: gcc-patches, palmer On 6/5/24 12:36 AM, Kito Cheng wrote: > Thanks for fixing this issue, and I am wondering doest it possible to > fix that without introduce target hook? I ask that because...GCC 14 > also has this bug, but I am not sure it's OK to introduce new target > hook for release branch? or would you suggest we just revert patch to > fix that on GCC 14? The question I would ask is why is the target hook needed. ie, what problem needs to be solved and how does a new target hook help. Jeff ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-06-28 10:46 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2024-06-05 1:50 [PATCH 0/2] fix RISC-V zcmp popretz [PR113715] Fei Gao 2024-06-05 1:50 ` [PATCH 1/2] target hooks: allow post processing after epilogue inserted Fei Gao 2024-06-05 1:50 ` [PATCH 2/2] [RISC-V]: fix zcmp popretz [PR113715] Fei Gao 2024-06-05 6:36 ` [PATCH 0/2] fix RISC-V " Kito Cheng 2024-06-05 7:47 ` Fei Gao 2024-06-05 13:58 ` Jeff Law 2024-06-06 2:42 ` Fei Gao 2024-06-08 20:36 ` Jeff Law 2024-06-28 10:46 ` Fei Gao 2024-06-05 13:51 ` Jeff Law
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).