On 2/3/2026 4:26 PM, Andrew Pinski wrote:
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.
That makes sense. We need to flush the cache if we cancel a replacement
as well since otherwise we leave the cache inconsistent with the state
of the IL.
Jeff
