On Mon, 17 Nov 2025, Konstantinos Eleftheriou wrote: > Hi Richard, thanks for the feedback. > > On Thu, Nov 13, 2025 at 12:07 PM Richard Biener <[email protected]> wrote: > > > On Wed, 12 Nov 2025, Konstantinos Eleftheriou wrote: > > > > > This patch adds a new cost function so that the targets can decide if the > > > store/load reordering will be profitable. It also adds the > > > `asf-default-cost-value` parameter, which is used as the default return > > > value for the cost function. > > > > It's a bit odd the param has "cost_value" and the hook says cost_p when > > this is a simple flag in the end. To that extent the "default" hook > > should be either returning true or false and the knob the user has > > is -f[no-]avoid-store-forwarding already, no need to add a --param. > > > The new parameter is "false" by default and "true" for AArch64 only. In > case that we keep the default implementation, if we change it to `return > flag_avoid_store_forwarding` and remove the new parameter, it will return > "true" for all targets, as the pass will be enabled by default on all > targets. Is that okay?
I don't see why we need to have a target hook then. I think it makes sense that target can opt out of the added compile-time of the whole pass if it does not implement any store-to-load forwarding (AVR?), so I'd like to see this implemented in terms of that. That said, we have other passes (ccmp?) that face a similar situation and I think we should have some consistency here. But I do not remember whether we settled on a canonical way to gate passes based on target features. Richard. > > > > One could even say the target hook should _not_ have a default > > implementation and only targets that implement the hook should have > > the pass enabled - we only pay with compile-time cost otherwise. > > > > So please rework this part. > > > > Richard. > > > > > gcc/ChangeLog: > > > > > > * doc/tm.texi: Regenerate. > > > * doc/tm.texi.in: Document new target hook. > > > * params.opt: New parameter. > > > * target.def: New DEFHOOK. > > > * targhooks.cc (default_avoid_store_forwarding_reorder_cost_p): > > > New function. > > > * targhooks.h (default_avoid_store_forwarding_reorder_cost_p): > > > New function declaration. > > > > > > Signed-off-by: Konstantinos Eleftheriou < > > [email protected]> > > > --- > > > > > > (no changes since v1) > > > > > > gcc/doc/tm.texi | 7 +++++++ > > > gcc/doc/tm.texi.in | 2 ++ > > > gcc/params.opt | 4 ++++ > > > gcc/target.def | 9 +++++++++ > > > gcc/targhooks.cc | 9 +++++++++ > > > gcc/targhooks.h | 5 +++++ > > > 6 files changed, 36 insertions(+) > > > > > > diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi > > > index fd208f53844a..aef552aac5f8 100644 > > > --- a/gcc/doc/tm.texi > > > +++ b/gcc/doc/tm.texi > > > @@ -7466,6 +7466,13 @@ needed, the sequence cost and additional relevant > > information is given in > > > the arguments so that the target can make an informed decision. > > > @end deftypefn > > > > > > +@deftypefn {Target Hook} bool > > TARGET_AVOID_STORE_FORWARDING_REORDER_COST_P (machine_mode, > > @var{machine_mode}, @var{HOST_WIDE_INT}) > > > +This hook decides if it's profitable for the target to rearrange a store > > > +and a load instruction to avoid a potential store forwarding stall. > > > +The store and load modes and the byte offset of the store within the > > > +load are given as arguments. > > > +@end deftypefn > > > + > > > @node Scheduling > > > @section Adjusting the Instruction Scheduler > > > > > > diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in > > > index 14315dd50805..0c3168cb6dde 100644 > > > --- a/gcc/doc/tm.texi.in > > > +++ b/gcc/doc/tm.texi.in > > > @@ -4781,6 +4781,8 @@ Define this macro if a non-short-circuit operation > > produced by > > > > > > @hook TARGET_AVOID_STORE_FORWARDING_P > > > > > > +@hook TARGET_AVOID_STORE_FORWARDING_REORDER_COST_P > > > + > > > @node Scheduling > > > @section Adjusting the Instruction Scheduler > > > > > > diff --git a/gcc/params.opt b/gcc/params.opt > > > index f8884e976e7f..96720651731d 100644 > > > --- a/gcc/params.opt > > > +++ b/gcc/params.opt > > > @@ -1091,6 +1091,10 @@ Maximum size of a single store merging region in > > bytes. > > > Common Joined UInteger Var(param_store_forwarding_max_distance) > > Init(10) IntegerRange(0, 1000) Param Optimization > > > Maximum number of instruction distance that a small store forwarded to > > a larger load may stall. Value '0' disables the cost checks for the > > avoid-store-forwarding pass. > > > > > > +-param=asf-default-cost-value= > > > +Common Joined UInteger Var(param_asf_default_cost_value) Init(0) > > IntegerRange(0, 1) Param Optimization > > > +Default return value for the avoid_store_forwarding_reorder_cost_p > > function. > > > + > > > -param=switch-conversion-max-branch-ratio= > > > Common Joined UInteger Var(param_switch_conversion_branch_ratio) > > Init(8) IntegerRange(1, 65536) Param Optimization > > > The maximum ratio between array size and switch branches for a switch > > conversion to take place. > > > diff --git a/gcc/target.def b/gcc/target.def > > > index f288329ffcab..d60510e5458e 100644 > > > --- a/gcc/target.def > > > +++ b/gcc/target.def > > > @@ -7158,6 +7158,15 @@ the arguments so that the target can make an > > informed decision.", > > > bool, (vec<store_fwd_info>, rtx, int, bool), > > > default_avoid_store_forwarding_p) > > > > > > +DEFHOOK > > > +(avoid_store_forwarding_reorder_cost_p, > > > + "This hook decides if it's profitable for the target to rearrange a > > store\n\ > > > +and a load instruction to avoid a potential store forwarding stall.\n\ > > > +The store and load modes and the byte offset of the store within the\n\ > > > +load are given as arguments.", > > > + bool, (machine_mode, machine_mode, HOST_WIDE_INT), > > > + default_avoid_store_forwarding_reorder_cost_p) > > > + > > > /* Determine the type of unwind info to emit for debugging. */ > > > DEFHOOK > > > (debug_unwind_info, > > > diff --git a/gcc/targhooks.cc b/gcc/targhooks.cc > > > index 1873d572ba3f..15d882f4703e 100644 > > > --- a/gcc/targhooks.cc > > > +++ b/gcc/targhooks.cc > > > @@ -2352,6 +2352,15 @@ default_avoid_store_forwarding_p > > (vec<store_fwd_info>, rtx, int total_cost, > > > return true; > > > } > > > > > > +/* The default implementation of TARGET_AVOID_STORE_FORWARDING_COST_P. > > */ > > > + > > > +bool > > > +default_avoid_store_forwarding_reorder_cost_p (machine_mode, > > machine_mode, > > > + HOST_WIDE_INT) > > > +{ > > > + return param_asf_default_cost_value; > > > +} > > > + > > > /* Determine the debugging unwind mechanism for the target. */ > > > > > > enum unwind_info_type > > > diff --git a/gcc/targhooks.h b/gcc/targhooks.h > > > index 92e7a4cb10f1..5389ea1cb78b 100644 > > > --- a/gcc/targhooks.h > > > +++ b/gcc/targhooks.h > > > @@ -264,6 +264,11 @@ extern unsigned char default_class_max_nregs > > (reg_class_t, machine_mode); > > > extern bool default_avoid_store_forwarding_p (vec<store_fwd_info>, rtx, > > int, > > > bool); > > > > > > +extern bool > > > +default_avoid_store_forwarding_reorder_cost_p (machine_mode store_mode, > > > + machine_mode load_mode, > > > + HOST_WIDE_INT offset); > > > + > > > extern enum unwind_info_type default_debug_unwind_info (void); > > > > > > extern void default_canonicalize_comparison (int *, rtx *, rtx *, bool); > > > > > > > -- > > Richard Biener <[email protected]> > > SUSE Software Solutions Germany GmbH, > > Frankenstrasse 146, 90461 Nuernberg, Germany; > > GF: Jochen Jaser, Andrew McDonald, Werner Knoblich; (HRB 36809, AG > > Nuernberg) > > > Konstantinos. > -- Richard Biener <[email protected]> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Jochen Jaser, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
