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

[Richard Sandiford <richard.sandiford@arm.com>]

Alex Coplan <alex.coplan@arm.com> writes:
> 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)?

Yeah, sounds good to me.

Richard