[PATCH 1/4] rtl-ssa: Run finalize_new_accesses forwards [PR113070]

[PATCH 1/4] rtl-ssa: Run finalize_new_accesses forwards [PR113070]

[Alex Coplan]

[-- Attachment #1: Type: text/plain, Size: 1236 bytes --]

The next patch in this series exposes an interface for creating new uses
in RTL-SSA.  The intent is that new user-created uses can consume new
user-created defs in the same change group.  This is so that we can
correctly update uses of memory when inserting a new store pair insn in
the aarch64 load/store pair fusion pass (the affected uses need to
consume the new store pair insn).

As it stands, finalize_new_accesses is called as part of the backwards
insn placement loop within change_insns, but if we want new uses to be
able to depend on new defs in the same change group, we need
finalize_new_accesses to be called on earlier insns first.  This is so
that when we process temporary uses and turn them into permanent uses,
we can follow the last_def link on the temporary def to ensure we end up
with a permanent use consuming a permanent def.

Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk?

Thanks,
Alex

gcc/ChangeLog:

	PR target/113070
	* rtl-ssa/changes.cc (function_info::change_insns): Split out the call
	to finalize_new_accesses from the backwards placement loop, run it
	forwards in a separate loop.
---
 gcc/rtl-ssa/changes.cc | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)


[-- Attachment #2: 0001-rtl-ssa-Run-finalize_new_accesses-forwards-PR113070.patch --]
[-- Type: text/x-patch, Size: 1532 bytes --]

diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
index 2fac45ae885..e538b637848 100644
--- a/gcc/rtl-ssa/changes.cc
+++ b/gcc/rtl-ssa/changes.cc
@@ -775,15 +775,26 @@ function_info::change_insns (array_slice<insn_change *> changes)
 	      placeholder = add_placeholder_after (after);
 	      following_insn = placeholder;
 	    }
-
-	  // Finalize the new list of accesses for the change.  Don't install
-	  // them yet, so that we still have access to the old lists below.
-	  finalize_new_accesses (change,
-				 placeholder ? placeholder : insn);
 	}
       placeholders[i] = placeholder;
     }
 
+  // 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.
+  //
+  // Note that we do this forwards instead of in the backwards loop above so
+  // that any new defs being inserted are processed before new uses of those
+  // defs, so that the (initially) temporary uses referring to temporary defs
+  // can be easily updated to become permanent uses referring to permanent defs.
+  for (unsigned i = 0; i < changes.size (); i++)
+    {
+      insn_change &change = *changes[i];
+      insn_info *placeholder = placeholders[i];
+      if (!change.is_deletion ())
+	finalize_new_accesses (change,
+			       placeholder ? placeholder : change.insn ());
+    }
+
   // Remove all definitions that are no longer needed.  After the above,
   // the only uses of such definitions should be dead phis and now-redundant
   // live-out uses.

Re: [PATCH 1/4] rtl-ssa: Run finalize_new_accesses forwards [PR113070]

[Jeff Law]

On 1/13/24 08:43, Alex Coplan wrote:
> The next patch in this series exposes an interface for creating new uses
> in RTL-SSA.  The intent is that new user-created uses can consume new
> user-created defs in the same change group.  This is so that we can
> correctly update uses of memory when inserting a new store pair insn in
> the aarch64 load/store pair fusion pass (the affected uses need to
> consume the new store pair insn).
> 
> As it stands, finalize_new_accesses is called as part of the backwards
> insn placement loop within change_insns, but if we want new uses to be
> able to depend on new defs in the same change group, we need
> finalize_new_accesses to be called on earlier insns first.  This is so
> that when we process temporary uses and turn them into permanent uses,
> we can follow the last_def link on the temporary def to ensure we end up
> with a permanent use consuming a permanent def.
> 
> Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk?
> 
> Thanks,
> Alex
> 
> gcc/ChangeLog:
> 
> 	PR target/113070
> 	* rtl-ssa/changes.cc (function_info::change_insns): Split out the call
> 	to finalize_new_accesses from the backwards placement loop, run it
> 	forwards in a separate loop.
So just to be explicit -- given this is adjusting the rtl-ssa 
infrastructure, I was going to let Richard S. own the review side -- he 
knows that code better than I.

Jeff

Re: [PATCH 1/4] rtl-ssa: Run finalize_new_accesses forwards [PR113070]

[Alex Coplan]

