| Message ID | 20230620094052.35993-2-gaofei@eswincomputing.com |
|---|---|
| State | New |
| Headers | show |
| Series | resolve confilct between RISC-V zcmp and shrink-wrap-separate | expand |
On 6/20/23 03:40, Fei Gao wrote: > gcc/ChangeLog: > > * shrink-wrap.cc (try_shrink_wrapping_separate):call > use_shrink_wrapping_separate. > (use_shrink_wrapping_separate): wrap the condition > check in use_shrink_wrapping_separate. > * shrink-wrap.h (use_shrink_wrapping_separate): add to extern I'm still missing somethign here. Why doesn't the RISC-V target simply disable separate shrink wrapping by indicating no components are eligible in the relevant cases. ie, I do not think we need another knob here. To be more concrete: > /* Implement TARGET_SHRINK_WRAP_GET_SEPARATE_COMPONENTS. */ > > static sbitmap > riscv_get_separate_components (void) > { > HOST_WIDE_INT offset; > sbitmap components = sbitmap_alloc (FIRST_PSEUDO_REGISTER); > bitmap_clear (components); > > if (riscv_use_save_libcall (&cfun->machine->frame) > || cfun->machine->interrupt_handler_p > || !cfun->machine->frame.gp_sp_offset.is_constant ()) > return components; Don't we get the behavior we want if we change this code to return an zero'd sbitmap? jeff
hi Jeff Please see my earlier reply here. https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg310656.html Maybe you scrolled past it in so many emails:) BR, Fei On 2023-06-25 21:36 Jeff Law <jeffreyalaw@gmail.com> wrote: > > > >On 6/20/23 03:40, Fei Gao wrote: >> gcc/ChangeLog: >> >> * shrink-wrap.cc (try_shrink_wrapping_separate):call >> use_shrink_wrapping_separate. >> (use_shrink_wrapping_separate): wrap the condition >> check in use_shrink_wrapping_separate. >> * shrink-wrap.h (use_shrink_wrapping_separate): add to extern >I'm still missing somethign here. > >Why doesn't the RISC-V target simply disable separate shrink wrapping by >indicating no components are eligible in the relevant cases. ie, I do >not think we need another knob here. > >To be more concrete: > >> /* Implement TARGET_SHRINK_WRAP_GET_SEPARATE_COMPONENTS. */ >> >> static sbitmap >> riscv_get_separate_components (void) >> { >> HOST_WIDE_INT offset; >> sbitmap components = sbitmap_alloc (FIRST_PSEUDO_REGISTER); >> bitmap_clear (components); >> >> if (riscv_use_save_libcall (&cfun->machine->frame) >> || cfun->machine->interrupt_handler_p >> || !cfun->machine->frame.gp_sp_offset.is_constant ()) >> return components; >Don't we get the behavior we want if we change this code to return an >zero'd sbitmap? > >jeff
On 6/25/23 20:29, Fei Gao wrote: > hi Jeff > > Please see my earlier reply here. > https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg310656.html > > Maybe you scrolled past it in so many emails:) It definitely got lost in my mountain of mail. jeff
On 6/25/23 20:29, Fei Gao wrote: > hi Jeff > > Please see my earlier reply here. > https://www.mail-archive.com/gcc-patches@gcc.gnu.org/msg310656.html > > Maybe you scrolled past it in so many emails:) Oh, so the issue isn't really the set of components being wrapped, but the way in which we save them. Yea, that's going to need some tinkering. It does make me wonder if we can handle this in riscv_override_options. That's a pretty standard place to deal with option conflicts. We ought to be able to check if both options are enabled, then disable zcmp push/pop at that poing without introducing any new hooks. jeff
diff --git a/gcc/shrink-wrap.cc b/gcc/shrink-wrap.cc index b8d7b557130..d534964321a 100644 --- a/gcc/shrink-wrap.cc +++ b/gcc/shrink-wrap.cc @@ -1776,16 +1776,14 @@ insert_prologue_epilogue_for_components (sbitmap components) commit_edge_insertions (); } -/* The main entry point to this subpass. FIRST_BB is where the prologue - would be normally put. */ -void -try_shrink_wrapping_separate (basic_block first_bb) +bool +use_shrink_wrapping_separate (void) { if (!(SHRINK_WRAPPING_ENABLED - && flag_shrink_wrap_separate - && optimize_function_for_speed_p (cfun) - && targetm.shrink_wrap.get_separate_components)) - return; + && flag_shrink_wrap_separate + && optimize_function_for_speed_p (cfun) + && targetm.shrink_wrap.get_separate_components)) + return false; /* We don't handle "strange" functions. */ if (cfun->calls_alloca @@ -1794,6 +1792,17 @@ try_shrink_wrapping_separate (basic_block first_bb) || crtl->calls_eh_return || crtl->has_nonlocal_goto || crtl->saves_all_registers) + return false; + + return true; +} + +/* The main entry point to this subpass. FIRST_BB is where the prologue + would be normally put. */ +void +try_shrink_wrapping_separate (basic_block first_bb) +{ + if (!use_shrink_wrapping_separate ()) return; /* Ask the target what components there are. If it returns NULL, don't diff --git a/gcc/shrink-wrap.h b/gcc/shrink-wrap.h index 161647711a3..82386c2b712 100644 --- a/gcc/shrink-wrap.h +++ b/gcc/shrink-wrap.h @@ -26,6 +26,7 @@ along with GCC; see the file COPYING3. If not see extern bool requires_stack_frame_p (rtx_insn *, HARD_REG_SET, HARD_REG_SET); extern void try_shrink_wrapping (edge *entry_edge, rtx_insn *prologue_seq); extern void try_shrink_wrapping_separate (basic_block first_bb); +extern bool use_shrink_wrapping_separate (void); #define SHRINK_WRAPPING_ENABLED \ (flag_shrink_wrap && targetm.have_simple_return ())
gcc/ChangeLog: * shrink-wrap.cc (try_shrink_wrapping_separate):call use_shrink_wrapping_separate. (use_shrink_wrapping_separate): wrap the condition check in use_shrink_wrapping_separate. * shrink-wrap.h (use_shrink_wrapping_separate): add to extern --- gcc/shrink-wrap.cc | 25 +++++++++++++++++-------- gcc/shrink-wrap.h | 1 + 2 files changed, 18 insertions(+), 8 deletions(-)