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.
Roger
--
diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
index e0df982880c..e13980fbbeb 100644
--- a/gcc/rtl-ssa/changes.cc
+++ b/gcc/rtl-ssa/changes.cc
@@ -198,7 +198,7 @@ rtl_ssa::changes_are_worthwhile (array_slice<insn_change
*const> changes,
if (!change->is_deletion ()
&& INSN_CODE (change->rtl ()) != NOOP_MOVE_INSN_CODE)
{
- change->new_cost = insn_cost (change->rtl (), for_speed);
+ change->new_cost = safe_insn_cost (change->rtl (), for_speed);
/* If the cost is unknown, replacement is not worthwhile. */
if (!change->new_cost)
{
diff --git a/gcc/rtl-ssa/insns.cc b/gcc/rtl-ssa/insns.cc
index 317a5809b18..2d3852a2b6c 100644
--- a/gcc/rtl-ssa/insns.cc
+++ b/gcc/rtl-ssa/insns.cc
@@ -54,7 +54,7 @@ insn_info::calculate_cost () const
// want to distinguish the cases will need to check INSN_CODE.
m_cost_or_uid = 0;
else
- m_cost_or_uid = insn_cost (m_rtl, optimize_bb_for_speed_p (cfg_bb));
+ m_cost_or_uid = safe_insn_cost (m_rtl, optimize_bb_for_speed_p (cfg_bb));
}
// Add NOTE to the instruction's notes.
diff --git a/gcc/rtl.h b/gcc/rtl.h
index eebcc18a4f1..ff5312da267 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -3794,6 +3794,7 @@ extern bool keep_with_call_p (const rtx_insn *);
extern bool label_is_jump_target_p (const_rtx, const rtx_insn *);
extern int pattern_cost (rtx, bool);
extern int insn_cost (rtx_insn *, bool);
+extern int safe_insn_cost (rtx_insn *, bool);
extern unsigned seq_cost (const rtx_insn *, bool);
/* Given an insn and condition, return a canonical description of
diff --git a/gcc/rtlanal.cc b/gcc/rtlanal.cc
index 27349a0a74f..d33fb8beed9 100644
--- a/gcc/rtlanal.cc
+++ b/gcc/rtlanal.cc
@@ -5812,6 +5812,17 @@ insn_cost (rtx_insn *insn, bool speed)
return pattern_cost (PATTERN (insn), speed);
}
+/* Variant of insn_cost that avoids complications with recog_data
+ caching. */
+
+int
+safe_insn_cost (rtx_insn *insn, bool speed)
+{
+ recog_data_saver recog_save;
+ recog_data.insn = nullptr;
+ return insn_cost (insn, speed);
+}
+
/* Returns estimate on cost of computing SEQ. */
unsigned
diff --git a/gcc/testsuite/gcc.target/mips/pr123833.c
b/gcc/testsuite/gcc.target/mips/pr123833.c
new file mode 100644
index 00000000000..4d0c9e8eaec
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/pr123833.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-O1 -mabi=64 -mhard-float" } */
+
+typedef short int16_t;
+typedef signed char int8_t;
+typedef long long int64_t;
+typedef unsigned char uint8_t;
+typedef unsigned long long uint64_t;
+
+#define BS_VEC(type, num) type __attribute__((vector_size(num * sizeof(type))))
+BS_VEC(int8_t, 2) backsmith_pure_0(int64_t);
+uint64_t backsmith_pure_4(int16_t BS_ARG_0)
+{
+ BS_VEC(int16_t, 16)
+ BS_VAR_0 = __builtin_convertvector(
+ __builtin_shufflevector(backsmith_pure_0(0), backsmith_pure_0(0), 0, 2,
+ 2, 3, 2, 1, 2, 3, 0, 2, 0, 0, 2, 2, 0, 1),
+ BS_VEC(int16_t, 16));
+ for (;;)
+ {
+ if (__builtin_ctzg((uint8_t)BS_VAR_0[3], BS_ARG_0))
+ for (;;);
+ return 0;
+ }
+}
+
