On Wed, Sep 9, 2026 at 8:02 PM Richard Sandiford
<[email protected]> wrote:
>
> Richard Sandiford <[email protected]> writes:
> >> The argument is that once you expose an inefficient API you'll
> >> get lazy uses and inefficiencies that could have been easily avoided.
> >> But I might be overly pessimistic here, not knowing how often the
> >> copy/move construction/assignment ops for auto_vec would occur.
> >>
> >> There is also the semantic difference with vec<> where
> >> assignment does a shallow copy of the m_vec pointer and _not_
> >> copy the data as with your non-move assignment/ctor. So can
> >> we at least avoid that? vec<> has vec<>::copy for this and
> >> truncate() + splice() for the assignment.
> >>
> >> We should possibly delete vec<>::operator=() and adjust code
> >> to use const vec<>& argument passing (it's one more indirection
> >> sadly). Adding move assignment might be useful, but I'd like
> >> to explicitly "see" it, like if we can force v1 = std::move (v2); or so?
> >
> > AIUI, the use case for this change is to allow copying of recog_data.
> > But IMO that isn't a good motivation. I mentioned in a previous
> > review that:
> >
> > I suppose the underlying problem is that recog_data shouldn't be a
> > global variable. Copying and restoring it is a hack to get around
> > that design decision. And that hack was borderline acceptable while
> > the copy was simple.
> >
> > IMO the ideal fix would be to remove the global state and handle the
> > cache in a different way. But that would be way too invasive. Even
> > changing recog_data to a pointer would probably be too much churn
> > (although I wouldn't be against it personally).
> >
> > So perhaps we should adopt a hack used elsewhere and make recog_data
> > a macro that [deferences a] pointer (either global, or in crtl or
> > something). By default the pointer points at a global recog_data_d,
> > but backends can use an RAII class or direct pointer assignment
> > to temporarily switch it to a temporary recog_data_d instead.
> >
> > It's all a bit icky, but maybe more acceptable than copying auto_vecs?
>
> FWIW, this is what I had in mind. Bootstrapped & regression-tested on
> x86_64-linux-gnu. Also tested by cross-building sh-linux-gnu. WDYT?
>
> Richard
>
>
> 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 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 case of swapping pointers in and out.
>
> The patch also converts i386 and sh to use recog_data_saver rather
> than manual code.
>
> 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.
>
> 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_ptr): New variable.
> (recog_data): Turn into a macro.
> (recog_data_saver): 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.
> * recog.cc (recog_data): Replace with...
> (main_recog_data, recog_data_ptr): ...these new variables.
> * config/i386/i386.cc (ix86_memory_address_reg_class): Use
> recog_data_saver rather than manually saving and restoring
> recog_data.
> (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/recog.cc | 3 +-
> gcc/recog.h | 27 ++++++++++---
> 6 files changed, 78 insertions(+), 71 deletions(-)
>
> 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.
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.
Overall I like it. Do we expect any possible issues by
zero-initializing the new
recog data?
>
> /* 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..c560520d84c 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_data_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_data_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..13b3a89c103 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_data_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_data_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..8fcfe1c3d18 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_data_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_data_saver recog_save;
> + if (insn_invalid_p (i, false))
> + FAIL;
> + }
>
> sh_check_add_incdec_notes (i);
>
> 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..b2b1f449ac8 100644
> --- a/gcc/recog.h
> +++ b/gcc/recog.h
> @@ -443,19 +443,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
> {
> - 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_data_saver ();
> + ~recog_data_saver ();
> +
> + recog_data_d *saved_recog_data_ptr;
> + int saved_alternative;
> };
>
> -#ifndef GENERATOR_FILE
> +inline recog_data_saver::recog_data_saver ()
> + : saved_recog_data_ptr (recog_data_ptr),
> + saved_alternative (which_alternative)
> +{
> + recog_data_ptr = &m_tmp_recog_data;
> +}
> +
> +inline recog_data_saver::~recog_data_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
> --
> 2.55.0
>
