| Message ID | ZVZaIkSAtZnKzu/k@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64: Rework ldp/stp patterns, add new ldp/stp pass | expand |
Alex Coplan <alex.coplan@arm.com> writes: > N.B. this is just a rebased (but otherwise unchanged) version of the > same patch already posted here: > > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633348.html > > this is the only unreviewed dependency from the previous series, so it > seemed easier just to re-post it (not least to appease the pre-commit > CI). > > -- >8 -- > > The upcoming aarch64 load pair pass needs to form store pairs, and can > re-order stores over loads when alias analysis determines this is safe. > In the case that both mem defs have uses in the RTL-SSA IR, and both > stores require re-ordering over their uses, we represent that as > (tentative) deletion of the original store insns and creation of a new > insn, to prevent requiring repeated re-parenting of uses during the > pass. We then update all mem uses that require re-parenting in one go > at the end of the pass. > > To support this, RTL-SSA needs to handle inserting new insns (rather > than just changing existing ones), so this patch adds support for that. > > New insns (and new accesses) are temporaries, allocated above a temporary > obstack_watermark, such that the user can easily back out of a change without > awkward bookkeeping. > > Bootstrapped/regtested as a series on aarch64-linux-gnu, OK for trunk? > > gcc/ChangeLog: > > * rtl-ssa/accesses.cc (function_info::create_set): New. > * rtl-ssa/accesses.h (access_info::is_temporary): New. > * rtl-ssa/changes.cc (move_insn): Handle new (temporary) insns. > (function_info::finalize_new_accesses): Handle new/temporary > user-created accesses. > (function_info::apply_changes_to_insn): Ensure m_is_temp flag > on new insns gets cleared. > (function_info::change_insns): Handle new/temporary insns. > (function_info::create_insn): New. > * rtl-ssa/changes.h (class insn_change): Make function_info a > friend class. > * rtl-ssa/functions.h (function_info): Declare new entry points: > create_set, create_insn. Declare new change_alloc helper. > * rtl-ssa/insns.cc (insn_info::print_full): Identify temporary insns in > dump. > * rtl-ssa/insns.h (insn_info): Add new m_is_temp flag and accompanying > is_temporary accessor. > * rtl-ssa/internals.inl (insn_info::insn_info): Initialize m_is_temp to > false. > * rtl-ssa/member-fns.inl (function_info::change_alloc): New. > * rtl-ssa/movement.h (restrict_movement_for_defs_ignoring): Add > handling for temporary defs. Looks good, but there were a couple of things I didn't understand: > --- > gcc/rtl-ssa/accesses.cc | 10 ++++++ > gcc/rtl-ssa/accesses.h | 4 +++ > gcc/rtl-ssa/changes.cc | 74 +++++++++++++++++++++++++++++++------- > gcc/rtl-ssa/changes.h | 2 ++ > gcc/rtl-ssa/functions.h | 14 ++++++++ > gcc/rtl-ssa/insns.cc | 5 +++ > gcc/rtl-ssa/insns.h | 7 +++- > gcc/rtl-ssa/internals.inl | 1 + > gcc/rtl-ssa/member-fns.inl | 12 +++++++ > gcc/rtl-ssa/movement.h | 8 ++++- > 10 files changed, 123 insertions(+), 14 deletions(-) > > diff --git a/gcc/rtl-ssa/accesses.cc b/gcc/rtl-ssa/accesses.cc > index 510545a8bad..76d70fd8bd3 100644 > --- a/gcc/rtl-ssa/accesses.cc > +++ b/gcc/rtl-ssa/accesses.cc > @@ -1456,6 +1456,16 @@ function_info::make_uses_available (obstack_watermark &watermark, > return use_array (new_uses, num_uses); > } > > +set_info * > +function_info::create_set (obstack_watermark &watermark, > + insn_info *insn, > + resource_info resource) > +{ > + auto set = change_alloc<set_info> (watermark, insn, resource); > + set->m_is_temp = true; > + return set; > +} > + > // Return true if ACCESS1 can represent ACCESS2 and if ACCESS2 can > // represent ACCESS1. > static bool > diff --git a/gcc/rtl-ssa/accesses.h b/gcc/rtl-ssa/accesses.h > index fce31d46717..7e7a90ece97 100644 > --- a/gcc/rtl-ssa/accesses.h > +++ b/gcc/rtl-ssa/accesses.h > @@ -204,6 +204,10 @@ public: > // in the main instruction pattern. > bool only_occurs_in_notes () const { return m_only_occurs_in_notes; } > > + // Return true if this is a temporary access, e.g. one created for > + // an insn that is about to be inserted. > + bool is_temporary () const { return m_is_temp; } > + > protected: > access_info (resource_info, access_kind); > > diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc > index aab532b9f26..da2a61d701a 100644 > --- a/gcc/rtl-ssa/changes.cc > +++ b/gcc/rtl-ssa/changes.cc > @@ -394,14 +394,20 @@ move_insn (insn_change &change, insn_info *after) > // At the moment we don't support moving instructions between EBBs, > // but this would be worth adding if it's useful. > insn_info *insn = change.insn (); > - gcc_assert (after->ebb () == insn->ebb ()); > + > bb_info *bb = after->bb (); > basic_block cfg_bb = bb->cfg_bb (); > > - if (insn->bb () != bb) > - // Force DF to mark the old block as dirty. > - df_insn_delete (rtl); > - ::remove_insn (rtl); > + if (!insn->is_temporary ()) > + { > + gcc_assert (after->ebb () == insn->ebb ()); > + > + if (insn->bb () != bb) > + // Force DF to mark the old block as dirty. > + df_insn_delete (rtl); > + ::remove_insn (rtl); > + } > + > ::add_insn_after (rtl, after_rtl, cfg_bb); > } > > @@ -439,10 +445,15 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos) > gcc_assert (def); > if (def->m_is_temp) > { > - // At present, the only temporary instruction definitions we > - // create are clobbers, such as those added during recog. > - gcc_assert (is_a<clobber_info *> (def)); > - def = allocate<clobber_info> (change.insn (), ref.regno); > + if (is_a<clobber_info *> (def)) > + def = allocate<clobber_info> (change.insn (), ref.regno); > + else if (is_a<set_info *> (def)) > + { > + def->m_is_temp = false; > + def = allocate<set_info> (change.insn (), def->resource ()); Why is it necessary to clear is_temp on the old temporary set, when it wasn't for the temporary clobber? > + } > + else > + gcc_unreachable (); > } > else if (!def->m_has_been_superceded) > { > @@ -511,7 +522,9 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos) > unsigned int i = 0; > for (use_info *use : change.new_uses) > { > - if (!use->m_has_been_superceded) > + if (use->m_is_temp) > + use->m_has_been_superceded = true; > + else if (!use->m_has_been_superceded) Where do the temporary uses come from? It doesn't look like this patch itself provided a new means of creating them. Thanks, Richard > { > use = allocate_temp<use_info> (insn, use->resource (), use->def ()); > use->m_has_been_superceded = true; > @@ -645,6 +658,8 @@ function_info::apply_changes_to_insn (insn_change &change) > > insn->set_accesses (builder.finish ().begin (), num_defs, num_uses); > } > + > + insn->m_is_temp = false; > } > > // Add a temporary placeholder instruction after AFTER. > @@ -677,7 +692,8 @@ function_info::change_insns (array_slice<insn_change *> changes) > if (!change->is_deletion ()) > { > // Remove any notes that are no longer relevant. > - update_notes (change->rtl ()); > + if (!change->insn ()->m_is_temp) > + update_notes (change->rtl ()); > > // Make sure that the placement of this instruction would still > // leave room for previous instructions. > @@ -686,6 +702,17 @@ function_info::change_insns (array_slice<insn_change *> changes) > // verify_insn_changes is supposed to make sure that this holds. > gcc_unreachable (); > min_insn = later_insn (min_insn, change->move_range.first); > + > + if (change->insn ()->m_is_temp) > + { > + change->m_insn = allocate<insn_info> (change->insn ()->bb (), > + change->rtl (), > + change->insn_uid ()); > + > + // Set the flag again so subsequent logic is aware. > + // It will be cleared later on. > + change->m_insn->m_is_temp = true; > + } > } > } > > @@ -784,7 +811,8 @@ function_info::change_insns (array_slice<insn_change *> changes) > // Remove the placeholder first so that we have a wider range of > // program points when inserting INSN. > insn_info *after = placeholder->prev_any_insn (); > - remove_insn (insn); > + if (!insn->is_temporary ()) > + remove_insn (insn); > remove_insn (placeholder); > insn->set_bb (after->bb ()); > add_insn_after (insn, after); > @@ -1105,6 +1133,28 @@ function_info::perform_pending_updates () > return changed_cfg; > } > > +insn_info * > +function_info::create_insn (obstack_watermark &watermark, > + rtx_code insn_code, > + rtx pat) > +{ > + rtx_insn *rti = nullptr; > + > + // TODO: extend, move in to emit-rtl.cc. > + switch (insn_code) > + { > + case INSN: > + rti = make_insn_raw (pat); > + break; > + default: > + gcc_unreachable (); > + } > + > + auto insn = change_alloc<insn_info> (watermark, nullptr, rti, INSN_UID (rti)); > + insn->m_is_temp = true; > + return insn; > +} > + > // Print a description of CHANGE to PP. > void > rtl_ssa::pp_insn_change (pretty_printer *pp, const insn_change &change) > diff --git a/gcc/rtl-ssa/changes.h b/gcc/rtl-ssa/changes.h > index d56e3a646e2..d91cf432afe 100644 > --- a/gcc/rtl-ssa/changes.h > +++ b/gcc/rtl-ssa/changes.h > @@ -32,6 +32,8 @@ namespace rtl_ssa { > // something that we might do. > class insn_change > { > + friend class function_info; > + > public: > enum delete_action { DELETE }; > > diff --git a/gcc/rtl-ssa/functions.h b/gcc/rtl-ssa/functions.h > index ecb40fdaf57..4ffd3fa44e2 100644 > --- a/gcc/rtl-ssa/functions.h > +++ b/gcc/rtl-ssa/functions.h > @@ -68,6 +68,16 @@ public: > // Return the SSA information for CFG_BB. > bb_info *bb (basic_block cfg_bb) const { return m_bbs[cfg_bb->index]; } > > + // Create a temporary def. > + set_info *create_set (obstack_watermark &watermark, > + insn_info *insn, > + resource_info resource); > + > + // Create a temporary insn with code INSN_CODE and pattern PAT. > + insn_info *create_insn (obstack_watermark &watermark, > + rtx_code insn_code, > + rtx pat); > + > // Return a list of all the instructions in the function, in reverse > // postorder. The list includes both real and artificial instructions. > // > @@ -195,6 +205,10 @@ public: > // Print the contents of the function to PP. > void print (pretty_printer *pp) const; > > + // Allocate an object of type T above the obstack watermark WM. > + template<typename T, typename... Ts> > + T *change_alloc (obstack_watermark &wm, Ts... args); > + > private: > class bb_phi_info; > class build_info; > diff --git a/gcc/rtl-ssa/insns.cc b/gcc/rtl-ssa/insns.cc > index 5fde3f2bb4b..2fa48e0dacd 100644 > --- a/gcc/rtl-ssa/insns.cc > +++ b/gcc/rtl-ssa/insns.cc > @@ -192,6 +192,11 @@ insn_info::print_full (pretty_printer *pp) const > pp_newline_and_indent (pp, 0); > pp_string (pp, "has volatile refs"); > } > + if (m_is_temp) > + { > + pp_newline_and_indent (pp, 0); > + pp_string (pp, "temporary"); > + } > } > pp_indentation (pp) -= 2; > } > diff --git a/gcc/rtl-ssa/insns.h b/gcc/rtl-ssa/insns.h > index a604fe295cd..6d0506706ad 100644 > --- a/gcc/rtl-ssa/insns.h > +++ b/gcc/rtl-ssa/insns.h > @@ -306,6 +306,8 @@ public: > // Print a full description of the instruction. > void print_full (pretty_printer *) const; > > + bool is_temporary () const { return m_is_temp; } > + > private: > // The first-order way of representing the order between instructions > // is to assign "program points", with higher point numbers coming > @@ -414,8 +416,11 @@ private: > unsigned int m_has_pre_post_modify : 1; > unsigned int m_has_volatile_refs : 1; > > + // Indicates the insn is a temporary / new user-allocated insn. > + unsigned int m_is_temp : 1; > + > // For future expansion. > - unsigned int m_spare : 27; > + unsigned int m_spare : 26; > > // The program point at which the instruction occurs. > // > diff --git a/gcc/rtl-ssa/internals.inl b/gcc/rtl-ssa/internals.inl > index e49297c12b3..907c4504352 100644 > --- a/gcc/rtl-ssa/internals.inl > +++ b/gcc/rtl-ssa/internals.inl > @@ -415,6 +415,7 @@ inline insn_info::insn_info (bb_info *bb, rtx_insn *rtl, int cost_or_uid) > m_is_asm (false), > m_has_pre_post_modify (false), > m_has_volatile_refs (false), > + m_is_temp (false), > m_spare (0), > m_point (0), > m_cost_or_uid (cost_or_uid), > diff --git a/gcc/rtl-ssa/member-fns.inl b/gcc/rtl-ssa/member-fns.inl > index ce2db045b78..b8940ca5566 100644 > --- a/gcc/rtl-ssa/member-fns.inl > +++ b/gcc/rtl-ssa/member-fns.inl > @@ -962,4 +962,16 @@ function_info::add_regno_clobber (obstack_watermark &watermark, > return true; > } > > +template<typename T, typename... Ts> > +inline T * > +function_info::change_alloc (obstack_watermark &wm, Ts... args) > +{ > + static_assert (std::is_trivially_destructible<T>::value, > + "destructor won't be called"); > + static_assert (alignof (T) <= obstack_alignment, > + "too much alignment required"); > + void *addr = XOBNEW (wm, T); > + return new (addr) T (std::forward<Ts> (args)...); > +} > + > } > diff --git a/gcc/rtl-ssa/movement.h b/gcc/rtl-ssa/movement.h > index ec076db406f..41226dd3666 100644 > --- a/gcc/rtl-ssa/movement.h > +++ b/gcc/rtl-ssa/movement.h > @@ -182,6 +182,11 @@ restrict_movement_for_defs_ignoring (insn_range_info &move_range, > { > for (def_info *def : defs) > { > + // Skip fresh defs that are being inserted, as these shouldn't > + // constrain movement. > + if (def->is_temporary ()) > + continue; > + > // If the definition is a clobber, we can move it with respect > // to other clobbers. > // > @@ -247,7 +252,8 @@ restrict_movement_for_defs_ignoring (insn_range_info &move_range, > > // Make sure that we don't move stores between basic blocks, since we > // don't have enough information to tell whether it's safe. > - if (def_info *def = memory_access (defs)) > + def_info *def = memory_access (defs); > + if (def && !def->is_temporary ()) > { > move_range = move_later_than (move_range, def->bb ()->head_insn ()); > move_range = move_earlier_than (move_range, def->bb ()->end_insn ());
On 21/11/2023 11:51, Richard Sandiford wrote: > Alex Coplan <alex.coplan@arm.com> writes: > > N.B. this is just a rebased (but otherwise unchanged) version of the > > same patch already posted here: > > > > https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633348.html > > > > this is the only unreviewed dependency from the previous series, so it > > seemed easier just to re-post it (not least to appease the pre-commit > > CI). > > > > -- >8 -- > > > > The upcoming aarch64 load pair pass needs to form store pairs, and can > > re-order stores over loads when alias analysis determines this is safe. > > In the case that both mem defs have uses in the RTL-SSA IR, and both > > stores require re-ordering over their uses, we represent that as > > (tentative) deletion of the original store insns and creation of a new > > insn, to prevent requiring repeated re-parenting of uses during the > > pass. We then update all mem uses that require re-parenting in one go > > at the end of the pass. > > > > To support this, RTL-SSA needs to handle inserting new insns (rather > > than just changing existing ones), so this patch adds support for that. > > > > New insns (and new accesses) are temporaries, allocated above a temporary > > obstack_watermark, such that the user can easily back out of a change without > > awkward bookkeeping. > > > > Bootstrapped/regtested as a series on aarch64-linux-gnu, OK for trunk? > > > > gcc/ChangeLog: > > > > * rtl-ssa/accesses.cc (function_info::create_set): New. > > * rtl-ssa/accesses.h (access_info::is_temporary): New. > > * rtl-ssa/changes.cc (move_insn): Handle new (temporary) insns. > > (function_info::finalize_new_accesses): Handle new/temporary > > user-created accesses. > > (function_info::apply_changes_to_insn): Ensure m_is_temp flag > > on new insns gets cleared. > > (function_info::change_insns): Handle new/temporary insns. > > (function_info::create_insn): New. > > * rtl-ssa/changes.h (class insn_change): Make function_info a > > friend class. > > * rtl-ssa/functions.h (function_info): Declare new entry points: > > create_set, create_insn. Declare new change_alloc helper. > > * rtl-ssa/insns.cc (insn_info::print_full): Identify temporary insns in > > dump. > > * rtl-ssa/insns.h (insn_info): Add new m_is_temp flag and accompanying > > is_temporary accessor. > > * rtl-ssa/internals.inl (insn_info::insn_info): Initialize m_is_temp to > > false. > > * rtl-ssa/member-fns.inl (function_info::change_alloc): New. > > * rtl-ssa/movement.h (restrict_movement_for_defs_ignoring): Add > > handling for temporary defs. > > Looks good, but there were a couple of things I didn't understand: Thanks for the review. > > > --- > > gcc/rtl-ssa/accesses.cc | 10 ++++++ > > gcc/rtl-ssa/accesses.h | 4 +++ > > gcc/rtl-ssa/changes.cc | 74 +++++++++++++++++++++++++++++++------- > > gcc/rtl-ssa/changes.h | 2 ++ > > gcc/rtl-ssa/functions.h | 14 ++++++++ > > gcc/rtl-ssa/insns.cc | 5 +++ > > gcc/rtl-ssa/insns.h | 7 +++- > > gcc/rtl-ssa/internals.inl | 1 + > > gcc/rtl-ssa/member-fns.inl | 12 +++++++ > > gcc/rtl-ssa/movement.h | 8 ++++- > > 10 files changed, 123 insertions(+), 14 deletions(-) > > > > diff --git a/gcc/rtl-ssa/accesses.cc b/gcc/rtl-ssa/accesses.cc > > index 510545a8bad..76d70fd8bd3 100644 > > --- a/gcc/rtl-ssa/accesses.cc > > +++ b/gcc/rtl-ssa/accesses.cc > > @@ -1456,6 +1456,16 @@ function_info::make_uses_available (obstack_watermark &watermark, > > return use_array (new_uses, num_uses); > > } > > > > +set_info * > > +function_info::create_set (obstack_watermark &watermark, > > + insn_info *insn, > > + resource_info resource) > > +{ > > + auto set = change_alloc<set_info> (watermark, insn, resource); > > + set->m_is_temp = true; > > + return set; > > +} > > + > > // Return true if ACCESS1 can represent ACCESS2 and if ACCESS2 can > > // represent ACCESS1. > > static bool > > diff --git a/gcc/rtl-ssa/accesses.h b/gcc/rtl-ssa/accesses.h > > index fce31d46717..7e7a90ece97 100644 > > --- a/gcc/rtl-ssa/accesses.h > > +++ b/gcc/rtl-ssa/accesses.h > > @@ -204,6 +204,10 @@ public: > > // in the main instruction pattern. > > bool only_occurs_in_notes () const { return m_only_occurs_in_notes; } > > > > + // Return true if this is a temporary access, e.g. one created for > > + // an insn that is about to be inserted. > > + bool is_temporary () const { return m_is_temp; } > > + > > protected: > > access_info (resource_info, access_kind); > > > > diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc > > index aab532b9f26..da2a61d701a 100644 > > --- a/gcc/rtl-ssa/changes.cc > > +++ b/gcc/rtl-ssa/changes.cc > > @@ -394,14 +394,20 @@ move_insn (insn_change &change, insn_info *after) > > // At the moment we don't support moving instructions between EBBs, > > // but this would be worth adding if it's useful. > > insn_info *insn = change.insn (); > > - gcc_assert (after->ebb () == insn->ebb ()); > > + > > bb_info *bb = after->bb (); > > basic_block cfg_bb = bb->cfg_bb (); > > > > - if (insn->bb () != bb) > > - // Force DF to mark the old block as dirty. > > - df_insn_delete (rtl); > > - ::remove_insn (rtl); > > + if (!insn->is_temporary ()) > > + { > > + gcc_assert (after->ebb () == insn->ebb ()); > > + > > + if (insn->bb () != bb) > > + // Force DF to mark the old block as dirty. > > + df_insn_delete (rtl); > > + ::remove_insn (rtl); > > + } > > + > > ::add_insn_after (rtl, after_rtl, cfg_bb); > > } > > > > @@ -439,10 +445,15 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos) > > gcc_assert (def); > > if (def->m_is_temp) > > { > > - // At present, the only temporary instruction definitions we > > - // create are clobbers, such as those added during recog. > > - gcc_assert (is_a<clobber_info *> (def)); > > - def = allocate<clobber_info> (change.insn (), ref.regno); > > + if (is_a<clobber_info *> (def)) > > + def = allocate<clobber_info> (change.insn (), ref.regno); > > + else if (is_a<set_info *> (def)) > > + { > > + def->m_is_temp = false; > > + def = allocate<set_info> (change.insn (), def->resource ()); > > Why is it necessary to clear is_temp on the old temporary set, > when it wasn't for the temporary clobber? In the case of a store pair insn (in the parallel form), I think we will see two writes of memory in properties.refs (). If we're inserting a new stp insn with a newly-created set of memory, then we must ensure that we only try to add one def of memory to m_temp_defs, or we will get an invalid access array. If we don't clear m_is_temp on the old def, then we will allocate and push two defs of memory in this case, which is clearly wrong. I don't think the same situation can happen with clobbers. AIUI, non-memory resources can only have at most one write in properties.refs (). Having said that, and having looked again at the code more closely, I don't think clearing the flag alone is the right solution, as the second time we process a memory write in this case, we will call record_reference against the old (temporary) def, and thus any changes made by record_reference will be lost. I think instead we should replace the temporary def with the newly-allocated (permanent) def in change.new_defs, and then push a pointer to this new def to m_temp_defs. So perhaps we could use find_access_index instead of find_access, and in the case that we find a temporary def, do something like: def = allocate<set_info> (change.insn (), def->resource ()); change.new_defs[def_index] = def; WDYT? > > > + } > > + else > > + gcc_unreachable (); > > } > > else if (!def->m_has_been_superceded) > > { > > @@ -511,7 +522,9 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos) > > unsigned int i = 0; > > for (use_info *use : change.new_uses) > > { > > - if (!use->m_has_been_superceded) > > + if (use->m_is_temp) > > + use->m_has_been_superceded = true; > > + else if (!use->m_has_been_superceded) > > Where do the temporary uses come from? It doesn't look like this patch > itself provided a new means of creating them. Ah, sorry, I think this is a hold-over from a previous iteration of the patch where we did provide an entry point for creating new uses. I dropped it in favour of teaching RTL-SSA to infer uses of mem (g:505f1202e3a1a1aecd0df10d0f1620df6fea4ab5). I'll drop this in the next iteration of the patch. Thanks, Alex > > Thanks, > Richard > > > { > > use = allocate_temp<use_info> (insn, use->resource (), use->def ()); > > use->m_has_been_superceded = true; > > @@ -645,6 +658,8 @@ function_info::apply_changes_to_insn (insn_change &change) > > > > insn->set_accesses (builder.finish ().begin (), num_defs, num_uses); > > } > > + > > + insn->m_is_temp = false; > > } > >
diff --git a/gcc/rtl-ssa/accesses.cc b/gcc/rtl-ssa/accesses.cc index 510545a8bad..76d70fd8bd3 100644 --- a/gcc/rtl-ssa/accesses.cc +++ b/gcc/rtl-ssa/accesses.cc @@ -1456,6 +1456,16 @@ function_info::make_uses_available (obstack_watermark &watermark, return use_array (new_uses, num_uses); } +set_info * +function_info::create_set (obstack_watermark &watermark, + insn_info *insn, + resource_info resource) +{ + auto set = change_alloc<set_info> (watermark, insn, resource); + set->m_is_temp = true; + return set; +} + // Return true if ACCESS1 can represent ACCESS2 and if ACCESS2 can // represent ACCESS1. static bool diff --git a/gcc/rtl-ssa/accesses.h b/gcc/rtl-ssa/accesses.h index fce31d46717..7e7a90ece97 100644 --- a/gcc/rtl-ssa/accesses.h +++ b/gcc/rtl-ssa/accesses.h @@ -204,6 +204,10 @@ public: // in the main instruction pattern. bool only_occurs_in_notes () const { return m_only_occurs_in_notes; } + // Return true if this is a temporary access, e.g. one created for + // an insn that is about to be inserted. + bool is_temporary () const { return m_is_temp; } + protected: access_info (resource_info, access_kind); diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc index aab532b9f26..da2a61d701a 100644 --- a/gcc/rtl-ssa/changes.cc +++ b/gcc/rtl-ssa/changes.cc @@ -394,14 +394,20 @@ move_insn (insn_change &change, insn_info *after) // At the moment we don't support moving instructions between EBBs, // but this would be worth adding if it's useful. insn_info *insn = change.insn (); - gcc_assert (after->ebb () == insn->ebb ()); + bb_info *bb = after->bb (); basic_block cfg_bb = bb->cfg_bb (); - if (insn->bb () != bb) - // Force DF to mark the old block as dirty. - df_insn_delete (rtl); - ::remove_insn (rtl); + if (!insn->is_temporary ()) + { + gcc_assert (after->ebb () == insn->ebb ()); + + if (insn->bb () != bb) + // Force DF to mark the old block as dirty. + df_insn_delete (rtl); + ::remove_insn (rtl); + } + ::add_insn_after (rtl, after_rtl, cfg_bb); } @@ -439,10 +445,15 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos) gcc_assert (def); if (def->m_is_temp) { - // At present, the only temporary instruction definitions we - // create are clobbers, such as those added during recog. - gcc_assert (is_a<clobber_info *> (def)); - def = allocate<clobber_info> (change.insn (), ref.regno); + if (is_a<clobber_info *> (def)) + def = allocate<clobber_info> (change.insn (), ref.regno); + else if (is_a<set_info *> (def)) + { + def->m_is_temp = false; + def = allocate<set_info> (change.insn (), def->resource ()); + } + else + gcc_unreachable (); } else if (!def->m_has_been_superceded) { @@ -511,7 +522,9 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos) unsigned int i = 0; for (use_info *use : change.new_uses) { - if (!use->m_has_been_superceded) + if (use->m_is_temp) + use->m_has_been_superceded = true; + else if (!use->m_has_been_superceded) { use = allocate_temp<use_info> (insn, use->resource (), use->def ()); use->m_has_been_superceded = true; @@ -645,6 +658,8 @@ function_info::apply_changes_to_insn (insn_change &change) insn->set_accesses (builder.finish ().begin (), num_defs, num_uses); } + + insn->m_is_temp = false; } // Add a temporary placeholder instruction after AFTER. @@ -677,7 +692,8 @@ function_info::change_insns (array_slice<insn_change *> changes) if (!change->is_deletion ()) { // Remove any notes that are no longer relevant. - update_notes (change->rtl ()); + if (!change->insn ()->m_is_temp) + update_notes (change->rtl ()); // Make sure that the placement of this instruction would still // leave room for previous instructions. @@ -686,6 +702,17 @@ function_info::change_insns (array_slice<insn_change *> changes) // verify_insn_changes is supposed to make sure that this holds. gcc_unreachable (); min_insn = later_insn (min_insn, change->move_range.first); + + if (change->insn ()->m_is_temp) + { + change->m_insn = allocate<insn_info> (change->insn ()->bb (), + change->rtl (), + change->insn_uid ()); + + // Set the flag again so subsequent logic is aware. + // It will be cleared later on. + change->m_insn->m_is_temp = true; + } } } @@ -784,7 +811,8 @@ function_info::change_insns (array_slice<insn_change *> changes) // Remove the placeholder first so that we have a wider range of // program points when inserting INSN. insn_info *after = placeholder->prev_any_insn (); - remove_insn (insn); + if (!insn->is_temporary ()) + remove_insn (insn); remove_insn (placeholder); insn->set_bb (after->bb ()); add_insn_after (insn, after); @@ -1105,6 +1133,28 @@ function_info::perform_pending_updates () return changed_cfg; } +insn_info * +function_info::create_insn (obstack_watermark &watermark, + rtx_code insn_code, + rtx pat) +{ + rtx_insn *rti = nullptr; + + // TODO: extend, move in to emit-rtl.cc. + switch (insn_code) + { + case INSN: + rti = make_insn_raw (pat); + break; + default: + gcc_unreachable (); + } + + auto insn = change_alloc<insn_info> (watermark, nullptr, rti, INSN_UID (rti)); + insn->m_is_temp = true; + return insn; +} + // Print a description of CHANGE to PP. void rtl_ssa::pp_insn_change (pretty_printer *pp, const insn_change &change) diff --git a/gcc/rtl-ssa/changes.h b/gcc/rtl-ssa/changes.h index d56e3a646e2..d91cf432afe 100644 --- a/gcc/rtl-ssa/changes.h +++ b/gcc/rtl-ssa/changes.h @@ -32,6 +32,8 @@ namespace rtl_ssa { // something that we might do. class insn_change { + friend class function_info; + public: enum delete_action { DELETE }; diff --git a/gcc/rtl-ssa/functions.h b/gcc/rtl-ssa/functions.h index ecb40fdaf57..4ffd3fa44e2 100644 --- a/gcc/rtl-ssa/functions.h +++ b/gcc/rtl-ssa/functions.h @@ -68,6 +68,16 @@ public: // Return the SSA information for CFG_BB. bb_info *bb (basic_block cfg_bb) const { return m_bbs[cfg_bb->index]; } + // Create a temporary def. + set_info *create_set (obstack_watermark &watermark, + insn_info *insn, + resource_info resource); + + // Create a temporary insn with code INSN_CODE and pattern PAT. + insn_info *create_insn (obstack_watermark &watermark, + rtx_code insn_code, + rtx pat); + // Return a list of all the instructions in the function, in reverse // postorder. The list includes both real and artificial instructions. // @@ -195,6 +205,10 @@ public: // Print the contents of the function to PP. void print (pretty_printer *pp) const; + // Allocate an object of type T above the obstack watermark WM. + template<typename T, typename... Ts> + T *change_alloc (obstack_watermark &wm, Ts... args); + private: class bb_phi_info; class build_info; diff --git a/gcc/rtl-ssa/insns.cc b/gcc/rtl-ssa/insns.cc index 5fde3f2bb4b..2fa48e0dacd 100644 --- a/gcc/rtl-ssa/insns.cc +++ b/gcc/rtl-ssa/insns.cc @@ -192,6 +192,11 @@ insn_info::print_full (pretty_printer *pp) const pp_newline_and_indent (pp, 0); pp_string (pp, "has volatile refs"); } + if (m_is_temp) + { + pp_newline_and_indent (pp, 0); + pp_string (pp, "temporary"); + } } pp_indentation (pp) -= 2; } diff --git a/gcc/rtl-ssa/insns.h b/gcc/rtl-ssa/insns.h index a604fe295cd..6d0506706ad 100644 --- a/gcc/rtl-ssa/insns.h +++ b/gcc/rtl-ssa/insns.h @@ -306,6 +306,8 @@ public: // Print a full description of the instruction. void print_full (pretty_printer *) const; + bool is_temporary () const { return m_is_temp; } + private: // The first-order way of representing the order between instructions // is to assign "program points", with higher point numbers coming @@ -414,8 +416,11 @@ private: unsigned int m_has_pre_post_modify : 1; unsigned int m_has_volatile_refs : 1; + // Indicates the insn is a temporary / new user-allocated insn. + unsigned int m_is_temp : 1; + // For future expansion. - unsigned int m_spare : 27; + unsigned int m_spare : 26; // The program point at which the instruction occurs. // diff --git a/gcc/rtl-ssa/internals.inl b/gcc/rtl-ssa/internals.inl index e49297c12b3..907c4504352 100644 --- a/gcc/rtl-ssa/internals.inl +++ b/gcc/rtl-ssa/internals.inl @@ -415,6 +415,7 @@ inline insn_info::insn_info (bb_info *bb, rtx_insn *rtl, int cost_or_uid) m_is_asm (false), m_has_pre_post_modify (false), m_has_volatile_refs (false), + m_is_temp (false), m_spare (0), m_point (0), m_cost_or_uid (cost_or_uid), diff --git a/gcc/rtl-ssa/member-fns.inl b/gcc/rtl-ssa/member-fns.inl index ce2db045b78..b8940ca5566 100644 --- a/gcc/rtl-ssa/member-fns.inl +++ b/gcc/rtl-ssa/member-fns.inl @@ -962,4 +962,16 @@ function_info::add_regno_clobber (obstack_watermark &watermark, return true; } +template<typename T, typename... Ts> +inline T * +function_info::change_alloc (obstack_watermark &wm, Ts... args) +{ + static_assert (std::is_trivially_destructible<T>::value, + "destructor won't be called"); + static_assert (alignof (T) <= obstack_alignment, + "too much alignment required"); + void *addr = XOBNEW (wm, T); + return new (addr) T (std::forward<Ts> (args)...); +} + } diff --git a/gcc/rtl-ssa/movement.h b/gcc/rtl-ssa/movement.h index ec076db406f..41226dd3666 100644 --- a/gcc/rtl-ssa/movement.h +++ b/gcc/rtl-ssa/movement.h @@ -182,6 +182,11 @@ restrict_movement_for_defs_ignoring (insn_range_info &move_range, { for (def_info *def : defs) { + // Skip fresh defs that are being inserted, as these shouldn't + // constrain movement. + if (def->is_temporary ()) + continue; + // If the definition is a clobber, we can move it with respect // to other clobbers. // @@ -247,7 +252,8 @@ restrict_movement_for_defs_ignoring (insn_range_info &move_range, // Make sure that we don't move stores between basic blocks, since we // don't have enough information to tell whether it's safe. - if (def_info *def = memory_access (defs)) + def_info *def = memory_access (defs); + if (def && !def->is_temporary ()) { move_range = move_later_than (move_range, def->bb ()->head_insn ()); move_range = move_earlier_than (move_range, def->bb ()->end_insn ());
N.B. this is just a rebased (but otherwise unchanged) version of the same patch already posted here: https://gcc.gnu.org/pipermail/gcc-patches/2023-October/633348.html this is the only unreviewed dependency from the previous series, so it seemed easier just to re-post it (not least to appease the pre-commit CI). -- >8 -- The upcoming aarch64 load pair pass needs to form store pairs, and can re-order stores over loads when alias analysis determines this is safe. In the case that both mem defs have uses in the RTL-SSA IR, and both stores require re-ordering over their uses, we represent that as (tentative) deletion of the original store insns and creation of a new insn, to prevent requiring repeated re-parenting of uses during the pass. We then update all mem uses that require re-parenting in one go at the end of the pass. To support this, RTL-SSA needs to handle inserting new insns (rather than just changing existing ones), so this patch adds support for that. New insns (and new accesses) are temporaries, allocated above a temporary obstack_watermark, such that the user can easily back out of a change without awkward bookkeeping. Bootstrapped/regtested as a series on aarch64-linux-gnu, OK for trunk? gcc/ChangeLog: * rtl-ssa/accesses.cc (function_info::create_set): New. * rtl-ssa/accesses.h (access_info::is_temporary): New. * rtl-ssa/changes.cc (move_insn): Handle new (temporary) insns. (function_info::finalize_new_accesses): Handle new/temporary user-created accesses. (function_info::apply_changes_to_insn): Ensure m_is_temp flag on new insns gets cleared. (function_info::change_insns): Handle new/temporary insns. (function_info::create_insn): New. * rtl-ssa/changes.h (class insn_change): Make function_info a friend class. * rtl-ssa/functions.h (function_info): Declare new entry points: create_set, create_insn. Declare new change_alloc helper. * rtl-ssa/insns.cc (insn_info::print_full): Identify temporary insns in dump. * rtl-ssa/insns.h (insn_info): Add new m_is_temp flag and accompanying is_temporary accessor. * rtl-ssa/internals.inl (insn_info::insn_info): Initialize m_is_temp to false. * rtl-ssa/member-fns.inl (function_info::change_alloc): New. * rtl-ssa/movement.h (restrict_movement_for_defs_ignoring): Add handling for temporary defs. --- gcc/rtl-ssa/accesses.cc | 10 ++++++ gcc/rtl-ssa/accesses.h | 4 +++ gcc/rtl-ssa/changes.cc | 74 +++++++++++++++++++++++++++++++------- gcc/rtl-ssa/changes.h | 2 ++ gcc/rtl-ssa/functions.h | 14 ++++++++ gcc/rtl-ssa/insns.cc | 5 +++ gcc/rtl-ssa/insns.h | 7 +++- gcc/rtl-ssa/internals.inl | 1 + gcc/rtl-ssa/member-fns.inl | 12 +++++++ gcc/rtl-ssa/movement.h | 8 ++++- 10 files changed, 123 insertions(+), 14 deletions(-)