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;
/* 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
