From 387dba05e4207fc3f9a2f2bcb09a343a999c76fc Mon Sep 17 00:00:00 2001 From: Richard Sandiford Date: Tue, 19 Nov 2024 10:19:24 +0000 Subject: [PATCH] Avoid repeated calls to temporarily_undo_changes [PR117297] In an attempt to reduce compile time, rtl-ssa computes the cost of existing instructions lazily rather than eagerly. However, this means that it might need to calculate the cost of an existing instruction while a change group is already in progress for the instruction. rtl_ssa::insn_info::calculate_cost therefore temporarily undoes any in-progress changes in order to get back the original pattern and insn code. rtl-ssa's main use of insn costs is in rtl_ssa::changes_are_worthwhile, which calculates the cost of a change involving an arbitrary number of instructions. Summing up the original cost of N instructions while those N instructions have in-progress changes could lead to O(N*N) rtl changes, since each lazy calculation might have to temporarily undo the changes to all N instructions. We can avoid that by converting the current temporarily_undo_changes/ redo_changes pair into an RAII class and extending it to allow nested uses. rtl_ssa::changes_are_worthwhile can then undo the in-progress changes once, before computing the original cost of all the instructions. gcc/ PR rtl-optimization/117297 * recog.h (temporarily_undo_changes, redo_changes): Delete in favor of... (undo_recog_changes): ...this new RAII class. * fwprop.cc (should_replace_address): Update accordingly. (fwprop_propagation::check_mem): Likewise. (try_fwprop_subst_note): Likewise. (try_fwprop_subst_pattern): Likewise. * rtl-ssa/insns.cc (insn_info::calculate_cost): Likewise. * rtl-ssa/changes.cc (rtl_ssa::changes_are_worthwhile): Temporarily undo all in-progress changes while computing the cost of the original sequence. * recog.cc (temporarily_undone_changes): Replace with... (undo_recog_changes::s_num_changes): ...this static member variable. (validate_change_1): Update check accordingly. (confirm_change_group): Likewise. (num_validated_changes): Likewise. (temporarily_undo_changes): Replace with... (undo_recog_changes::undo_recog_changes): ...this constructor. (redo_changes): Replace with... (undo_recog_changes::~undo_recog_changes): ...this destructor. --- gcc/fwprop.cc | 30 ++++++++++++++++-------------- gcc/recog.cc | 39 +++++++++++++-------------------------- gcc/recog.h | 31 +++++++++++++++++++++++++++++-- gcc/rtl-ssa/changes.cc | 26 +++++++++++++++++--------- gcc/rtl-ssa/insns.cc | 3 +-- 5 files changed, 76 insertions(+), 53 deletions(-) diff --git a/gcc/fwprop.cc b/gcc/fwprop.cc index 8cba6b7ce9f..5ddefdf6d2f 100644 --- a/gcc/fwprop.cc +++ b/gcc/fwprop.cc @@ -146,10 +146,11 @@ should_replace_address (int old_num_changes, rtx mem, rtx_insn *insn) /* Prefer the new address if it is less expensive. */ bool speed = optimize_bb_for_speed_p (BLOCK_FOR_INSN (insn)); - temporarily_undo_changes (old_num_changes); - gain = address_cost (XEXP (mem, 0), GET_MODE (mem), - MEM_ADDR_SPACE (mem), speed); - redo_changes (old_num_changes); + { + undo_recog_changes undo (old_num_changes); + gain = address_cost (XEXP (mem, 0), GET_MODE (mem), + MEM_ADDR_SPACE (mem), speed); + } gain -= address_cost (XEXP (mem, 0), GET_MODE (mem), MEM_ADDR_SPACE (mem), speed); @@ -160,9 +161,8 @@ should_replace_address (int old_num_changes, rtx mem, rtx_insn *insn) if (gain == 0) { gain = set_src_cost (XEXP (mem, 0), VOIDmode, speed); - temporarily_undo_changes (old_num_changes); + undo_recog_changes undo (old_num_changes); gain -= set_src_cost (XEXP (mem, 0), VOIDmode, speed); - redo_changes (old_num_changes); } return (gain > 0); @@ -220,9 +220,11 @@ fwprop_propagation::check_mem (int old_num_changes, rtx mem) return false; } - temporarily_undo_changes (old_num_changes); - bool can_simplify = can_simplify_addr (XEXP (mem, 0)); - redo_changes (old_num_changes); + bool can_simplify = [&]() + { + undo_recog_changes undo (old_num_changes); + return can_simplify_addr (XEXP (mem, 0)); + } (); if (!can_simplify) { failure_reason = "would replace a frame address"; @@ -414,9 +416,10 @@ try_fwprop_subst_note (insn_info *use_insn, set_info *def, { fprintf (dump_file, "\nin notes of insn %d, replacing:\n ", INSN_UID (use_rtl)); - temporarily_undo_changes (0); - print_inline_rtx (dump_file, note, 2); - redo_changes (0); + { + undo_recog_changes undo (0); + print_inline_rtx (dump_file, note, 2); + } fprintf (dump_file, "\n with:\n "); print_inline_rtx (dump_file, note, 2); fprintf (dump_file, "\n"); @@ -468,9 +471,8 @@ try_fwprop_subst_pattern (obstack_watermark &attempt, insn_change &use_change, { fprintf (dump_file, "\npropagating insn %d into insn %d, replacing:\n", def_insn->uid (), use_insn->uid ()); - temporarily_undo_changes (0); + undo_recog_changes undo (0); print_rtl_single (dump_file, PATTERN (use_rtl)); - redo_changes (0); } bool ok = recog (attempt, use_change); diff --git a/gcc/recog.cc b/gcc/recog.cc index 4c63f9301c6..d1811f3618d 100644 --- a/gcc/recog.cc +++ b/gcc/recog.cc @@ -194,7 +194,7 @@ static change_t *changes; static int changes_allocated; static int num_changes = 0; -static int temporarily_undone_changes = 0; +int undo_recog_changes::s_num_changes = 0; /* Validate a proposed change to OBJECT. LOC is the location in the rtl at which NEW_RTX will be placed. If NEW_LEN is >= 0, XVECLEN (NEW_RTX, 0) @@ -220,7 +220,7 @@ static bool validate_change_1 (rtx object, rtx *loc, rtx new_rtx, bool in_group, bool unshare, int new_len = -1) { - gcc_assert (temporarily_undone_changes == 0); + gcc_assert (!undo_recog_changes::is_active ()); rtx old = *loc; /* Single-element parallels aren't valid and won't match anything. @@ -536,7 +536,7 @@ confirm_change_group (void) int i; rtx last_object = NULL; - gcc_assert (temporarily_undone_changes == 0); + gcc_assert (!undo_recog_changes::is_active ()); for (i = 0; i < num_changes; i++) { rtx object = changes[i].object; @@ -592,7 +592,7 @@ num_validated_changes (void) void cancel_changes (int num) { - gcc_assert (temporarily_undone_changes == 0); + gcc_assert (!undo_recog_changes::is_active ()); int i; /* Back out all the changes. Do this in the opposite order in which @@ -627,34 +627,21 @@ swap_change (int num) } } -/* Temporarily undo all the changes numbered NUM and up, with a view - to reapplying them later. The next call to the changes machinery - must be: - - redo_changes (NUM) - - otherwise things will end up in an invalid state. */ - -void -temporarily_undo_changes (int num) +undo_recog_changes::undo_recog_changes (int num) + : m_old_num_changes (s_num_changes) { - gcc_assert (temporarily_undone_changes == 0 && num <= num_changes); - for (int i = num_changes - 1; i >= num; i--) + gcc_assert (num <= num_changes - s_num_changes); + for (int i = num_changes - s_num_changes - 1; i >= num; i--) swap_change (i); - temporarily_undone_changes = num_changes - num; + s_num_changes = num_changes - num; } -/* Redo the changes that were temporarily undone by: - - temporarily_undo_changes (NUM). */ - -void -redo_changes (int num) +undo_recog_changes::~undo_recog_changes () { - gcc_assert (temporarily_undone_changes == num_changes - num); - for (int i = num; i < num_changes; ++i) + for (int i = num_changes - s_num_changes; + i < num_changes - m_old_num_changes; ++i) swap_change (i); - temporarily_undone_changes = 0; + s_num_changes = m_old_num_changes; } /* Reduce conditional compilation elsewhere. */ diff --git a/gcc/recog.h b/gcc/recog.h index 1dccce78ba4..17ccdb6296b 100644 --- a/gcc/recog.h +++ b/gcc/recog.h @@ -202,6 +202,35 @@ inline insn_propagation::insn_propagation (rtx_insn *insn) { } +/* An RAII class that temporarily undoes part of the current change group. + The sequence: + + { + ...; + undo_recog_changes undo (NUM); + STMTS; + } + + executes STMTS with all the changes numbered NUM and up temporarily + reverted. STMTS must not try to add to the current change group. + + Nested uses are supported, but each nested NUM must be no greater than + outer NUMs. */ + +class undo_recog_changes +{ +public: + undo_recog_changes (int); + ~undo_recog_changes (); + + static bool is_active () { return s_num_changes != 0; } + +private: + int m_old_num_changes; + + static int s_num_changes; +}; + extern void init_recog (void); extern void init_recog_no_volatile (void); extern bool check_asm_operands (rtx); @@ -216,8 +245,6 @@ extern void confirm_change_group (void); extern bool apply_change_group (void); extern int num_validated_changes (void); extern void cancel_changes (int); -extern void temporarily_undo_changes (int); -extern void redo_changes (int); extern bool constrain_operands (int, alternative_mask); extern bool constrain_operands_cached (rtx_insn *, int); extern bool memory_address_addr_space_p (machine_mode, rtx, addr_space_t, diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc index f2be1195bc0..5f3a0f0f0ec 100644 --- a/gcc/rtl-ssa/changes.cc +++ b/gcc/rtl-ssa/changes.cc @@ -173,21 +173,29 @@ rtl_ssa::changes_are_worthwhile (array_slice changes, bool strict_p) { unsigned int old_cost = 0; - unsigned int new_cost = 0; sreal weighted_old_cost = 0; - sreal weighted_new_cost = 0; auto entry_count = ENTRY_BLOCK_PTR_FOR_FN (cfun)->count; + { + undo_recog_changes undo (0); + for (insn_change *change : changes) + { + // Count zero for the old cost if the old instruction was a no-op + // move or had an unknown cost. This should reduce the chances of + // making an unprofitable change. + old_cost += change->old_cost (); + basic_block cfg_bb = change->bb ()->cfg_bb (); + if (optimize_bb_for_speed_p (cfg_bb)) + weighted_old_cost += (cfg_bb->count.to_sreal_scale (entry_count) + * change->old_cost ()); + + } + } + unsigned int new_cost = 0; + sreal weighted_new_cost = 0; for (insn_change *change : changes) { - // Count zero for the old cost if the old instruction was a no-op - // move or had an unknown cost. This should reduce the chances of - // making an unprofitable change. - old_cost += change->old_cost (); basic_block cfg_bb = change->bb ()->cfg_bb (); bool for_speed = optimize_bb_for_speed_p (cfg_bb); - if (for_speed) - weighted_old_cost += (cfg_bb->count.to_sreal_scale (entry_count) - * change->old_cost ()); if (!change->is_deletion () && INSN_CODE (change->rtl ()) != NOOP_MOVE_INSN_CODE) { diff --git a/gcc/rtl-ssa/insns.cc b/gcc/rtl-ssa/insns.cc index bfc6683cc45..4af79bee7b9 100644 --- a/gcc/rtl-ssa/insns.cc +++ b/gcc/rtl-ssa/insns.cc @@ -49,14 +49,13 @@ void insn_info::calculate_cost () const { basic_block cfg_bb = BLOCK_FOR_INSN (m_rtl); - temporarily_undo_changes (0); + undo_recog_changes (0); if (INSN_CODE (m_rtl) == NOOP_MOVE_INSN_CODE) // insn_cost also uses 0 to mean "don't know". Callers that // want to distinguish the cases will need to check INSN_CODE. m_cost_or_uid = 0; else m_cost_or_uid = insn_cost (m_rtl, optimize_bb_for_speed_p (cfg_bb)); - redo_changes (0); } // Add NOTE to the instruction's notes.