feedable <[email protected]> writes:
> On 09/09/2026 21:02, Richard Sandiford 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
> Irrespective of the `auto_vec` state, I think this is a positive change.
> However, do note that at least aarch64 also does the same trick with
> copying `recog_data` in its early-ra, and other backends may also do
> that.
Are you sure about early-ra? It swaps operands around, but it doesn't
seem to do a full copy AFAICT. At least, the change below didn't seem
to trip over it.
> If we are to proceed with this change, I would advise deleting
> copy&move construction and assignment for `recog_data_d` to explicitly
> fail compilation on the uses that remain. IIRC there should be a couple
> more copies in IRA&LRA, so this should reveal those too.
Ah yeah, good point. Doing that shows that I missed the case in
reload.cc.
Before posting the patch, I did try grepping for recog_data_d, for
assignments to recog_data (in case of auto), and various other things,
but I couldn't see anything in other backends. I suppose I should do
a "build all targets" to check though. (I missed the reload.cc one
because I'd misread it as being part of the recog* stuff.)
Deleting the constructors means that I need to do something about:
memset (&recog_data, 0, sizeof (recog_data));
in varasm.cc:assemble_asm (added by r15-5932-gca4d6285974817).
TBH I'm not sure why that's necessary. Maybe it was just included
to be conservative? I see that in your series you dropped the
memsetting here, but I suppose your series also adds more runtime
checking thanks to using vec operator[] over plain arrays.
Thanks,
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;
>>
>> /* 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
