Alex Coplan <alex.cop...@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,

Reply via email to