On Tue, Feb 3, 2026 at 3:03 PM Jeffrey Law <[email protected]> wrote:
>
>
>
> On 2/3/2026 3:23 AM, Roger Sayle wrote:
> > This is my fix for PR 123833, an ICE-on-valid regression in GCC 16, that
> > is categorized as target-specific for mips64el, but the underlying issue
> > affects all targets that use RTL insn attributes in their implementations
> > of insn_costs, for example to determine the length, latency or type of
> > an instruction.
> >
> > The unanticipated interaction is in rtl_ssa::changes_are_worthwhile,
> > which when making a change (for example during fwprop) compares the
> > target's insn_cost of the old instruction pattern to the replacement
> > pattern. For backends whose insn_cost implementation uses attributes
> > from the machine description, this involves calling recog on the same
> > rtx_insn* but with different patterns in quick succession, which plays
> > havoc with recog's recog_data.insn caching. In the test case given
> > in the PR, this ultimately leads to the recog_data cache becoming
> > stale in fwprop, which ultimately leads to an RTL assertion failure
> > in a later pass. Targets without these assertions would just see
> > inexplicable/weird behavior.
> >
> > The proposed fix is to introduce a safe_insn_cost wrapper that saves
> > and restores recog_data around calls to insn_cost, which can then
> > be used in circumstances where an instruction is in the process of
> > changing. For now these are rtl_ssa::changes_are_worthwhile (the
> > critical instance) and rtl-ssa/insns.cc's insn_info::calculate_cost
> > (the only other use of insn_cost in the rtl-ssa subdirectory).
> > In the hope this may be useful/needed in other locations, I've made
> > safe_insn_cost a global function (like insn_cost), but the implementation
> > could be kept local (or duplicated) if preferred by reviewers.
> >
> > I doubt there's an observable compile-time performance impact from this
> > change, but in theory it's possible to avoid overhead of saving/restoring
> > recog_data (on target's whose insn_costs don't call recog) by selectively
> > clearing recog_data.insn if it would be used or has been updated by the
> > call to insn_cost. But that logic is a little more fragile. Likewise,
> > for RTL_CHECKING, it might be useful to cache INSN_CODE in recog_data,
> > and add an assert that this also matches for insn == recog_data.insn
> > cache hits (which would diagnose stale cache contents in future). I'm
> > not sure either of these refinements is suitable for stage 4.
> >
> > A previous patch for this issue, that fixed the symptoms but didn't cure
> > the disease, is described in bugzilla. Many thanks to Andrew Pinski
> > for prompting me to track down where the recog_data corruption was
> > taking place.
> >
> >
> > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap
> > and make -k check, both with and without --target_board=unix{-m32},
> > with no new failures. Ok for mainline?
> >
> >
> >
> > 2026-02-03 Roger Sayle <[email protected]>
> >
> > gcc/ChangeLog
> > PR rtl-optimization/123833
> > * rtlanal.cc (safe_insn_cost): Wrapper around insn_cost to save
> > and restore recog_data using recog_data_saver.
> > * rtl.h (safe_insn_cost): Prototype here.
> > * rtl-ssa/changes.cc (rtl_ssa:changes_are_worthwhile): Call
> > safe_insn_cost instead of insn_cost, while the insn is changing.
> > * rtl-ssa/insns.cc (insn_info::calculate_cost): Likewise.
> >
> > gcc/testsuite
> > PR rtl-optimization/123833
> > * gcc.target/mips/pr123833.c: New test case.
> Part of me wants to know why calling recog after making changes is
> problematical. What I would expect is that we make some change in the
> RTL, force re-recognition then cost. The re-recognition ought to wipe
> and repopulate the cache.
>
> So it feels like something is amiss, somewhere and this is just working
> around that.
So looking into this further, I see we do the cost which means
populating the recog cache with the new instruction.
BUT then we reject the replacement.
```
propagating insn 19 into insn 29, replacing:
(set (reg:DI 207 [ _10 ])
(and:DI (reg:DI 198 [ _21 ])
(const_int 255 [0xff])))
successfully matched this instruction to *zero_extendqidi2:
(set (reg:DI 207 [ _10 ])
(zero_extend:DI (subreg:QI (reg:SI 203) 0)))
original cost = 4 (weighted: 2.000000), replacement cost = 4
(weighted: 2.000000); rejecting replacement
change not profitable
```
And the recog's cache is still the replacement version. Rather than
the original version.
So maybe cancel_changes should invalidate the cache. Just in case the
cache included changes that have now been canceled out.
>
> jeff
