Message ID | ZaKv0rFkGjlDO14D@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: > This exposes an interface for users to create new uses in RTL-SSA. > This is needed for updating uses after inserting a new store pair insn > in the aarch64 load/store pair fusion pass. > > gcc/ChangeLog: > > PR target/113070 > * rtl-ssa/accesses.cc (function_info::create_use): New. > * rtl-ssa/changes.cc (function_info::finalize_new_accesses): > Handle temporary uses, ensure new uses end up referring to > permanent defs. > * rtl-ssa/functions.h (function_info::create_use): Declare. > --- > gcc/rtl-ssa/accesses.cc | 10 ++++++++++ > gcc/rtl-ssa/changes.cc | 24 +++++++++++++++++++----- > gcc/rtl-ssa/functions.h | 5 +++++ > 3 files changed, 34 insertions(+), 5 deletions(-) > > diff --git a/gcc/rtl-ssa/accesses.cc b/gcc/rtl-ssa/accesses.cc > index ce4a8b8dc00..3f1304fc5bf 100644 > --- a/gcc/rtl-ssa/accesses.cc > +++ b/gcc/rtl-ssa/accesses.cc > @@ -1466,6 +1466,16 @@ function_info::create_set (obstack_watermark &watermark, > return set; > } > > +use_info * > +function_info::create_use (obstack_watermark &watermark, > + insn_info *insn, > + set_info *set) > +{ > + auto use = change_alloc<use_info> (watermark, insn, set->resource (), set); > + use->m_is_temp = true; > + return use; > +} > + > // Return true if ACCESS1 can represent ACCESS2 and if ACCESS2 can > // represent ACCESS1. > static bool > diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc > index e538b637848..ce51d6ccd8d 100644 > --- a/gcc/rtl-ssa/changes.cc > +++ b/gcc/rtl-ssa/changes.cc > @@ -538,7 +538,9 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos) > unsigned int i = 0; > for (use_info *use : change.new_uses) > { > - if (!use->m_has_been_superceded) > + if (use->m_is_temp) > + use->m_has_been_superceded = true; > + else if (!use->m_has_been_superceded) > { Is this part necessary for correctness, or is it just a compile-time optimisation? We already have temporary uses via make_uses_available, and in principle, it's possible to reuse the uses for multiple changes within the same group. E.g. when replacing A with B in multiple instructions, it's OK for the associated insn changes to refer to A's uses directly, or to uses created for A by make_uses_available. So IMO it'd better to drop this hunk if we can. > use = allocate_temp<use_info> (insn, use->resource (), use->def ()); > use->m_has_been_superceded = true; > @@ -609,15 +611,27 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos) > m_temp_uses[i] = use = allocate<use_info> (*use); > use->m_is_temp = false; > set_info *def = use->def (); > - // Handle cases in which the value was previously not used > - // within the block. > - if (def && def->m_is_temp) > + if (!def || !def->m_is_temp) > + continue; > + > + if (auto phi = dyn_cast<phi_info *> (def)) > { > - phi_info *phi = as_a<phi_info *> (def); > + // Handle cases in which the value was previously not used > + // within the block. > gcc_assert (phi->is_degenerate ()); > phi = create_degenerate_phi (phi->ebb (), phi->input_value (0)); > use->set_def (phi); > } > + else > + { > + // The temporary def may also be a set added with this change, in > + // which case the permanent set is stored in the last_def link, > + // and we need to update the use to refer to the permanent set. > + gcc_assert (is_a<set_info *> (def)); > + auto perm_set = as_a<set_info *> (def->last_def ()); > + gcc_assert (!perm_set->is_temporary ()); > + use->set_def (perm_set); > + } > } > } > > diff --git a/gcc/rtl-ssa/functions.h b/gcc/rtl-ssa/functions.h > index 58d0b50ea83..962180e27d6 100644 > --- a/gcc/rtl-ssa/functions.h > +++ b/gcc/rtl-ssa/functions.h > @@ -73,6 +73,11 @@ public: > insn_info *insn, > resource_info resource); > > + // Create a temporary use. How about something like: // Create a temporary use of SET as part of a change to INSN. // SET can be a pre-existing definition or one that is being created // as part of the same change group. (Feel free to tweak the wording.) OK those changes, thanks. Richard > + use_info *create_use (obstack_watermark &watermark, > + insn_info *insn, > + set_info *set); > + > // Create a temporary insn with code INSN_CODE and pattern PAT. > insn_info *create_insn (obstack_watermark &watermark, > rtx_code insn_code,
On 22/01/2024 13:45, Richard Sandiford wrote: > Alex Coplan <alex.coplan@arm.com> writes: > > This exposes an interface for users to create new uses in RTL-SSA. > > This is needed for updating uses after inserting a new store pair insn > > in the aarch64 load/store pair fusion pass. > > > > gcc/ChangeLog: > > > > PR target/113070 > > * rtl-ssa/accesses.cc (function_info::create_use): New. > > * rtl-ssa/changes.cc (function_info::finalize_new_accesses): > > Handle temporary uses, ensure new uses end up referring to > > permanent defs. > > * rtl-ssa/functions.h (function_info::create_use): Declare. > > --- > > gcc/rtl-ssa/accesses.cc | 10 ++++++++++ > > gcc/rtl-ssa/changes.cc | 24 +++++++++++++++++++----- > > gcc/rtl-ssa/functions.h | 5 +++++ > > 3 files changed, 34 insertions(+), 5 deletions(-) > > > > diff --git a/gcc/rtl-ssa/accesses.cc b/gcc/rtl-ssa/accesses.cc > > index ce4a8b8dc00..3f1304fc5bf 100644 > > --- a/gcc/rtl-ssa/accesses.cc > > +++ b/gcc/rtl-ssa/accesses.cc > > @@ -1466,6 +1466,16 @@ function_info::create_set (obstack_watermark &watermark, > > return set; > > } > > > > +use_info * > > +function_info::create_use (obstack_watermark &watermark, > > + insn_info *insn, > > + set_info *set) > > +{ > > + auto use = change_alloc<use_info> (watermark, insn, set->resource (), set); > > + use->m_is_temp = true; > > + return use; > > +} > > + > > // Return true if ACCESS1 can represent ACCESS2 and if ACCESS2 can > > // represent ACCESS1. > > static bool > > diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc > > index e538b637848..ce51d6ccd8d 100644 > > --- a/gcc/rtl-ssa/changes.cc > > +++ b/gcc/rtl-ssa/changes.cc > > @@ -538,7 +538,9 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos) > > unsigned int i = 0; > > for (use_info *use : change.new_uses) > > { > > - if (!use->m_has_been_superceded) > > + if (use->m_is_temp) > > + use->m_has_been_superceded = true; > > + else if (!use->m_has_been_superceded) > > { > > Is this part necessary for correctness, or is it just a compile-time > optimisation? We already have temporary uses via make_uses_available, > and in principle, it's possible to reuse the uses for multiple changes > within the same group. E.g. when replacing A with B in multiple > instructions, it's OK for the associated insn changes to refer to > A's uses directly, or to uses created for A by make_uses_available. > > So IMO it'd better to drop this hunk if we can. Yeah, I agree it's just a compile-time optimisation and shouldn't be needed for correctness. I initially thought it might save on memory, but IIUC the memory allocated with allocate_temp will get freed when we return from finalize_new_accesses anyway. So I'll drop that hunk and re-test the series, thanks. Alex > > > use = allocate_temp<use_info> (insn, use->resource (), use->def ()); > > use->m_has_been_superceded = true; > > @@ -609,15 +611,27 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos) > > m_temp_uses[i] = use = allocate<use_info> (*use); > > use->m_is_temp = false; > > set_info *def = use->def (); > > - // Handle cases in which the value was previously not used > > - // within the block. > > - if (def && def->m_is_temp) > > + if (!def || !def->m_is_temp) > > + continue; > > + > > + if (auto phi = dyn_cast<phi_info *> (def)) > > { > > - phi_info *phi = as_a<phi_info *> (def); > > + // Handle cases in which the value was previously not used > > + // within the block. > > gcc_assert (phi->is_degenerate ()); > > phi = create_degenerate_phi (phi->ebb (), phi->input_value (0)); > > use->set_def (phi); > > } > > + else > > + { > > + // The temporary def may also be a set added with this change, in > > + // which case the permanent set is stored in the last_def link, > > + // and we need to update the use to refer to the permanent set. > > + gcc_assert (is_a<set_info *> (def)); > > + auto perm_set = as_a<set_info *> (def->last_def ()); > > + gcc_assert (!perm_set->is_temporary ()); > > + use->set_def (perm_set); > > + } > > } > > } > > > > diff --git a/gcc/rtl-ssa/functions.h b/gcc/rtl-ssa/functions.h > > index 58d0b50ea83..962180e27d6 100644 > > --- a/gcc/rtl-ssa/functions.h > > +++ b/gcc/rtl-ssa/functions.h > > @@ -73,6 +73,11 @@ public: > > insn_info *insn, > > resource_info resource); > > > > + // Create a temporary use. > > How about something like: > > // Create a temporary use of SET as part of a change to INSN. > // SET can be a pre-existing definition or one that is being created > // as part of the same change group. > > (Feel free to tweak the wording.) > > OK those changes, thanks. > > Richard > > > + use_info *create_use (obstack_watermark &watermark, > > + insn_info *insn, > > + set_info *set); > > + > > // Create a temporary insn with code INSN_CODE and pattern PAT. > > insn_info *create_insn (obstack_watermark &watermark, > > rtx_code insn_code,
diff --git a/gcc/rtl-ssa/accesses.cc b/gcc/rtl-ssa/accesses.cc index ce4a8b8dc00..3f1304fc5bf 100644 --- a/gcc/rtl-ssa/accesses.cc +++ b/gcc/rtl-ssa/accesses.cc @@ -1466,6 +1466,16 @@ function_info::create_set (obstack_watermark &watermark, return set; } +use_info * +function_info::create_use (obstack_watermark &watermark, + insn_info *insn, + set_info *set) +{ + auto use = change_alloc<use_info> (watermark, insn, set->resource (), set); + use->m_is_temp = true; + return use; +} + // Return true if ACCESS1 can represent ACCESS2 and if ACCESS2 can // represent ACCESS1. static bool diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc index e538b637848..ce51d6ccd8d 100644 --- a/gcc/rtl-ssa/changes.cc +++ b/gcc/rtl-ssa/changes.cc @@ -538,7 +538,9 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos) unsigned int i = 0; for (use_info *use : change.new_uses) { - if (!use->m_has_been_superceded) + if (use->m_is_temp) + use->m_has_been_superceded = true; + else if (!use->m_has_been_superceded) { use = allocate_temp<use_info> (insn, use->resource (), use->def ()); use->m_has_been_superceded = true; @@ -609,15 +611,27 @@ function_info::finalize_new_accesses (insn_change &change, insn_info *pos) m_temp_uses[i] = use = allocate<use_info> (*use); use->m_is_temp = false; set_info *def = use->def (); - // Handle cases in which the value was previously not used - // within the block. - if (def && def->m_is_temp) + if (!def || !def->m_is_temp) + continue; + + if (auto phi = dyn_cast<phi_info *> (def)) { - phi_info *phi = as_a<phi_info *> (def); + // Handle cases in which the value was previously not used + // within the block. gcc_assert (phi->is_degenerate ()); phi = create_degenerate_phi (phi->ebb (), phi->input_value (0)); use->set_def (phi); } + else + { + // The temporary def may also be a set added with this change, in + // which case the permanent set is stored in the last_def link, + // and we need to update the use to refer to the permanent set. + gcc_assert (is_a<set_info *> (def)); + auto perm_set = as_a<set_info *> (def->last_def ()); + gcc_assert (!perm_set->is_temporary ()); + use->set_def (perm_set); + } } } diff --git a/gcc/rtl-ssa/functions.h b/gcc/rtl-ssa/functions.h index 58d0b50ea83..962180e27d6 100644 --- a/gcc/rtl-ssa/functions.h +++ b/gcc/rtl-ssa/functions.h @@ -73,6 +73,11 @@ public: insn_info *insn, resource_info resource); + // Create a temporary use. + use_info *create_use (obstack_watermark &watermark, + insn_info *insn, + set_info *set); + // Create a temporary insn with code INSN_CODE and pattern PAT. insn_info *create_insn (obstack_watermark &watermark, rtx_code insn_code,