On 17/01/2024 07:42, Jeff Law wrote:
> 
> 
> On 1/13/24 08:43, Alex Coplan wrote:
> > The next patch in this series exposes an interface for creating new uses
> > in RTL-SSA.  The intent is that new user-created uses can consume new
> > user-created defs in the same change group.  This is so that we can
> > correctly update uses of memory when inserting a new store pair insn in
> > the aarch64 load/store pair fusion pass (the affected uses need to
> > consume the new store pair insn).
> > 
> > As it stands, finalize_new_accesses is called as part of the backwards
> > insn placement loop within change_insns, but if we want new uses to be
> > able to depend on new defs in the same change group, we need
> > finalize_new_accesses to be called on earlier insns first.  This is so
> > that when we process temporary uses and turn them into permanent uses,
> > we can follow the last_def link on the temporary def to ensure we end up
> > with a permanent use consuming a permanent def.
> > 
> > Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk?
> > 
> > Thanks,
> > Alex
> > 
> > gcc/ChangeLog:
> > 
> > 	PR target/113070
> > 	* rtl-ssa/changes.cc (function_info::change_insns): Split out the call
> > 	to finalize_new_accesses from the backwards placement loop, run it
> > 	forwards in a separate loop.
> So just to be explicit -- given this is adjusting the rtl-ssa
> infrastructure, I was going to let Richard S. own the review side -- he
> knows that code better than I.

Yeah, that's fine, thanks.  Richard is away this week but back on Monday, so
hopefully he can take a look at it then.

Alex

> 
> Jeff

Re: [PATCH 1/4] rtl-ssa: Run finalize_new_accesses forwards [PR113070]

[Richard Sandiford]

Alex Coplan <alex.coplan@arm.com> writes:
> The next patch in this series exposes an interface for creating new uses
> in RTL-SSA.  The intent is that new user-created uses can consume new
> user-created defs in the same change group.  This is so that we can
> correctly update uses of memory when inserting a new store pair insn in
> the aarch64 load/store pair fusion pass (the affected uses need to
> consume the new store pair insn).
>
> As it stands, finalize_new_accesses is called as part of the backwards
> insn placement loop within change_insns, but if we want new uses to be
> able to depend on new defs in the same change group, we need
> finalize_new_accesses to be called on earlier insns first.  This is so
> that when we process temporary uses and turn them into permanent uses,
> we can follow the last_def link on the temporary def to ensure we end up
> with a permanent use consuming a permanent def.
>
> Bootstrapped/regtested on aarch64-linux-gnu, OK for trunk?
>
> Thanks,
> Alex
>
> gcc/ChangeLog:
>
> 	PR target/113070
> 	* rtl-ssa/changes.cc (function_info::change_insns): Split out the call
> 	to finalize_new_accesses from the backwards placement loop, run it
> 	forwards in a separate loop.

OK, thanks.

Richard

> ---
>  gcc/rtl-ssa/changes.cc | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/rtl-ssa/changes.cc b/gcc/rtl-ssa/changes.cc
> index 2fac45ae885..e538b637848 100644
> --- a/gcc/rtl-ssa/changes.cc
> +++ b/gcc/rtl-ssa/changes.cc
> @@ -775,15 +775,26 @@ function_info::change_insns (array_slice<insn_change *> changes)
>  	      placeholder = add_placeholder_after (after);
>  	      following_insn = placeholder;
>  	    }
> -
> -	  // Finalize the new list of accesses for the change.  Don't install
> -	  // them yet, so that we still have access to the old lists below.
> -	  finalize_new_accesses (change,
> -				 placeholder ? placeholder : insn);
>  	}
>        placeholders[i] = placeholder;
>      }
>  
> +  // 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.
> +  //
> +  // Note that we do this forwards instead of in the backwards loop above so
> +  // that any new defs being inserted are processed before new uses of those
> +  // defs, so that the (initially) temporary uses referring to temporary defs
> +  // can be easily updated to become permanent uses referring to permanent defs.
> +  for (unsigned i = 0; i < changes.size (); i++)
> +    {
> +      insn_change &change = *changes[i];
> +      insn_info *placeholder = placeholders[i];
> +      if (!change.is_deletion ())
> +	finalize_new_accesses (change,
> +			       placeholder ? placeholder : change.insn ());
> +    }
> +
>    // Remove all definitions that are no longer needed.  After the above,
>    // the only uses of such definitions should be dead phis and now-redundant
>    // live-out uses.