Re: [PATCH 3/4] rtl-ssa: Ensure new defs get inserted [PR113070]
Alex Coplan writes: > On 22/01/2024 13:49, Richard Sandiford wrote: >> Alex Coplan 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
Re: [PATCH 3/4] rtl-ssa: Ensure new defs get inserted [PR113070]
On 22/01/2024 13:49, Richard Sandiford wrote:
> Alex Coplan 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 &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 (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 &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 c
Re: [PATCH 3/4] rtl-ssa: Ensure new defs get inserted [PR113070]
Alex Coplan 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).
> 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 &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 (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 &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 (def) && !def->is_call_clobber ())
> +if ((is_a (def) && !def->is_call_clobber ())
> + || (is_a (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
> changes)
>placeholders[i] = placeholder;
> }
>
> + /
