On Mon, Feb 3, 2025 at 10:32 AM H.J. Lu <hjl.to...@gmail.com> wrote:
>
> On Mon, Feb 3, 2025 at 5:27 PM Richard Biener
> <richard.guent...@gmail.com> wrote:
> >
> > On Mon, Feb 3, 2025 at 7:23 AM H.J. Lu <hjl.to...@gmail.com> wrote:
> > >
> > > commit 3b9b8d6cfdf59337f4b7ce10ce92a98044b2657b
> > > Author: Surya Kumari Jangala <jskum...@linux.ibm.com>
> > > Date: Tue Jun 25 08:37:49 2024 -0500
> > >
> > > ira: Scale save/restore costs of callee save registers with block
> > > frequency
> > >
> > > scales the cost of saving/restoring a callee-save hard register in
> > > epilogue
> > > and prologue with the entry block frequency, which, if not optimizing for
> > > size, is 10000, for all targets. As the result, callee-saved registers
> > > may not be used to preserve local variable values across calls on some
> > > targets, like x86. Add a target hook for the callee-saved register cost
> > > scale in epilogue and prologue used by IRA. The default version of this
> > > target hook returns 1 if optimizing for size, otherwise returns the entry
> > > block frequency. Add an x86 version of this target hook to restore the
> > > old behavior prior to the above commit.
> > >
> > > PR rtl-optimization/111673
> > > PR rtl-optimization/115932
> > > PR rtl-optimization/116028
> > > PR rtl-optimization/117081
> > > PR rtl-optimization/117082
> > > PR rtl-optimization/118497
> > > * ira-color.cc (assign_hard_reg): Call the target hook for the
> > > callee-saved register cost scale in epilogue and prologue.
> > > * target.def (ira_callee_saved_register_cost_scale): New target
> > > hook.
> > > * targhooks.cc (default_ira_callee_saved_register_cost_scale):
> > > New.
> > > * targhooks.h (default_ira_callee_saved_register_cost_scale):
> > > Likewise.
> > > * config/i386/i386.cc (ix86_ira_callee_saved_register_cost_scale):
> > > New.
> > > (TARGET_IRA_CALLEE_SAVED_REGISTER_COST_SCALE): Likewise.
> > > * doc/tm.texi: Regenerated.
> > > * doc/tm.texi.in (TARGET_IRA_CALLEE_SAVED_REGISTER_COST_SCALE):
> > > New.
> > >
> > > Signed-off-by: H.J. Lu <hjl.to...@gmail.com>
> > > ---
> > > gcc/config/i386/i386.cc | 11 +++++++++++
> > > gcc/doc/tm.texi | 8 ++++++++
> > > gcc/doc/tm.texi.in | 2 ++
> > > gcc/ira-color.cc | 3 +--
> > > gcc/target.def | 12 ++++++++++++
> > > gcc/targhooks.cc | 8 ++++++++
> > > gcc/targhooks.h | 1 +
> > > 7 files changed, 43 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
> > > index f89201684a8..3128973ba79 100644
> > > --- a/gcc/config/i386/i386.cc
> > > +++ b/gcc/config/i386/i386.cc
> > > @@ -20600,6 +20600,14 @@ ix86_class_likely_spilled_p (reg_class_t rclass)
> > > return false;
> > > }
> > >
> > > +/* Implement TARGET_IRA_CALLEE_SAVED_REGISTER_COST_SCALE. */
> > > +
> > > +static int
> > > +ix86_ira_callee_saved_register_cost_scale (int)
> > > +{
> > > + return 1;
> > > +}
> > > +
> > > /* Return true if a set of DST by the expression SRC should be allowed.
> > > This prevents complex sets of likely_spilled hard regs before split1.
> > > */
> > >
> > > @@ -27078,6 +27086,9 @@ ix86_libgcc_floating_mode_supported_p
> > > #define TARGET_PREFERRED_OUTPUT_RELOAD_CLASS
> > > ix86_preferred_output_reload_class
> > > #undef TARGET_CLASS_LIKELY_SPILLED_P
> > > #define TARGET_CLASS_LIKELY_SPILLED_P ix86_class_likely_spilled_p
> > > +#undef TARGET_IRA_CALLEE_SAVED_REGISTER_COST_SCALE
> > > +#define TARGET_IRA_CALLEE_SAVED_REGISTER_COST_SCALE \
> > > + ix86_ira_callee_saved_register_cost_scale
> > >
> > > #undef TARGET_VECTORIZE_BUILTIN_VECTORIZATION_COST
> > > #define TARGET_VECTORIZE_BUILTIN_VECTORIZATION_COST \
> > > diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
> > > index 0de24eda6f0..9f42913a4ef 100644
> > > --- a/gcc/doc/tm.texi
> > > +++ b/gcc/doc/tm.texi
> > > @@ -3047,6 +3047,14 @@ A target hook which can change allocno class for
> > > given pseudo from
> > > The default version of this target hook always returns given class.
> > > @end deftypefn
> > >
> > > +@deftypefn {Target Hook} int TARGET_IRA_CALLEE_SAVED_REGISTER_COST_SCALE
> > > (int @var{hard_regno})
> > > +A target hook which returns the callee-saved register @var{hard_regno}
> > > +cost scale in epilogue and prologue used by IRA.
> > > +
> > > +The default version of this target hook returns 1 if optimizing for
> > > +size, otherwise returns the entry block frequency.
> > > +@end deftypefn
> > > +
> > > @deftypefn {Target Hook} bool TARGET_LRA_P (void)
> > > A target hook which returns true if we use LRA instead of reload pass.
> > >
> > > diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
> > > index 631d04131e3..6dbe22581ca 100644
> > > --- a/gcc/doc/tm.texi.in
> > > +++ b/gcc/doc/tm.texi.in
> > > @@ -2388,6 +2388,8 @@ in the reload pass.
> > >
> > > @hook TARGET_IRA_CHANGE_PSEUDO_ALLOCNO_CLASS
> > >
> > > +@hook TARGET_IRA_CALLEE_SAVED_REGISTER_COST_SCALE
> > > +
> > > @hook TARGET_LRA_P
> > >
> > > @hook TARGET_REGISTER_PRIORITY
> > > diff --git a/gcc/ira-color.cc b/gcc/ira-color.cc
> > > index 0699b349a1a..233060e1587 100644
> > > --- a/gcc/ira-color.cc
> > > +++ b/gcc/ira-color.cc
> > > @@ -2180,8 +2180,7 @@ assign_hard_reg (ira_allocno_t a, bool retry_p)
> > > + ira_memory_move_cost[mode][rclass][1])
> > > * saved_nregs / hard_regno_nregs (hard_regno,
> > > mode) - 1)
> > > - * (optimize_size ? 1 :
> > > - REG_FREQ_FROM_BB (ENTRY_BLOCK_PTR_FOR_FN
> > > (cfun)));
> > > + * targetm.ira_callee_saved_register_cost_scale
> > > (hard_regno);
> > > cost += add_cost;
> > > full_cost += add_cost;
> > > }
> > > diff --git a/gcc/target.def b/gcc/target.def
> > > index 4050b2ebdd4..c348b15815a 100644
> > > --- a/gcc/target.def
> > > +++ b/gcc/target.def
> > > @@ -5714,6 +5714,18 @@ DEFHOOK
> > > reg_class_t, (int, reg_class_t, reg_class_t),
> > > default_ira_change_pseudo_allocno_class)
> > >
> > > +/* Scale of callee-saved register cost in epilogue and prologue used by
> > > + IRA. */
> > > +DEFHOOK
> > > +(ira_callee_saved_register_cost_scale,
> > > + "A target hook which returns the callee-saved register
> > > @var{hard_regno}\n\
> > > +cost scale in epilogue and prologue used by IRA.\n\
> > > +\n\
> > > +The default version of this target hook returns 1 if optimizing for\n\
> > > +size, otherwise returns the entry block frequency.",
> > > + int, (int hard_regno),
> > > + default_ira_callee_saved_register_cost_scale)
> > > +
> > > /* Return true if we use LRA instead of reload. */
> > > DEFHOOK
> > > (lra_p,
> > > diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc
> > > index f80dc8b2e7e..344075efa41 100644
> > > --- a/gcc/targhooks.cc
> > > +++ b/gcc/targhooks.cc
> > > @@ -1305,6 +1305,14 @@ default_ira_change_pseudo_allocno_class (int regno
> > > ATTRIBUTE_UNUSED,
> > > return cl;
> > > }
> > >
> > > +int
> > > +default_ira_callee_saved_register_cost_scale (int)
> > > +{
> > > + return (optimize_size
> > > + ? 1
> >
> > Why use optimize_size when REG_FREQ_FROM_BB uses
>
> My patch just copies what
>
> commit 3b9b8d6cfdf59337f4b7ce10ce92a98044b2657b
> Author: Surya Kumari Jangala <jskum...@linux.ibm.com>
> Date: Tue Jun 25 08:37:49 2024 -0500
>
> ira: Scale save/restore costs of callee save registers with block
> frequency
>
> does.
>
> > ((optimize_function_for_size_p (cfun) \
> > || !cfun->cfg->count_max.initialized_p ())
> >
> > ? You are changing behavior for all targets this way? Did you consider
> > altering REG_FREQ_FROM_BB to
> >
> > #define REG_FREQ_FROM_BB(bb, size_value)
> > ((optimize_function_for_size_p (cfun) \
> > || !cfun->cfg->count_max.initialized_p ())
> > \
> > ? default \
> > : ((bb)->count.to_frequency (cfun)
> > \
> > * REG_FREQ_MAX / BB_FREQ_MAX)
> > \
> > ? ((bb)->count.to_frequency (cfun)
> > \
> > * REG_FREQ_MAX / BB_FREQ_MAX)
> > \
> > : 1)
> >
> > so return a specified value for the optimize_size case? (see the
> > other thread where
> > I wonder about that odd !cfun->cfg->count_max.initialized_p () check)
> >
> > IMO at this point a new target hook should preserve existing behavior by
> > default
> > or alternatively the original patch should be reverted as causing
> > regressions and
> > a new patch introducing the target hook should be installed in next stage1.
>
> I believe the original patch should be reverted. Then my patch isn't needed.
I'm OK with that, but it's not my call. I do wonder why the contributor did not
address any of the fallout. Maybe he's gone? Peter?
Thanks,
Richard.
>
> >
> > Richard.
> >
> > > + : REG_FREQ_FROM_BB (ENTRY_BLOCK_PTR_FOR_FN (cfun)));
> > > +}
> > > +
> > > extern bool
> > > default_lra_p (void)
> > > {
> > > diff --git a/gcc/targhooks.h b/gcc/targhooks.h
> > > index 7d15f632c23..8871e01430c 100644
> > > --- a/gcc/targhooks.h
> > > +++ b/gcc/targhooks.h
> > > @@ -173,6 +173,7 @@ extern void default_emit_call_builtin___clear_cache
> > > (rtx, rtx);
> > > extern poly_int64 default_return_pops_args (tree, tree, poly_int64);
> > > extern reg_class_t default_ira_change_pseudo_allocno_class (int,
> > > reg_class_t,
> > > reg_class_t);
> > > +extern int default_ira_callee_saved_register_cost_scale (int);
> > > extern bool default_lra_p (void);
> > > extern int default_register_priority (int);
> > > extern bool default_register_usage_leveling_p (void);
> > > --
> > > 2.48.1
> > >
>
>
>
> --
> H.J.
