On Wed, Jun 17, 2026 at 10:05 AM Jovan Dmitrovic <[email protected]> wrote: > > Hello Richard, > thank you for your input. I agree that I should've been fully transparent > about all the changes made.
I'll note that esp. given there's lack of people fully grasping IVOPTs it's important to split up changes as much as possible to ease review. > I have to point out that we had discussion on this topic back in 2023 [1]. > the first return value was originally your idea. This also aligns with > estimate_reg_pressure_cost in loop analysis, which returns 0 in the > same circumstance. Again, this change should've been mentioned by the > commit message. I see (and I don't remember ...). Please split this change out, ideally also referring to the previous discussion. > As for the n_cand bias, and correct me if I'm wrong, I was under the > impression > that we already have a bias in selecting a set of fewer candidates in place. Could be, but it's change to the magic "cost" number. Please split it out, pointing to the alternate place where fewer-candidate IVs are preferenced. > If that is the case, wouldn't adding this n_cands to overall cost negatively > impact our candidate selection heuristic, making it very unlikely for larger > candidate sets ever to be chosen? I have no idea. I'm doing these reviews because nobody else does and I always have to dive into IVOPTs and try to understand it. So I'm mostly trying to avoid changing things because heuristics are usually magic and any change will definitely cause regressions. That might be fine, at least if overall this improves maintainability of the code and regressions are outweight by progressions elsewhere. Richard. > > Best, > Jovan > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2023-May/618577.html > > > On 6/16/26 15:56, Richard Biener wrote: > > On Wed, May 20, 2026 at 2:38 PM Jovan Dmitrovic > > <[email protected]> wrote: > >> Currently, ivopt calculates register pressure cost so that the cost of > >> spilling induction variables is exactly double the cost of spilling > >> loop invariants, which is not exactly right. > >> As ivopts already has `adjust_setup_cost` function, we can use it > >> in order to make the invariant part of the cost relatively cheaper. > >> > >> Tested with CoreMark PRO on RISC-V P8700 CPU (~1.2% improvement), > >> and on x86-64 Intel Core i7-12700H CPU (~0.4% improvement). > >> > >> gcc/ChangeLog: > >> > >> * tree-ssa-loop-ivopts.cc (ivopts_estimate_reg_pressure): > >> Change register pressure estimate in order to respect > >> the impact of spilling induction variables, compared > >> to spilling loop invariants. > >> > >> Co-authored-by: Radosav Krunić <[email protected]> > >> Signed-off-by: Djordje Todorović <[email protected]> > >> Signed-off-by: Chao-Ying Fu <[email protected]> > >> Signed-off-by: Jovan Dmitrović <[email protected]> > >> Signed-off-by: Radosav Krunić <[email protected]> > >> --- > >> gcc/tree-ssa-loop-ivopts.cc | 23 +++++++++++++---------- > >> 1 file changed, 13 insertions(+), 10 deletions(-) > >> > >> diff --git a/gcc/tree-ssa-loop-ivopts.cc b/gcc/tree-ssa-loop-ivopts.cc > >> index 776487d0f8b1..19ba620fb070 100644 > >> --- a/gcc/tree-ssa-loop-ivopts.cc > >> +++ b/gcc/tree-ssa-loop-ivopts.cc > >> @@ -6085,6 +6085,7 @@ ivopts_estimate_reg_pressure (struct ivopts_data > >> *data, unsigned n_invs, > >> unsigned cost; > >> unsigned n_old = data->regs_used, n_new = n_invs + n_cands; > >> unsigned regs_needed = n_new + n_old, available_regs = > >> target_avail_regs; > >> + unsigned iv_spill_cost = 0; > >> bool speed = data->speed; > >> > >> /* If there is a call in the loop body, the call-clobbered registers > >> @@ -6094,7 +6095,7 @@ ivopts_estimate_reg_pressure (struct ivopts_data > >> *data, unsigned n_invs, > >> > >> /* If we have enough registers. */ > >> if (regs_needed + target_res_regs < available_regs) > >> - cost = n_new; > >> + cost = 0; > >> /* If close to running out of registers, try to preserve them. */ > >> else if (regs_needed <= available_regs) > >> cost = target_reg_cost [speed] * regs_needed; > >> @@ -6103,17 +6104,19 @@ ivopts_estimate_reg_pressure (struct ivopts_data > >> *data, unsigned n_invs, > >> else if (n_cands <= available_regs) > >> cost = target_reg_cost [speed] * available_regs > >> + target_spill_cost [speed] * (regs_needed - available_regs); > >> - /* If the number of candidates runs out available registers, we penalize > >> - extra candidate registers using target_spill_cost * 2. Because it is > >> - more expensive to spill induction variable than invariant. */ > >> + /* If the number of candidates runs out available registers, > >> + save the spill cost as separate variable iv_spill_cost. */ > >> else > >> - cost = target_reg_cost [speed] * available_regs > >> - + target_spill_cost [speed] * (n_cands - available_regs) * 2 > >> - + target_spill_cost [speed] * (regs_needed - n_cands); > >> + { > >> + cost = target_reg_cost [speed] * available_regs > >> + + target_spill_cost [speed] * (regs_needed - n_cands); > >> + iv_spill_cost = target_spill_cost[speed] * > >> + (n_cands - available_regs); > >> + } > >> > >> - /* Finally, add the number of candidates, so that we prefer eliminating > >> - induction variables if possible. */ > >> - return cost + n_cands; > >> + /* Adjust cost of invariant register pressure in order to make > >> + the spilling of induction variables the most taxing. */ > >> + return adjust_setup_cost(data, cost) + iv_spill_cost; > > You are changing many details here that you do not specifically > > mention, one is the return value when regs_needed + target_res_regs < > > available_regs, > > another is no longer adding the n_cands bias. > > > > Please avoid that. I think that eventually scaling the invariant > > spill cost with adjust_setup_cost makes sense, but please only > > do that, or in a more obvious way. > > > >> } > >> > >> /* For each size of the induction variable set determine the penalty. */ > >> -- > >> 2.34.1
