[PATCH 1/4] rtl-ssa: Run finalize_new_accesses forwards [PR113070]
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(-)
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 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]
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]
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]
Alex Coplan 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
> 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.
