On Wed, Nov 19, 2025 at 10:32 AM Richard Biener <[email protected]> wrote:
> On Tue, 18 Nov 2025, Konstantinos Eleftheriou wrote: > > > On Mon, Nov 17, 2025 at 2:51 PM Richard Biener <[email protected]> > wrote: > > > > > 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. > > > > > We are thinking of adding a flag that would be set to "true" on targets > > that > > want to use the pass and enable it by default on AArch64 and maybe > x86-64. > > How does that sound? > > Well. I think it's OK to enable on a subset of targets by default, > but it should be target maintainers say whether a target is among the > crowd or not. There's the target specific optimization_table as > mechanism to achieve such defaults, like > > gcc/common/config/i386/i386-common.cc > > static const struct default_options ix86_option_optimization_table[] = > { > /* Enable redundant extension instructions removal at -O2 and higher. > */ > { OPT_LEVELS_2_PLUS, OPT_free, NULL, 1 }, > /* Enable function splitting at -O2 and higher. */ > { OPT_LEVELS_2_PLUS, OPT_freorder_blocks_and_partition, NULL, 1 }, > ... > > that's the canonical way to achieve a per-target default. > > Iff there's an actual mechanism that a target can do individual > costing that might be also a way to signal support and we can > decide a default based on such implementation, but I don't see that > here so I suggest to go the above way. > > I've seen no agreement to enable this on x86 btw. > > Richard. > A new version is on the list ( https://gcc.gnu.org/pipermail/gcc-patches/2025-November/701232.html). We removed the cost-function and enabled the pass on AArch64 only. Konstantinos. > > > Konstantinos. > > > > > > > > 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) > > > > -- > 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)
