On Thu, 22 Jan 2026, Richard Sandiford wrote: > Avinash Jayakar <[email protected]> writes: > > Hi, > > > > This is a patch proposed to fix the latent scheduler issue seen in PR123144, > > PR80357 and PR94014. > > > > I was not able to generate a test case for this specific scenario. All the > > test > > cases mentioned in this PR mostly compile fine due to changes in gimple > > passes. > > I tried to generate gimple of the optimized and compile using -fgimple, but > > there were many parsing issues. Any suggestions on how to test this robustly > > would be welcome. > > > > Bootstrapped and regtested on powerpc64le and x86_64 without any > > regressions. > > Also tested on gcc-14 which fixed PR113390. Ok for trunk and release-14? > > > > Thanks and regards, > > Avinash Jayakar > > > > A latent issue in haifa scheduler cause ICE sometimes due to > > inconsistent register pressure values. PR123144, PR80357 and PR94014 > > showed how this issue caused ICE. > > > > The problem was in the model_recompute function. During region > > scheduling, the ready list may have instruction from multiple basic > > blocks, but the model_schedule always has insns within the target_bb. > > So when model_recompute is run for instruction that is not in the > > model_schedule, the register pressure values would go negative in some > > cases leading to assertion error in model_update_limit_points_in_group. > > > > This patch just adds a guard in model_recompute function to not do it > > when insn is not in model. > > > > gcc/ChangeLog: > > PR 123144 > > PR 80357 > > PR 94014 > > * haifa-sched.cc (model_recompute): Return if insn not in > > model_schedule. > > --- > > gcc/haifa-sched.cc | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/gcc/haifa-sched.cc b/gcc/haifa-sched.cc > > index 4daa4d7b18a..b641d7a21a2 100644 > > --- a/gcc/haifa-sched.cc > > +++ b/gcc/haifa-sched.cc > > @@ -2130,6 +2130,14 @@ model_recompute (rtx_insn *insn) > > /* The destinations of INSN were previously live from POINT onwards, but > > are > > now live from model_curr_point onwards. Set up DELTA accordingly. */ > > point = model_index (insn); > > + > > + /* In case if insn is not in the model, no need to recompute. > > + This may happen in region scheduling, where the model schedule only has > > insns > > + from the basic block, but scheduled insn may be in a different block in > > the > > + same region. Recompute in this case would result in inconsistent > > pressure > > + values. */ > > + if (point == model_num_insns) > > + return; > > reg_pressure = INSN_REG_PRESSURE (insn); > > for (pci = 0; pci < ira_pressure_classes_num; pci++) > > { > > Thanks for looking at this. I'm not sure that the patch is the right > approach though. IIRC, the idea is that, when insns are pulled from > other blocks, the pressures should be updated to account for any change > in the live registers. This can be seen with the first insn in the block > that triggers the ICE in PR 123144: > > ;; Ready list (t = 0): 303/b4:108(cost=0:prio=39:delay=1:idx=5) > 301/b4:106(cost=0:prio=39:delay=1:idx=5) 129:92(cost=0:prio=13:idx=0) > 308/b4:118(cost=0:prio=25:idx=5) 306/b4:115(cost=0:prio=29:idx=5) > 304/b4:112(cost=0:prio=33:idx=5) 160/b4:110(cost=0:prio=35:idx=5) > // rs6000_variable_issue (more = 7) = 6 > ;; 0--> b 4: i 160 {r244=vec_select(r234#0,parallel);clobber > scratch;}:DU_any_power8,VSU_power8:GENERAL_REGS+1(1)ALTIVEC_REGS+0(0)VSX_REGS+0(0)CR_REGS+0(0)SPECIAL_REGS+0(0) > ;; +------------------------------------------------------ > ;; | New pressure for model schedule > ;; +------------------------------------------------------ > ;; | 4 133 pc={(r229==0)?L138:pc} > GENERAL_REGS:[19->20] ALTIVEC_REGS:[2->2] VSX_REGS:[0->0] CR_REGS:[0->0] > SPECIAL_REGS:[0->0] > ;; | 3 132 r229=cmp(r228,0) > GENERAL_REGS:[19->20] ALTIVEC_REGS:[2->2] VSX_REGS:[0->0] CR_REGS:[0->0] > SPECIAL_REGS:[0->0] > ;; | 2 131 r228=zxn([r227+0x60]) > GENERAL_REGS:[19->20] ALTIVEC_REGS:[2->2] VSX_REGS:[0->0] CR_REGS:[0->0] > SPECIAL_REGS:[0->0] > ;; | 1 130 r227=r161+r226 > GENERAL_REGS:[19->20] ALTIVEC_REGS:[2->2] VSX_REGS:[0->0] CR_REGS:[0->0] > SPECIAL_REGS:[0->0] > ;; | 0 129 r226=sxn(r132) > GENERAL_REGS:[19->20] ALTIVEC_REGS:[2->2] VSX_REGS:[0->0] CR_REGS:[0->0] > SPECIAL_REGS:[0->0] > ;; +------------------------------------------------------ > ;; GENERAL_REGS:20(-7) ALTIVEC_REGS:2(-29) VSX_REGS:0(-61) > CR_REGS:0(-8) SPECIAL_REGS:0(-2) > > That is, r244 (the destination of insn 160) is assumed to occupy > GENERAL_REGS. Since insn 160 came from a different block, there is an > increase in the GENERAL_REGS pressure estimates for each unscheduled > insn in the block. The loop: > > reg_pressure = INSN_REG_PRESSURE (insn); > for (pci = 0; pci < ira_pressure_classes_num; pci++) > { > cl = ira_pressure_classes[pci]; > delta[cl] = reg_pressure[pci].set_increase; > } > > sets up this increase. If, later, the last use of r244 in the region > is scheduled, the following loop: > > /* Record which registers previously died at POINT, but which now die > before POINT. Adjust DELTA so that it represents the effect of > this change after POINT - 1. Set NUM_PENDING_BIRTHS to the number of > registers that will be born in the range [model_curr_point, POINT). */ > num_uses = 0; > num_pending_births = 0; > bitmap_clear (tmp_bitmap); > for (use = INSN_REG_USE_LIST (insn); use != NULL; use = use->next_insn_use) > { > new_last = model_last_use_except (use); > if (new_last < point && bitmap_set_bit (tmp_bitmap, use->regno)) > { > gcc_assert (num_uses < ARRAY_SIZE (uses)); > uses[num_uses].last_use = new_last; > uses[num_uses].regno = use->regno; > /* This register is no longer live after POINT - 1. */ > mark_regno_birth_or_death (NULL, delta, use->regno, false); > num_uses++; > if (new_last >= 0) > num_pending_births++; > } > } > > should bring about a corresponding decrease in pressure. > > Similarly, if the region initially has uses of a register both in the > block being scheduled ("block B") and in other blocks, scheduling the > last use in other blocks would make the last use in block B a dying use. > This would likewise affect the model schedule, with the pressure > decreasing after the last use in block B. > > Like you say in the covering note, something has gone wrong if the > pressures end up negative. I think the problem is that the initial > start-of-block pressures are based on the original live-in sets for > the basic block, with no updates for insns that were moved by previous > schedules in the same region. model_recompute can therefore count > a dying use for something that wasn't assumed to be live. > > The patch below tries to work around that. Bootstrapped & > regression-tested on powerpc64le-linux-gnu (gcc120). OK to install?
OK. > Although the testcase no longer triggers an ICE, I think it's still > worth including for defensive reasons. > > Thanks, > Richard > > > -- 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)
