On 21/11/2023 11:51, Richard Sandiford wrote: > Alex Coplan <alex.cop...@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; > > } > >