On Thu, Sep 10, 2026 at 7:29 PM Richard Biener <[email protected]> wrote: > > 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(-) > >
This caused: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=127354 -- H.J.
