https://gcc.gnu.org/g:b1215a5b1e6948733f46ecb4d3918083a1d298f8
commit r17-4078-gb1215a5b1e6948733f46ecb4d3918083a1d298f8 Author: Richard Sandiford <[email protected]> Date: Thu Sep 10 13:08:15 2026 +0100 Avoid copying recog_data_ds 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. Diff: --- 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 1b76fe91ca21..68c6791c9928 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 3f00ddaa5479..a8edf28f687f 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 880b0b9239f2..713421538746 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 18744d3a75d1..85980f31953b 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 4a23eaefe024..01770e5e0c78 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 f9cff68e4573..dfce5df743d0 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 34c7d17d9020..4d94fc26d94c 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 7917cc94a9d0..c6d99b1527fb 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 0dbc35d926a3..ad39bdddcfc4 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)
