| Message ID | ZaKvlmv7yjTpcUTB@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | aarch64, rtl-ssa: Fix wrong code in ldp fusion pass [PR113070] | expand |
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
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
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.
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.