diff mbox series

[2/4] rtl-ssa: Support for creating new uses [PR113070]

Message ID ZaKv0rFkGjlDO14D@arm.com
State Unresolved
Headers
Series aarch64, rtl-ssa: Fix wrong code in ldp fusion pass [PR113070] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Alex Coplan Jan. 13, 2024, 3:44 p.m. UTC 
  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(-)

Comments

Richard Sandiford Jan. 22, 2024, 1:45 p.m. UTC | #1 
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,
Alex Coplan Jan. 22, 2024, 9:31 p.m. UTC | #2 
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 mbox series

Patch

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,