[3/4] rtl-ssa: Ensure new defs get inserted [PR113070]
Checks
Commit Message
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(-)
Comments
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).
> 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 &);
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 &);
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
@@ -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"
@@ -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.
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.
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)
@@ -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 &);