On Thu, Sep 10, 2026 at 12:45 PM Richard Sandiford
<[email protected]> wrote:
>
> 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?
OK.
Thanks,
Richard.
> 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
>
