Message ID | ZaKwCA4MBjcT5ViR@arm.com |
---|---|
State | New |
Headers | show |
Series | aarch64, rtl-ssa: Fix wrong code in ldp fusion pass [PR113070] | expand |
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
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. 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) 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 &);