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. 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.

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

Reply via email to