Re: [PATCH 3/4] rtl-ssa: Ensure new defs get inserted [PR113070]

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

On 22/01/2024 13:49, Richard Sandiford wrote:
> Alex Coplan <alex.coplan@arm.com> writes:
> > In r14-5820-ga49befbd2c783e751dc2110b544fe540eb7e33eb I added support to
> > RTL-SSA for inserting new insns, which included support for users
> > creating new defs.
> >
> > However, I missed that apply_changes_to_insn needed updating to ensure
> > that the new defs actually got inserted into the main def chain.  This
> > meant that when the aarch64 ldp/stp pass inserted a new stp insn, the
> > stp would just get skipped over during subsequent alias analysis, as its
> > def never got inserted into the memory def chain.  This (unsurprisingly)
> > led to wrong code.
> >
> > This patch fixes the issue by ensuring new user-created defs get
> > inserted.  I would have preferred to have used a flag internal to the
> > defs instead of a separate data structure to keep track of them, but since
> > machine_mode increased to 16 bits we're already at 64 bits in access_info,
> > and we can't really reuse m_is_temp as the logic in finalize_new_accesses
> > requires it to get cleared.
> >
> > Bootstrapped/regtested as a series on aarch64-linux-gnu, OK for trunk?
> >
> > Thanks,
> > Alex
> >
> > gcc/ChangeLog:
> >
> > 	PR target/113070
> > 	* rtl-ssa.h: Include hash-set.h.
> > 	* rtl-ssa/changes.cc (function_info::finalize_new_accesses): Add
> > 	new_sets parameter and use it to keep track of new user-created sets.
> > 	(function_info::apply_changes_to_insn): Also call add_def on new sets.
> > 	(function_info::change_insns): Add hash_set to keep track of new
> > 	user-created defs.  Plumb it through.
> > 	* rtl-ssa/functions.h: Add hash_set parameter to finalize_new_accesses and
> > 	apply_changes_to_insn.
> > ---
> >  gcc/rtl-ssa.h           |  1 +
> >  gcc/rtl-ssa/changes.cc  | 28 +++++++++++++++++++++-------
> >  gcc/rtl-ssa/functions.h |  6 ++++--
> >  3 files changed, 26 insertions(+), 9 deletions(-)
> >
> > diff --git a/gcc/rtl-ssa.h b/gcc/rtl-ssa.h
> > index f0cf656f5ac..17337639ae8 100644
> > --- a/gcc/rtl-ssa.h
> > +++ b/gcc/rtl-ssa.h
> > @@ -50,6 +50,7 @@
> >  #include "mux-utils.h"
> >  #include "rtlanal.h"
> >  #include "cfgbuild.h"
> > +#include "hash-set.h"
> >  
> >  // Provides the global crtl->ssa.
> >  #include "memmodel.h"
> > diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
> > index ce51d6ccd8d..6119ec3535b 100644
> > --- a/gcc/rtl-ssa/changes.cc
> > +++ b/gcc/rtl-ssa/changes.cc
> > @@ -429,7 +429,8 @@ update_insn_in_place (insn_change &change)
> >  // POS gives the final position of INSN, which hasn't yet been moved into
> >  // place.
> 
> The new parameter should be documented.  How about:
> 
>   // place.  NEW_SETS contains the new set_infos that are being added as part
>   // of this change (as opposed to being moved or repurposed from existing
>   // instructions).

That comment looks appropriate for apply_changes_to_insn, where NEW_SETS has
already been populated, but doesn't seem accurate for finalize_new_accesses.
How about:

  // place.  Keep track of any newly-created set_infos being added as
  // part of this change by adding them to NEW_SETS.

for finalize_new_accesses?  OK with that change (and using your suggestion for
apply_changes_to_insn)?

Thanks,
Alex

