Re: [PATCH 01/11] rtl-ssa: Support for inserting new insns
On 21/11/2023 11:51, Richard Sandiford wrote:
> Alex Coplan 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 (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
Re: [PATCH 01/11] rtl-ssa: Support for inserting new insns
Alex Coplan 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 (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)
> -
