Re: [PATCH 2/4] rtl-ssa: Support for creating new uses [PR113070]
On 22/01/2024 13:45, Richard Sandiford wrote:
> Alex Coplan 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
> > ,
> >return set;
> > }
> >
> > +use_info *
> > +function_info::create_use (obstack_watermark ,
> > + insn_info *insn,
> > + set_info *set)
> > +{
> > + auto use = change_alloc (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
> > , 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 (insn, use->resource (), use->def ());
> > use->m_has_been_superceded = true;
> > @@ -609,15 +611,27 @@ function_info::finalize_new_accesses (insn_change
> > , insn_info *pos)
> > m_temp_uses[i] = use = allocate (*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 (def))
> > {
> > - phi_info *phi = as_a (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 (def));
> > + auto perm_set = as_a (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 ,
> > + insn_info *insn,
> >
Re: [PATCH 2/4] rtl-ssa: Support for creating new uses [PR113070]
Alex Coplan 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
> ,
>return set;
> }
>
> +use_info *
> +function_info::create_use (obstack_watermark ,
> +insn_info *insn,
> +set_info *set)
> +{
> + auto use = change_alloc (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
> , 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 (insn, use->resource (), use->def ());
> use->m_has_been_superceded = true;
> @@ -609,15 +611,27 @@ function_info::finalize_new_accesses (insn_change
> , insn_info *pos)
> m_temp_uses[i] = use = allocate (*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 (def))
> {
> - phi_info *phi = as_a (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 (def));
> + auto perm_set = as_a (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 ,
> + insn_info *insn,
> + set_info *set);
> +
>// Create a temporary insn with code INSN_CODE and pattern PAT.
>insn_info *create_insn (obstack_watermark ,
> rtx_code insn_code,
[PATCH 2/4] rtl-ssa: Support for creating new uses [PR113070]
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 ,
return set;
}
+use_info *
+function_info::create_use (obstack_watermark ,
+ insn_info *insn,
+ set_info *set)
+{
+ auto use = change_alloc (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 , 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 (insn, use->resource (), use->def ());
use->m_has_been_superceded = true;
@@ -609,15 +611,27 @@ function_info::finalize_new_accesses (insn_change , insn_info *pos)
m_temp_uses[i] = use = allocate (*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 (def))
{
- phi_info *phi = as_a (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 (def));
+ auto perm_set = as_a (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 ,
+ insn_info *insn,
+ set_info *set);
+
// Create a temporary insn with code INSN_CODE and pattern PAT.
insn_info *create_insn (obstack_watermark ,
rtx_code insn_code,
