Richard Biener <[email protected]> writes:
> On Wed, Sep 9, 2026 at 8:02 PM Richard Sandiford
> <[email protected]> wrote:
>> diff --git a/gcc/config/i386/i386-expand.cc b/gcc/config/i386/i386-expand.cc
>> index 7fa9d7f6b8d..ad44dd7df6f 100644
>> --- a/gcc/config/i386/i386-expand.cc
>> +++ b/gcc/config/i386/i386-expand.cc
>> @@ -17825,7 +17825,7 @@ ix86_vector_duplicate_value (machine_mode mode, rtx
>> target, rtx val)
>> /* Save/restore recog_data in case this is called from splitters
>> or other routines where recog_data needs to stay valid across
>> force_reg. See PR106577. */
>> - recog_data_d recog_data_save = recog_data;
>> + recog_data_saver recog_save;
>
> So this captures the save/restore with something very similar. You
> say there's more
> "recog state", so maybe it should be named recog_state_saver.
Ah, yeah, that sounds better.
> That said, I'd then try to put all of the recog state into a singleton class,
> macros can then get the appropriate members. It does look like this
> could be an incremental
> improvement ontop of this.
So that which_alternative becomes:
#define which_alternative recog_data.which_alternative
? Yeah, that might be worth doing. But one thing I've been meaning
to try is to make insns cache the alternative as well as the INSN_CODE.
On LP64 hosts, INSN_CODE is a 32-bit field in a 64-bit hole, so there's
already room. If we do that, it should be possible to get rid of
which_alternative altogether, and hopefully then also avoid some
extract_insn calls.
> Overall I like it. Do we expect any possible issues by
> zero-initializing the new
> recog data?
We're still using a standard default constructor, so the temporary
recog_data_d isn't initialised. It would be zero-initialised if we
added = {} or included it in the initialiser list, but I don't think
that should be necessary.
Tested on x86_64-linux-gnu and by building one target per CPU directory
(with a cut-down version of config-list.mk). OK to install?
Thanks,
Richard
Changes in v2:
- Rename recog_data_saver to recog_state_saver
- Delete recog_data_d move and copy constructors
- Deal with the fallout from that in varasm.cc
-- 8< --
recog_data is a global variable that is filled in by extract_insn.
The scope of a particular extract_insn call is not well-contained,
so recog_data has to be treated as cooperatively managed state.
This is particularly awkward because innocuous-looking things
like insn_invalid_p and get_attr_* use extract_insn under the hood.
Call chains can therefore write to recog_data as a "hidden" side-effect
when the caller isn't expecting recog_data to change.
The way we've traditionally coped with this is by saving and
restoring recog_data wherever the problem has been observed.
There is now an RAII class (recog_data_saver) to do this,
but reload, i386, and sh also save and restore manually.
However, recog_data_d is a big structure, so these saves and restores
are not cheap. This would become even more of a problem with an
unconstrained number of operands, as needed by the wasm backend.
This patch therefore turns recog_data into a macro that internally
dereferences a pointer. Saving and restoring recog_data is then
simply a matter of swapping pointers in and out.
One i386 function saved and restored which_alternative as well as
recog_data. I think recog_data_saver should do the same, since the
two pieces of state are closely related.
As Richard says, if recog_data_saver saves more than recog_data,
a better name for it would be "recog_state_saver". That didn't seem
worth splitting out as a separate patch since there's only one
existing user (in emit-rtl.cc).
The patch also converts reload, i386, and sh to use recog_state_saver
rather than manual code.
As feedable suggested, I've also deleted the copy and move constructors
for recog_data_d. This required removing a full memset in
varasm.cc:assemble_asm, to satisfy -Wclass-memaccess. As far as I can
tell, only n_dups, n_alternatives (sometimes), and is_operator were left
to zero initialisation. The patch initialises them explicitly instead.
I realise this isn't going to win any elegance or design awards.
But it seems like an improvement over the status quo and sidesteps
at least one need for auto_vec to be copyable.
It might make sense for insn_invalid_p to save and restore recog_data
itself, but that seems like a separate decision and patch.
gcc/
* recog.h (recog_data_d): Delete copy and move constructors.
(recog_data_ptr): New variable.
(recog_data): Turn into a macro.
(recog_data_saver): Rename to...
(recog_state_saver): ...this and move into the !GENERATOR_FILE block.
Save and restore which_alternative. Save and restore recog_data_ptr
rather than doing a full recog_data_d copy.
* varasm.cc (assemble_asm): Explicitly initialize n_dups,
n_alternatives, and is_operator.
* recog.cc (recog_data): Replace with...
(main_recog_data, recog_data_ptr): ...these new variables.
* emit-rtl.cc (address_reload_context::emit_autoinc): Update
after the rename.
* reload.cc (can_reload_into): Use recog_state_saver rather than
manually saving and restoring recog_data.
* config/i386/i386.cc (ix86_memory_address_reg_class): Likewise.
(ix86_lea_outperforms): Likewise.
* config/i386/i386-expand.cc (ix86_vector_duplicate_value): Likewise.
* config/sh/sh.cc (sh_recog_treg_set_expr): Likewise.
(sh_split_treg_set_expr): Likewise.
* config/sh/sh.md: Likewise in two peephole2s.
---
gcc/config/i386/i386-expand.cc | 3 +-
gcc/config/i386/i386.cc | 22 ++++-------
gcc/config/sh/sh.cc | 72 +++++++++++++++++-----------------
gcc/config/sh/sh.md | 22 +++++------
gcc/emit-rtl.cc | 2 +-
gcc/recog.cc | 3 +-
gcc/recog.h | 33 +++++++++++++---
gcc/reload.cc | 4 +-
gcc/varasm.cc | 10 +++--
9 files changed, 92 insertions(+), 79 deletions(-)
diff --git a/gcc/config/i386/i386-expand.cc b/gcc/config/i386/i386-expand.cc
index 7fa9d7f6b8d..b9c7982624f 100644
--- a/gcc/config/i386/i386-expand.cc
+++ b/gcc/config/i386/i386-expand.cc
@@ -17825,7 +17825,7 @@ ix86_vector_duplicate_value (machine_mode mode, rtx
target, rtx val)
/* Save/restore recog_data in case this is called from splitters
or other routines where recog_data needs to stay valid across
force_reg. See PR106577. */
- recog_data_d recog_data_save = recog_data;
+ recog_state_saver recog_save;
/* First attempt to recognize VAL as-is. */
dup = gen_vec_duplicate (mode, val);
@@ -17859,7 +17859,6 @@ ix86_vector_duplicate_value (machine_mode mode, rtx
target, rtx val)
ok = recog_memoized (insn) >= 0;
gcc_assert (ok);
}
- recog_data = recog_data_save;
return true;
}
diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
index 3f00ddaa547..a8edf28f687 100644
--- a/gcc/config/i386/i386.cc
+++ b/gcc/config/i386/i386.cc
@@ -12092,15 +12092,14 @@ ix86_memory_address_reg_class (rtx_insn* insn)
/* Try to recognize the insn before calling get_attr_addr.
Save current recog_data and current alternative. */
- struct recog_data_d saved_recog_data = recog_data;
- int saved_alternative = which_alternative;
+ recog_state_saver recog_save;
/* Update recog_data for processing of alternatives. */
extract_insn_cached (insn);
/* If current alternative is not set, loop through enabled
alternatives and get the most limited register class. */
- if (saved_alternative == -1)
+ if (recog_save.saved_alternative == -1)
{
alternative_mask enabled = get_enabled_alternatives (insn);
@@ -12115,13 +12114,10 @@ ix86_memory_address_reg_class (rtx_insn* insn)
}
else
{
- which_alternative = saved_alternative;
+ which_alternative = recog_save.saved_alternative;
addr_rclass = get_attr_addr (insn);
}
- recog_data = saved_recog_data;
- which_alternative = saved_alternative;
-
return addr_rclass;
}
@@ -16517,15 +16513,13 @@ ix86_lea_outperforms (rtx_insn *insn, unsigned int
regno0, unsigned int regno1,
return true;
}
- /* Remember recog_data content. */
- struct recog_data_d recog_data_save = recog_data;
-
- dist_define = distance_non_agu_define (regno1, regno2, insn);
- dist_use = distance_agu_use (regno0, insn);
-
/* distance_non_agu_define can call get_attr_type which can call
recog_memoized, restore recog_data back to previous content. */
- recog_data = recog_data_save;
+ {
+ recog_state_saver recog_save;
+ dist_define = distance_non_agu_define (regno1, regno2, insn);
+ dist_use = distance_agu_use (regno0, insn);
+ }
if (dist_define < 0 || dist_define >= LEA_MAX_STALL)
{
diff --git a/gcc/config/sh/sh.cc b/gcc/config/sh/sh.cc
index 880b0b9239f..71342153874 100644
--- a/gcc/config/sh/sh.cc
+++ b/gcc/config/sh/sh.cc
@@ -12257,7 +12257,7 @@ sh_recog_treg_set_expr (rtx op, machine_mode mode)
/* We are going to invoke recog in a re-entrant way and thus
have to capture its current state and restore it afterwards. */
- recog_data_d prev_recog_data = recog_data;
+ recog_state_saver recog_save;
/* Note we can't use insn_raw here since that increases the uid
and could cause debug compare differences; this insn never leaves
@@ -12297,7 +12297,6 @@ sh_recog_treg_set_expr (rtx op, machine_mode mode)
}
PUT_MODE (op, prev_op_mode);
- recog_data = prev_recog_data;
return result >= 0;
}
@@ -12418,47 +12417,46 @@ sh_split_treg_set_expr (rtx x, rtx_insn* curr_insn)
a nott. */
bool append_nott = false;
- /* We are going to invoke recog/split_insns in a re-entrant way and thus
- have to capture its current state and restore it afterwards. */
- recog_data_d prev_recog_data = recog_data;
-
- if (negt_reg_operand (x, GET_MODE (x)))
- {
- /* This is a normal movt followed by a nott. It will be converted
- into a movrt after initial expansion. */
- XEXP (PATTERN (i), 1) = get_t_reg_rtx ();
- append_nott = true;
- }
- else
- {
- /* If the comparison op doesn't have a mode set, set it to SImode. */
- if (COMPARISON_P (x) && GET_MODE (x) == VOIDmode)
- PUT_MODE (x, SImode);
+ std::pair<rtx_insn*, rtx_insn*> insnlist;
+ {
+ /* We are going to invoke recog/split_insns in a re-entrant way and thus
+ have to capture its current state and restore it afterwards. */
+ recog_state_saver recog_save;
- int insn_code = recog (PATTERN (i), i, 0);
+ if (negt_reg_operand (x, GET_MODE (x)))
+ {
+ /* This is a normal movt followed by a nott. It will be converted
+ into a movrt after initial expansion. */
+ XEXP (PATTERN (i), 1) = get_t_reg_rtx ();
+ append_nott = true;
+ }
+ else
+ {
+ /* If the comparison op doesn't have a mode set, set it to SImode. */
+ if (COMPARISON_P (x) && GET_MODE (x) == VOIDmode)
+ PUT_MODE (x, SImode);
- if (insn_code < 0 && COMPARISON_P (x))
- {
- machine_mode cmp_mode = GET_MODE (XEXP (x, 0));
- if (cmp_mode == VOIDmode)
- cmp_mode = GET_MODE (XEXP (x, 1));
+ int insn_code = recog (PATTERN (i), i, 0);
- PUT_CODE (x, reverse_condition (GET_CODE (x)));
- insn_code = recog (PATTERN (i), i, 0);
- append_nott = true;
- }
+ if (insn_code < 0 && COMPARISON_P (x))
+ {
+ machine_mode cmp_mode = GET_MODE (XEXP (x, 0));
+ if (cmp_mode == VOIDmode)
+ cmp_mode = GET_MODE (XEXP (x, 1));
- gcc_assert (insn_code >= 0);
- }
+ PUT_CODE (x, reverse_condition (GET_CODE (x)));
+ insn_code = recog (PATTERN (i), i, 0);
+ append_nott = true;
+ }
- /* Try to recursively split the insn. Some insns might refuse to split
- any further while we are in the treg_set_expr splitting phase. They
- will be emitted as part of the outer insn and then split again. */
- std::pair<rtx_insn*, rtx_insn*> insnlist =
- sh_try_split_insn_simple (i, curr_insn);
+ gcc_assert (insn_code >= 0);
+ }
- /* Restore recog state. */
- recog_data = prev_recog_data;
+ /* Try to recursively split the insn. Some insns might refuse to split
+ any further while we are in the treg_set_expr splitting phase. They
+ will be emitted as part of the outer insn and then split again. */
+ insnlist = sh_try_split_insn_simple (i, curr_insn);
+ }
rtx_insn* nott_insn = sh_is_nott_insn (insnlist.second)
? insnlist.second
diff --git a/gcc/config/sh/sh.md b/gcc/config/sh/sh.md
index 18744d3a75d..85980f31953 100644
--- a/gcc/config/sh/sh.md
+++ b/gcc/config/sh/sh.md
@@ -11227,12 +11227,11 @@
rtx_insn* i = emit_insn (gen_rtx_SET (operands[2],
sh_remove_overlapping_post_inc (operands[2], operands[3])));
- recog_data_d prev_recog_data = recog_data;
- bool i_invalid = insn_invalid_p (i, false);
- recog_data = prev_recog_data;
-
- if (i_invalid)
- FAIL;
+ {
+ recog_state_saver recog_save;
+ if (insn_invalid_p (i, false))
+ FAIL;
+ }
sh_check_add_incdec_notes (i);
@@ -11267,12 +11266,11 @@
rtx_insn* i = emit_insn (gen_rtx_SET (operands[2],
sh_remove_overlapping_post_inc (operands[2], operands[3])));
- recog_data_d prev_recog_data = recog_data;
- bool i_invalid = insn_invalid_p (i, false);
- recog_data = prev_recog_data;
-
- if (i_invalid)
- FAIL;
+ {
+ recog_state_saver recog_save;
+ if (insn_invalid_p (i, false))
+ FAIL;
+ }
sh_check_add_incdec_notes (i);
diff --git a/gcc/emit-rtl.cc b/gcc/emit-rtl.cc
index 4a23eaefe02..01770e5e0c7 100644
--- a/gcc/emit-rtl.cc
+++ b/gcc/emit-rtl.cc
@@ -2606,7 +2606,7 @@ address_reload_context::emit_autoinc (rtx value,
poly_int64 inc_amount)
{
/* Since we're going to call recog, and might be called within recog,
we need to ensure we save and restore recog_data. */
- recog_data_saver recog_save;
+ recog_state_saver recog_save;
/* REG or MEM to be copied and incremented. */
rtx incloc = XEXP (value, 0);
diff --git a/gcc/recog.cc b/gcc/recog.cc
index f9cff68e457..dfce5df743d 100644
--- a/gcc/recog.cc
+++ b/gcc/recog.cc
@@ -71,7 +71,8 @@ struct target_recog *this_target_recog =
&default_target_recog;
int volatile_ok;
-struct recog_data_d recog_data;
+static struct recog_data_d main_recog_data;
+struct recog_data_d *recog_data_ptr = &main_recog_data;
/* Contains a vector of operand_alternative structures, such that
operand OP of alternative A is at index A * n_operands + OP.
diff --git a/gcc/recog.h b/gcc/recog.h
index d7c816a6097..11b5e2b6f1b 100644
--- a/gcc/recog.h
+++ b/gcc/recog.h
@@ -385,6 +385,10 @@ extern bool raw_constraint_p;
struct recog_data_d
{
+ recog_data_d () = default;
+ recog_data_d (const recog_data_d &) = delete;
+ recog_data_d (const recog_data_d &&) = delete;
+
/* It is very tempting to make the 5 operand related arrays into a
structure and index on that. However, to be source compatible
with all of the existing md file insn constraints and output
@@ -443,19 +447,36 @@ struct recog_data_d
rtx_insn *insn;
};
-extern struct recog_data_d recog_data;
+extern struct recog_data_d *recog_data_ptr;
+#define recog_data (*recog_data_ptr)
+#ifndef GENERATOR_FILE
/* RAII class for saving/restoring recog_data. */
-class recog_data_saver
+class recog_state_saver
{
- recog_data_d m_saved_data;
+ recog_data_d m_tmp_recog_data;
public:
- recog_data_saver () : m_saved_data (recog_data) {}
- ~recog_data_saver () { recog_data = m_saved_data; }
+ recog_state_saver ();
+ ~recog_state_saver ();
+
+ recog_data_d *saved_recog_data_ptr;
+ int saved_alternative;
};
-#ifndef GENERATOR_FILE
+inline recog_state_saver::recog_state_saver ()
+ : saved_recog_data_ptr (recog_data_ptr),
+ saved_alternative (which_alternative)
+{
+ recog_data_ptr = &m_tmp_recog_data;
+}
+
+inline recog_state_saver::~recog_state_saver ()
+{
+ recog_data_ptr = saved_recog_data_ptr;
+ which_alternative = saved_alternative;
+}
+
extern const operand_alternative *recog_op_alt;
/* Return a pointer to an array in which index OP describes the constraints
diff --git a/gcc/reload.cc b/gcc/reload.cc
index 7917cc94a9d..c6d99b1527f 100644
--- a/gcc/reload.cc
+++ b/gcc/reload.cc
@@ -884,7 +884,6 @@ can_reload_into (rtx in, int regno, machine_mode mode)
rtx dst;
rtx_insn *test_insn;
int r = 0;
- struct recog_data_d save_recog_data;
/* For matching constraints, we often get notional input reloads where
we want to use the original register as the reload register. I.e.
@@ -905,13 +904,12 @@ can_reload_into (rtx in, int regno, machine_mode mode)
be fine. */
dst = gen_rtx_REG (mode, regno);
test_insn = make_insn_raw (gen_rtx_SET (dst, in));
- save_recog_data = recog_data;
+ recog_state_saver recog_save;
if (recog_memoized (test_insn) >= 0)
{
extract_insn (test_insn);
r = constrain_operands (1, get_enabled_alternatives (test_insn));
}
- recog_data = save_recog_data;
return r;
}
diff --git a/gcc/varasm.cc b/gcc/varasm.cc
index 0dbc35d926a..ad39bdddcfc 100644
--- a/gcc/varasm.cc
+++ b/gcc/varasm.cc
@@ -1752,8 +1752,9 @@ assemble_asm (tree asm_str)
}
constraints = XALLOCAVEC (const char *, noutputs + ninputs);
ops = XALLOCAVEC (rtx, noutputs + ninputs);
- memset (&recog_data, 0, sizeof (recog_data));
recog_data.n_operands = ninputs + noutputs;
+ recog_data.n_dups = 0;
+ recog_data.n_alternatives = 0;
recog_data.is_asm = true;
reload_completed = 0;
cse_not_expected = 1;
@@ -1857,8 +1858,11 @@ assemble_asm (tree asm_str)
recog_data.n_alternatives += (*p++ == ',');
}
for (i = 0; i < recog_data.n_operands; i++)
- recog_data.operand_type[i]
- = recog_data.constraints[i][0] == '=' ? OP_OUT : OP_IN;
+ {
+ recog_data.operand_type[i]
+ = recog_data.constraints[i][0] == '=' ? OP_OUT : OP_IN;
+ recog_data.is_operator[i] = false;
+ }
reload_completed = 1;
constrain_operands (1, ALL_ALTERNATIVES);
if (which_alternative < 0)
--
2.55.0
