Re: [PATCH 01/11] rtl-ssa: Support for inserting new insns

[Alex Coplan <alex.coplan@arm.com>]

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;
> >  }
> >