> 
> 
> >  void
> > -function_info::finalize_new_accesses (insn_change &change, insn_info *pos)
> > +function_info::finalize_new_accesses (insn_change &change, insn_info *pos,
> > +				      hash_set<def_info *> &new_sets)
> >  {
> >    insn_info *insn = change.insn ();
> >  
> > @@ -465,6 +466,12 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos)
> >  		// later in case we see a second write to the same resource.
> >  		def_info *perm_def = allocate<set_info> (change.insn (),
> >  							 def->resource ());
> > +
> > +		// Keep track of the new set so we remember to add it to the
> > +		// def chain later.
> > +		if (new_sets.add (perm_def))
> > +		  gcc_unreachable (); // We shouldn't see duplicates here.
> > +
> >  		def->set_last_def (perm_def);
> >  		def = perm_def;
> >  	      }
> > @@ -647,7 +654,8 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos)
> >  // Copy information from CHANGE to its underlying insn_info, given that
> >  // the insn_info has already been placed appropriately.
> 
> Similarly here.
> 
> OK with those changes, thanks.
> 
> Richard
> 
> >  void
> > -function_info::apply_changes_to_insn (insn_change &change)
> > +function_info::apply_changes_to_insn (insn_change &change,
> > +				      hash_set<def_info *> &new_sets)
> >  {
> >    insn_info *insn = change.insn ();
> >    if (change.is_deletion ())
> > @@ -659,10 +667,11 @@ function_info::apply_changes_to_insn (insn_change &change)
> >    // Copy the cost.
> >    insn->set_cost (change.new_cost);
> >  
> > -  // Add all clobbers.  Sets and call clobbers never move relative to
> > -  // other definitions, so are OK as-is.
> > +  // Add all clobbers and newly-created sets.  Existing sets and call
> > +  // clobbers never move relative to other definitions, so are OK as-is.
> >    for (def_info *def : change.new_defs)
> > -    if (is_a<clobber_info *> (def) && !def->is_call_clobber ())
> > +    if ((is_a<clobber_info *> (def) && !def->is_call_clobber ())
> > +	|| (is_a<set_info *> (def) && new_sets.contains (def)))
> >        add_def (def);
> >  
> >    // Add all uses, now that their position is final.
> > @@ -793,6 +802,10 @@ function_info::change_insns (array_slice<insn_change *> changes)
> >        placeholders[i] = placeholder;
> >      }
> >  
> > +  // We need to keep track of newly-added sets as these need adding to
> > +  // the def chain later.
> > +  hash_set<def_info *> new_sets;
> > +
> >    // Finalize the new list of accesses for each change.  Don't install them yet,
> >    // so that we still have access to the old lists below.
> >    //
> > @@ -806,7 +819,8 @@ function_info::change_insns (array_slice<insn_change *> changes)
> >        insn_info *placeholder = placeholders[i];
> >        if (!change.is_deletion ())
> >  	finalize_new_accesses (change,
> > -			       placeholder ? placeholder : change.insn ());
> > +			       placeholder ? placeholder : change.insn (),
> > +			       new_sets);
> >      }
> >  
> >    // Remove all definitions that are no longer needed.  After the above,
> > @@ -861,7 +875,7 @@ function_info::change_insns (array_slice<insn_change *> changes)
> >  
> >    // Apply the changes to the underlying insn_infos.
> >    for (insn_change *change : changes)
> > -    apply_changes_to_insn (*change);
> > +    apply_changes_to_insn (*change, new_sets);
> >  
> >    // Now that the insns and accesses are up to date, add any REG_UNUSED notes.
> >    for (insn_change *change : changes)
> > diff --git a/gcc/rtl-ssa/functions.h b/gcc/rtl-ssa/functions.h
> > index 962180e27d6..4573453d896 100644
> > --- a/gcc/rtl-ssa/functions.h
> > +++ b/gcc/rtl-ssa/functions.h
> > @@ -299,8 +299,10 @@ private:
> >    void process_uses_of_deleted_def (set_info *);
> >    insn_info *add_placeholder_after (insn_info *);
> >    void possibly_queue_changes (insn_change &);
> > -  void finalize_new_accesses (insn_change &, insn_info *);
> > -  void apply_changes_to_insn (insn_change &);
> > +  void finalize_new_accesses (insn_change &, insn_info *,
> > +			      hash_set<def_info *> &);
> > +  void apply_changes_to_insn (insn_change &,
> > +			      hash_set<def_info *> &);
> >  
> >    void init_function_data ();
> >    void calculate_potential_phi_regs (build_info &);