Re: [PATCH V4 1/3] aarch64: Place target independent and dependent changed code in one file

[Alex Coplan <alex.coplan@arm.com>]

On 09/04/2024 20:01, Ajit Agarwal wrote:
> Hello Alex:
> 
> On 09/04/24 7:29 pm, Alex Coplan wrote:
> > On 09/04/2024 17:30, Ajit Agarwal wrote:
> >>
> >>
> >> On 05/04/24 10:03 pm, Alex Coplan wrote:
> >>> On 05/04/2024 13:53, Ajit Agarwal wrote:
> >>>> Hello Alex/Richard:
> >>>>
> >>>> All review comments are incorporated.
> >>>
> >>> Thanks, I was kind-of expecting you to also send the renaming patch as a
> >>> preparatory patch as we discussed.
> >>>
> >>> Sorry for another meta comment, but: I think the reason that the Linaro
> >>> CI isn't running tests on your patches is actually because you're
> >>> sending 1/3 of a series but not sending the rest of the series.
> >>>
> >>> So please can you either send this as an individual preparatory patch
> >>> (not marked as a series) or if you're going to send a series (e.g. with
> >>> a preparatory rename patch as 1/2 and this as 2/2) then send the entire
> >>> series when you make updates.  That way the CI should test your patches,
> >>> which would be helpful.
> >>>
> >>
> >> Addressed.
> >>  
> >>>>
> >>>> Common infrastructure of load store pair fusion is divided into target
> >>>> independent and target dependent changed code.
> >>>>
> >>>> Target independent code is the Generic code with pure virtual function
> >>>> to interface betwwen target independent and dependent code.
> >>>>
> >>>> Target dependent code is the implementation of pure virtual function for
> >>>> aarch64 target and the call to target independent code.
> >>>>
> >>>> Thanks & Regards
> >>>> Ajit
> >>>>
> >>>>
> >>>> aarch64: Place target independent and dependent changed code in one file
> >>>>
> >>>> Common infrastructure of load store pair fusion is divided into target
> >>>> independent and target dependent changed code.
> >>>>
> >>>> Target independent code is the Generic code with pure virtual function
> >>>> to interface betwwen target independent and dependent code.
> >>>>
> >>>> Target dependent code is the implementation of pure virtual function for
> >>>> aarch64 target and the call to target independent code.
> >>>>
> >>>> 2024-04-06  Ajit Kumar Agarwal  <aagarwa1@linux.ibm.com>
> >>>>
> >>>> gcc/ChangeLog:
> >>>>
> >>>> 	* config/aarch64/aarch64-ldp-fusion.cc: Place target
> >>>> 	independent and dependent changed code.
> >>>
> >>> You're going to need a proper ChangeLog eventually, but I guess there's
> >>> no need for that right now.
> >>>
> >>>> ---
> >>>>  gcc/config/aarch64/aarch64-ldp-fusion.cc | 371 +++++++++++++++--------
> >>>>  1 file changed, 249 insertions(+), 122 deletions(-)
> >>>>
> >>>> diff --git a/gcc/config/aarch64/aarch64-ldp-fusion.cc b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> >>>> index 22ed95eb743..cb21b514ef7 100644
> >>>> --- a/gcc/config/aarch64/aarch64-ldp-fusion.cc
> >>>> +++ b/gcc/config/aarch64/aarch64-ldp-fusion.cc
> >>>> @@ -138,8 +138,122 @@ struct alt_base
> >>>>    poly_int64 offset;
> >>>>  };
> >>>>  
> >>>> +// Virtual base class for load/store walkers used in alias analysis.
> >>>> +struct alias_walker
> >>>> +{
> >>>> +  virtual bool conflict_p (int &budget) const = 0;
> >>>> +  virtual insn_info *insn () const = 0;
> >>>> +  virtual bool valid () const  = 0;
> >>>
> >>> Heh, looking at this made me realise there is a whitespace bug here in
> >>> the existing code (double space after const).  Sorry about that!  I'll
> >>> push an obvious fix for that.
> >>>
> >>>> +  virtual void advance () = 0;
> >>>> +};
> >>>> +
> >>>> +struct pair_fusion {
> >>>> +
> >>>> +  pair_fusion () {};
> >>>
> >>> This ctor looks pointless at the moment.  Perhaps instead we could put
> >>> the contents of ldp_fusion_init in here and then delete that function?
> >>>
> >>
> >> Addressed.
> >>
> >>>> +  virtual bool fpsimd_op_p (rtx reg_op, machine_mode mem_mode,
> >>>> +			       bool load_p) = 0;
> >>>
> >>> Please can we have comments above each of these virtual functions
> >>> describing any parameters, what the purpose of the hook is, and the
> >>> interpretation of the return value?  This will serve as the
> >>> documentation for other targets that want to make use of the pass.
> >>>
> >>> It might make sense to have a default-false implementation for
> >>> fpsimd_op_p, especially if you don't want to make use of this bit for
> >>> rs6000.
> >>>
> >>
> >> Addressed.
> >>  
> >>>> +
> >>>> +  virtual bool pair_operand_mode_ok_p (machine_mode mode) = 0;
> >>>> +  virtual bool pair_trailing_writeback_p () = 0;
> >>>
> >>> Sorry for the run-around, but: I think this and
> >>> handle_writeback_opportunities () should be the same function, either
> >>> returning an enum or taking an enum and returning a boolean.
> >>>
> >>> At a minimum they should have more similar sounding names.
> >>>
> >>
> >> Addressed.
> >>
> >>>> +  virtual bool pair_reg_operand_ok_p (bool load_p, rtx reg_op,
> >>>> +				      machine_mode mem_mode) = 0;
> >>>> +  virtual int pair_mem_alias_check_limit () = 0;
> >>>> +  virtual bool handle_writeback_opportunities () = 0 ;
> >>>> +  virtual bool pair_mem_ok_with_policy (rtx first_mem, bool load_p,
> >>>> +					machine_mode mode) = 0;
> >>>> +  virtual rtx gen_mem_pair (rtx *pats,  rtx writeback,
> >>>
> >>> Nit: excess whitespace after pats,
> >>>
> >>>> +			    bool load_p) = 0;
> >>>> +  virtual bool pair_mem_promote_writeback_p (rtx pat) = 0;
> >>>> +  virtual bool track_load_p () = 0;
> >>>> +  virtual bool track_store_p () = 0;
> >>>
> >>> I think it would probably make more sense for these two to have
> >>> default-true implementations rather than being pure virtual functions.
> >>>
> >>> Also, sorry for the bikeshedding, but please can we keep the plural
> >>> names (so track_loads_p and track_stores_p).
> >>>
> >>>> +  virtual bool cand_insns_empty_p (std::list<insn_info *> &insns) = 0;
> >>>
> >>> Why does this need to be virtualised?  I would it expect it to
> >>> just be insns.empty () on all targets.
> >>>
> >>>> +  virtual bool pair_mem_in_range_p (HOST_WIDE_INT off) = 0;
> >>>> +  void ldp_fusion_bb (bb_info *bb);
> >>>
> >>> Just to check, are you planning to rename this function in a separate
> >>> patch?
> >>>
> >>
> >> Yes.
> >>>> +
> >>>> +  ~pair_fusion() { }
> >>>
> >>> As with the ctor, perhaps we could put the contents of ldp_fusion_destroy
> >>> in here and then delete that function?
> >>>
> >>
> >> Addressed.
> >>>> +};
> >>>> +
> >>>> +struct aarch64_pair_fusion : public pair_fusion
> >>>> +{
> >>>> +public:
> >>>
> >>
> >> Addressed.
> >>> Nit: there shouldn't be a need for this as public is already the default
> >>> for structs.
> >>>
> >>>> +  aarch64_pair_fusion () : pair_fusion () {};
> >>>
> >>> This ctor looks pointless, the default one should do, so can we remove
> >>> this?
> >>>
> >>
> >> Addressed.
> >>>> +  bool fpsimd_op_p (rtx reg_op, machine_mode mem_mode,
> >>>> +		    bool load_p) override final
> >>>> +  {
> >>>> +    const bool fpsimd_op_p
> >>>> +      = reload_completed
> >>>> +      ? (REG_P (reg_op) && FP_REGNUM_P (REGNO (reg_op)))
> >>>> +      : (GET_MODE_CLASS (mem_mode) != MODE_INT
> >>>> +	 && (load_p || !aarch64_const_zero_rtx_p (reg_op)));
> >>>> +    return fpsimd_op_p;
> >>>
> >>> Can this just directly return the boolean expression instead of storing
> >>> it into a variable first?
> >>>
> >> Addressed.
> >>>> +  }
> >>>> +
> >>>> +  bool pair_mem_promote_writeback_p (rtx pat)
> >>>> +  {
> >>>> +     if (reload_completed
> >>>> +	 && aarch64_ldp_writeback > 1
> >>>> +	 && GET_CODE (pat) == PARALLEL
> >>>> +	 && XVECLEN (pat, 0) == 2)
> >>>> +	return true;
> >>>> +
> >>>> +     return false;
> >>>> +  }
> >>>> +
> >>>> +  bool pair_mem_ok_with_policy (rtx first_mem, bool load_p,
> >>>> +				machine_mode mode)
> >>>> +  {
> >>>> +    return aarch64_mem_ok_with_ldpstp_policy_model (first_mem,
> >>>> +						     load_p,
> >>>> +						     mode);
> >>>> +  }
> >>>> +  bool pair_operand_mode_ok_p (machine_mode mode);
> >>>
> >>> Why have you implemented this (one-line function) out-of-line but the
> >>> others are implemented inline?  Please be consistent.
> >>>
> >>
> >> Addressed.
> >>>> +
> >>>> +  rtx gen_mem_pair (rtx *pats, rtx writeback, bool load_p);
> >>>> +
> >>>> +  bool pair_trailing_writeback_p  ()
> >>>> +  {
> >>>> +    return aarch64_ldp_writeback > 1;
> >>>> +  }
> >>>> +  bool pair_reg_operand_ok_p (bool load_p, rtx reg_op,
> >>>> +			      machine_mode mem_mode)
> >>>> +  {
> >>>> +    return (load_p
> >>>> +	     ? aarch64_ldp_reg_operand (reg_op, mem_mode)
> >>>> +	     : aarch64_stp_reg_operand (reg_op, mem_mode));
> >>>> +  }
> >>>> +  int pair_mem_alias_check_limit ()
> >>>> +  {
> >>>> +    return aarch64_ldp_alias_check_limit;
> >>>> +  }
> >>>> +  bool handle_writeback_opportunities ()
> >>>> +  {
> >>>> +    return aarch64_ldp_writeback;
> >>>> +  }
> >>>> +  bool track_load_p ()
> >>>> +  {
> >>>> +    const bool track_loads
> >>>> +      = aarch64_tune_params.ldp_policy_model != AARCH64_LDP_STP_POLICY_NEVER;
> >>>> +    return track_loads;
> >>>> +  }
> >>>
> >>
> >> Addressed.
> >>> As above, can this just return the boolean expression directly without
> >>> the temporary?
> >>>
> >>>> +  bool track_store_p ()
> >>>> +  {
> >>>> +    const bool track_stores
> >>>> +      = aarch64_tune_params.stp_policy_model != AARCH64_LDP_STP_POLICY_NEVER;
> >>>> +    return track_stores;
> >>>> +  }
> >>>
> >>
> >> Addressed.
> >>> Likewise.
> >>>
> >>>> +  bool cand_insns_empty_p (std::list<insn_info *> &insns)
> >>>> +  {
> >>>> +    return insns.empty();
> >>>> +  }
> >>>
> >>> As mentioned on the declaration, I don't see why this needs to be
> >>> virtualised.
> >>>
> >>>> +  bool pair_mem_in_range_p (HOST_WIDE_INT off)
> >>>> +  {
> >>>> +    return (off < LDP_MIN_IMM || off > LDP_MAX_IMM);
> >>>> +  }
> >>>
> >>> Hmm, this looks to test exactly the opposite condition from what the
> >>> name suggests, i.e. the implementation looks like what I would expect
> >>> for a function called pair_mem_out_of_range_p.
> >>>
> >>> Please can you invert the condition and fix up callers appropriately?
> >>>
> >>> This suggests you're not really thinking about the code you're writing,
> >>> please try to think a bit more carefully before posting patches.
> >>>
> >>
> >> Sorry for my mistake as I have overlooked it.
> >>>> +};
> >>>> +
> >>>> +
> >>>>  // State used by the pass for a given basic block.
> >>>> -struct ldp_bb_info
> >>>> +struct pair_fusion_bb_info
> >>>>  {
> >>>>    using def_hash = nofree_ptr_hash<def_info>;
> >>>>    using expr_key_t = pair_hash<tree_operand_hash, int_hash<int, -1, -2>>;
> >>>> @@ -160,14 +274,17 @@ struct ldp_bb_info
> >>>>  
> >>>>    static const size_t obstack_alignment = sizeof (void *);
> >>>>    bb_info *m_bb;
> >>>> +  pair_fusion *bb_state;
> >>>
> >>
> >> Addressed.
> >>
> >>> Can we call this m_pass instead of bb_state?  The pair_fusion class is
> >>> explicitly at the pass level rather than the bb level.
> >>>
> >>> Also I think this (and indeed m_bb, although that's pre-existing) could
> >>> be private members.  Not sure there's a good reason for making them
> >>> public.
> >>>
> >>
> >> Addressed.
> >>  
> >>> It might be slightly nicer to use a reference for m_pass unless there's
> >>> a good reason to use a pointer, although I don't feel strongly about
> >>> this.
> >>>
> >>>>  
> >>>> -  ldp_bb_info (bb_info *bb) : m_bb (bb), m_emitted_tombstone (false)
> >>>> +  pair_fusion_bb_info (bb_info *bb,
> >>>> +		      aarch64_pair_fusion *d) : m_bb (bb),
> >>>
> >>> This generic class needs to take the generic pass type, i.e. just a
> >>> pair_fusion * rather than an aarch64_pair_fusion *.
> >>>
> >>>> +		      bb_state (d), m_emitted_tombstone (false)
> >>>>    {
> >>>>      obstack_specify_allocation (&m_obstack, OBSTACK_CHUNK_SIZE,
> >>>>  				obstack_alignment, obstack_chunk_alloc,
> >>>>  				obstack_chunk_free);
> >>>>    }
> >>>> -  ~ldp_bb_info ()
> >>>> +  ~pair_fusion_bb_info ()
> >>>>    {
> >>>>      obstack_free (&m_obstack, nullptr);
> >>>>  
> >>>> @@ -177,10 +294,32 @@ struct ldp_bb_info
> >>>>  	bitmap_obstack_release (&m_bitmap_obstack);
> >>>>        }
> >>>>    }
> >>>> +  void track_access (insn_info *, bool load, rtx mem);
> >>>> +  void transform ();
> >>>> +  void cleanup_tombstones ();
> >>>
> >>> I think most (or indeed all?) of the changes here shouldn't be needed.
> >>> AFAICS, there shouldn't be a need to:
> >>>  - drop inline from these functions.
> >>>  - change the accessibility of any members.
> >>>
> >>
> >> Addressed.
> >>> Plese can you drop these changes or explain why they're needed (from
> >>> here until the end of the class).
> >>>
> >>>> +  void merge_pairs (insn_list_t &, insn_list_t &,
> >>>> +		    bool load_p, unsigned access_size);
> >>>> +  void transform_for_base (int load_size, access_group &group);
> >>>> +
> >>>> +  bool try_fuse_pair (bool load_p, unsigned access_size,
> >>>> +			     insn_info *i1, insn_info *i2);
> >>>> +
> >>>> +  bool fuse_pair (bool load_p, unsigned access_size,
> >>>> +		  int writeback,
> >>>> +		  insn_info *i1, insn_info *i2,
> >>>> +		  base_cand &base,
> >>>> +		  const insn_range_info &move_range);
> >>>>  
> >>>> -  inline void track_access (insn_info *, bool load, rtx mem);
> >>>> -  inline void transform ();
> >>>> -  inline void cleanup_tombstones ();
> >>>> +  void do_alias_analysis (insn_info *alias_hazards[4],
> >>>> +			  alias_walker *walkers[4],
> >>>> +			  bool load_p);
> >>>> +
> >>>> +  void track_tombstone (int uid);
> >>>> +
> >>>> +  bool track_via_mem_expr (insn_info *, rtx mem, lfs_fields lfs);
> >>>> +
> >>>> +  template<typename Map>
> >>>> +    void traverse_base_map (Map &map);
> >>>>  
> >>>>  private:
> >>>>    obstack m_obstack;
> >>>> @@ -191,30 +330,32 @@ private:
> >>>>    bool m_emitted_tombstone;
> >>>>  
> >>>>    inline splay_tree_node<access_record *> *node_alloc (access_record *);
> >>>> +};
> >>>>  
> >>>> -  template<typename Map>
> >>>> -  inline void traverse_base_map (Map &map);
> >>>> -  inline void transform_for_base (int load_size, access_group &group);
> >>>> -
> >>>> -  inline void merge_pairs (insn_list_t &, insn_list_t &,
> >>>> -			   bool load_p, unsigned access_size);
> >>>> -
> >>>> -  inline bool try_fuse_pair (bool load_p, unsigned access_size,
> >>>> -			     insn_info *i1, insn_info *i2);
> >>>> -
> >>>> -  inline bool fuse_pair (bool load_p, unsigned access_size,
> >>>> -			 int writeback,
> >>>> -			 insn_info *i1, insn_info *i2,
> >>>> -			 base_cand &base,
> >>>> -			 const insn_range_info &move_range);
> >>>> -
> >>>> -  inline void track_tombstone (int uid);
> >>>> +rtx aarch64_pair_fusion::gen_mem_pair (rtx *pats,
> >>>
> >>> Style nit: newline after rtx.
> >>>
> >> Addressed.
> >>
> >>>> +		  rtx writeback,
> >>>> +		  bool load_p)
> >>>> +  {
> >>>> +    rtx pair_pat;
> >>>>  
> >>>> -  inline bool track_via_mem_expr (insn_info *, rtx mem, lfs_fields lfs);
> >>>> -};
> >>>> +    if (writeback)
> >>>> +      {
> >>>> +	auto patvec = gen_rtvec (3, writeback, pats[0], pats[1]);
> >>>> +	pair_pat = gen_rtx_PARALLEL (VOIDmode, patvec);
> >>>> +      }
> >>>> +    else if (load_p)
> >>>> +      pair_pat = aarch64_gen_load_pair (XEXP (pats[0], 0),
> >>>> +					XEXP (pats[1], 0),
> >>>> +					XEXP (pats[0], 1));
> >>>> +    else
> >>>> +      pair_pat = aarch64_gen_store_pair (XEXP (pats[0], 0),
> >>>> +					 XEXP (pats[0], 1),
> >>>> +					 XEXP (pats[1], 1));
> >>>> +     return pair_pat;
> >>>
> >>> Can the assignments just be direct returns here, please?  Then
> >>> get rid of the temporary.
> >>>
> >>
> >> Addressed.
> >>  
> >>>> +  }
> >>>>  
> >>>>  splay_tree_node<access_record *> *
> >>>> -ldp_bb_info::node_alloc (access_record *access)
> >>>> +pair_fusion_bb_info::node_alloc (access_record *access)
> >>>>  {
> >>>>    using T = splay_tree_node<access_record *>;
> >>>>    void *addr = obstack_alloc (&m_obstack, sizeof (T));
> >>>> @@ -262,7 +403,7 @@ drop_writeback (rtx mem)
> >>>>  // RTX_AUTOINC addresses.  The interface is like strip_offset except we take a
> >>>>  // MEM so that we know the mode of the access.
> >>>>  static rtx
> >>>> -ldp_strip_offset (rtx mem, poly_int64 *offset)
> >>>> +pair_mem_strip_offset (rtx mem, poly_int64 *offset)
> >>>
> >>> I thought we discussed splitting the renaming into a separate patch?
> >>>
> >>>>  {
> >>>>    rtx addr = XEXP (mem, 0);
> >>>>  
> >>>> @@ -332,6 +473,12 @@ ldp_operand_mode_ok_p (machine_mode mode)
> >>>>    return reload_completed || mode != TImode;
> >>>>  }
> >>>>  
> >>>> +bool
> >>>> +aarch64_pair_fusion::pair_operand_mode_ok_p (machine_mode mode)
> >>>> +{
> >>>> +  return ldp_operand_mode_ok_p (mode);
> >>>> +}
> >>>> +
> >>>>  // Given LFS (load_p, fpsimd_p, size) fields in FIELDS, encode these
> >>>>  // into an integer for use as a hash table key.
> >>>>  static int
> >>>> @@ -396,7 +543,7 @@ access_group::track (Alloc alloc_node, poly_int64 offset, insn_info *insn)
> >>>>  // MEM_EXPR base (i.e. a tree decl) relative to which we can track the access.
> >>>>  // LFS is used as part of the key to the hash table, see track_access.
> >>>>  bool
> >>>> -ldp_bb_info::track_via_mem_expr (insn_info *insn, rtx mem, lfs_fields lfs)
> >>>> +pair_fusion_bb_info::track_via_mem_expr (insn_info *insn, rtx mem, lfs_fields lfs)
> >>>
> >>> Again, please do any renaming in a separate patch.
> >>>
> >>
> >> Addressed.
> >>>>  {
> >>>>    if (!MEM_EXPR (mem) || !MEM_OFFSET_KNOWN_P (mem))
> >>>>      return false;
> >>>> @@ -412,9 +559,10 @@ ldp_bb_info::track_via_mem_expr (insn_info *insn, rtx mem, lfs_fields lfs)
> >>>>    const machine_mode mem_mode = GET_MODE (mem);
> >>>>    const HOST_WIDE_INT mem_size = GET_MODE_SIZE (mem_mode).to_constant ();
> >>>>  
> >>>> -  // Punt on misaligned offsets.  LDP/STP instructions require offsets to be a
> >>>> -  // multiple of the access size, and we believe that misaligned offsets on
> >>>> -  // MEM_EXPR bases are likely to lead to misaligned offsets w.r.t. RTL bases.
> >>>> +  // Punt on misaligned offsets.  Paired memory access instructions require
> >>>> +  // offsets to be a multiple of the access size, and we believe that
> >>>> +  // misaligned offsets on MEM_EXPR bases are likely to lead to misaligned
> >>>> +  // offsets w.r.t. RTL bases.
> >>>>    if (!multiple_p (offset, mem_size))
> >>>>      return false;
> >>>>  
> >>>> @@ -438,46 +586,38 @@ ldp_bb_info::track_via_mem_expr (insn_info *insn, rtx mem, lfs_fields lfs)
> >>>>  }
> >>>>  
> >>>>  // Main function to begin pair discovery.  Given a memory access INSN,
> >>>> -// determine whether it could be a candidate for fusing into an ldp/stp,
> >>>> +// determine whether it could be a candidate for fusing into an pair mem,
> >>>
> >>> s/an pair mem/a paired access/
> >>>
> >>>>  // and if so, track it in the appropriate data structure for this basic
> >>>>  // block.  LOAD_P is true if the access is a load, and MEM is the mem
> >>>>  // rtx that occurs in INSN.
> >>>>  void
> >>>> -ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem)
> >>>> +pair_fusion_bb_info::track_access (insn_info *insn, bool load_p, rtx mem)
> >>>>  {
> >>>>    // We can't combine volatile MEMs, so punt on these.
> >>>>    if (MEM_VOLATILE_P (mem))
> >>>>      return;
> >>>>  
> >>>> -  // Ignore writeback accesses if the param says to do so.
> >>>> -  if (!aarch64_ldp_writeback
> >>>> +  // Ignore writeback accesses if the param says to do so
> >>>
> >>
> >> Addressed.
> >>> This comment needs updating, I guess something like "if the hook says to
> >>> do so".  But also please don't delete the full stop.
> >>>
> >>>> +  if (!bb_state->handle_writeback_opportunities ()
> >>>>        && GET_RTX_CLASS (GET_CODE (XEXP (mem, 0))) == RTX_AUTOINC)
> >>>>      return;
> >>>>  
> >>>>    const machine_mode mem_mode = GET_MODE (mem);
> >>>> -  if (!ldp_operand_mode_ok_p (mem_mode))
> >>>> +
> >>>
> >>> I don't think we need the extra whitespace here.
> >>>
> >>>> +  if (!bb_state->pair_operand_mode_ok_p (mem_mode))
> >>>>      return;
> >>>>  
> >>>>    rtx reg_op = XEXP (PATTERN (insn->rtl ()), !load_p);
> >>>>  
> >>>> -  // Ignore the access if the register operand isn't suitable for ldp/stp.
> >>>> -  if (load_p
> >>>> -      ? !aarch64_ldp_reg_operand (reg_op, mem_mode)
> >>>> -      : !aarch64_stp_reg_operand (reg_op, mem_mode))
> >>>> +  if (!bb_state->pair_reg_operand_ok_p (load_p, reg_op, mem_mode))
> >>>>      return;
> >>>> -
> >>>>    // We want to segregate FP/SIMD accesses from GPR accesses.
> >>>>    //
> >>>>    // Before RA, we use the modes, noting that stores of constant zero
> >>>>    // operands use GPRs (even in non-integer modes).  After RA, we use
> >>>>    // the hard register numbers.
> >>>> -  const bool fpsimd_op_p
> >>>> -    = reload_completed
> >>>> -    ? (REG_P (reg_op) && FP_REGNUM_P (REGNO (reg_op)))
> >>>> -    : (GET_MODE_CLASS (mem_mode) != MODE_INT
> >>>> -       && (load_p || !aarch64_const_zero_rtx_p (reg_op)));
> >>>> -
> >>>> -  // Note ldp_operand_mode_ok_p already rejected VL modes.
> >>>> +  const bool fpsimd_op_p = bb_state->fpsimd_op_p (reg_op, mem_mode, load_p);
> >>>> +  // Note pair_operand_mode_ok_p already rejected VL modes.
> >>>>    const HOST_WIDE_INT mem_size = GET_MODE_SIZE (mem_mode).to_constant ();
> >>>>    const lfs_fields lfs = { load_p, fpsimd_op_p, mem_size };
> >>>>  
> >>>> @@ -487,7 +627,7 @@ ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem)
> >>>>    poly_int64 mem_off;
> >>>>    rtx addr = XEXP (mem, 0);
> >>>>    const bool autoinc_p = GET_RTX_CLASS (GET_CODE (addr)) == RTX_AUTOINC;
> >>>> -  rtx base = ldp_strip_offset (mem, &mem_off);
> >>>> +  rtx base = pair_mem_strip_offset (mem, &mem_off);
> >>>
> >>> Again, let's split the renaming off into a separate patch.
> >>>
> >>
> >> Addressed.
> >>>>    if (!REG_P (base))
> >>>>      return;
> >>>>  
> >>>> @@ -506,8 +646,8 @@ ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem)
> >>>>    // elimination offset pre-RA, we should postpone forming pairs on such
> >>>>    // accesses until after RA.
> >>>>    //
> >>>> -  // As it stands, addresses with offsets in range for LDR but not
> >>>> -  // in range for LDP/STP are currently reloaded inefficiently,
> >>>> +  // As it stands, addresses in range for an individual load/store but not
> >>>> +  // for a paired access are currently reloaded inefficiently,
> >>>>    // ending up with a separate base register for each pair.
> >>>>    //
> >>>>    // In theory LRA should make use of
> >>>> @@ -519,8 +659,8 @@ ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem)
> >>>>    // that calls targetm.legitimize_address_displacement.
> >>>>    //
> >>>>    // So for now, it's better to punt when we can't be sure that the
> >>>> -  // offset is in range for LDP/STP.  Out-of-range cases can then be
> >>>> -  // handled after RA by the out-of-range LDP/STP peepholes.  Eventually, it
> >>>> +  // offset is in range for paired access.  Out-of-range cases can then be
> >>>> +  // handled after RA by the out-of-range PAIR MEM  peepholes.  Eventually, it
> >>>>    // would be nice to handle known out-of-range opportunities in the
> >>>>    // pass itself (for stack accesses, this would be in the post-RA pass).
> >>>>    if (!reload_completed
> >>>> @@ -573,8 +713,8 @@ ldp_bb_info::track_access (insn_info *insn, bool load_p, rtx mem)
> >>>>  	gcc_unreachable (); // Base defs should be unique.
> >>>>      }
> >>>>  
> >>>> -  // Punt on misaligned offsets.  LDP/STP require offsets to be a multiple of
> >>>> -  // the access size.
> >>>> +  // Punt on misaligned offsets.  Paired memory accesses require offsets
> >>>> +  // to be a multiple of the access size.
> >>>>    if (!multiple_p (mem_off, mem_size))
> >>>>      return;
> >>>>  
> >>>> @@ -614,7 +754,7 @@ static bool no_ignore (insn_info *) { return false; }
> >>>>  // making use of alias disambiguation.
> >>>>  static insn_info *
> >>>>  latest_hazard_before (insn_info *insn, rtx *ignore,
> >>>> -		      insn_info *ignore_insn = nullptr)
> >>>> +		      insn_info *ignore_insn = 0)
> >>>
> >>> Is this change needed?
> >>>
> >>
> >> Addressed.
> >>>>  {
> >>>>    insn_info *result = nullptr;
> >>>>  
> >>>> @@ -1150,7 +1290,7 @@ extract_writebacks (bool load_p, rtx pats[2], int changed)
> >>>>        const bool autoinc_p = GET_RTX_CLASS (GET_CODE (addr)) == RTX_AUTOINC;
> >>>>  
> >>>>        poly_int64 offset;
> >>>> -      rtx this_base = ldp_strip_offset (mem, &offset);
> >>>> +      rtx this_base = pair_mem_strip_offset (mem, &offset);
> >>>>        gcc_assert (REG_P (this_base));
> >>>>        if (base_reg)
> >>>>  	gcc_assert (rtx_equal_p (base_reg, this_base));
> >>>> @@ -1286,7 +1426,11 @@ find_trailing_add (insn_info *insns[2],
> >>>>  
> >>>>    off_hwi /= access_size;
> >>>>  
> >>>> -  if (off_hwi < LDP_MIN_IMM || off_hwi > LDP_MAX_IMM)
> >>>> +  pair_fusion *pfuse;
> >>>> +  aarch64_pair_fusion derived;
> >>>> +  pfuse = &derived;
> >>>
> >>> Huh? We'll want to use the same instance of the pass structure (pair_fusion)
> >>> that gets created at the top level, so that should get passed around everywhere.
> >>>
> >>> In the case of find_trailing_add we probably don't want to add any more
> >>> parameters (as Segher noted there are already too many), so perhaps the
> >>> easiest thing would be to make find_trailing_add itself a (non-virtual)
> >>> member function of pair_fusion.
> >>>
> >>> Then here we can just query pair_mem_in_range_p directly.
> >>>
> >>> For try_promote_writeback itself you can either make it a non-virtual
> >>> member function of pair_fusion or just pass a reference to the pass
> >>> object in when you call it.
> >>>
> >>> Note also that you don't have to create a pointer of the parent type in
> >>> order to call a virtual function on a derived object, you can just call
> >>> it directly.
> >>>
> >>
> >> Addressed.
> >>>> +
> >>>> +  if (pfuse->pair_mem_in_range_p (off_hwi))
> >>>>      return nullptr;
> >>>
> >>> Note of course this condition needs inverting when you invert the
> >>> implementation.
> >>>
> >>
> >> Addressed.
> >>>>  
> >>>>    auto dump_prefix = [&]()
> >>>> @@ -1328,7 +1472,7 @@ find_trailing_add (insn_info *insns[2],
> >>>>  // We just emitted a tombstone with uid UID, track it in a bitmap for
> >>>>  // this BB so we can easily identify it later when cleaning up tombstones.
> >>>>  void
> >>>> -ldp_bb_info::track_tombstone (int uid)
> >>>> +pair_fusion_bb_info::track_tombstone (int uid)
> >>>
> >>> Again, please split the renaming off.  Same with other instances below.
> >>>
> >>>>  {
> >>>>    if (!m_emitted_tombstone)
> >>>>      {
> >>>> @@ -1528,7 +1672,7 @@ fixup_debug_uses (obstack_watermark &attempt,
> >>>>  	  gcc_checking_assert (GET_RTX_CLASS (GET_CODE (XEXP (mem, 0)))
> >>>>  			       == RTX_AUTOINC);
> >>>>  
> >>>> -	  base = ldp_strip_offset (mem, &offset);
> >>>> +	  base = pair_mem_strip_offset (mem, &offset);
> >>>>  	  gcc_checking_assert (REG_P (base) && REGNO (base) == base_regno);
> >>>>  	}
> >>>>        fixup_debug_use (attempt, use, def, base, offset);
> >>>> @@ -1664,7 +1808,7 @@ fixup_debug_uses (obstack_watermark &attempt,
> >>>>  // BASE gives the chosen base candidate for the pair and MOVE_RANGE is
> >>>>  // a singleton range which says where to place the pair.
> >>>>  bool
> >>>> -ldp_bb_info::fuse_pair (bool load_p,
> >>>> +pair_fusion_bb_info::fuse_pair (bool load_p,
> >>>>  			unsigned access_size,
> >>>>  			int writeback,
> >>>>  			insn_info *i1, insn_info *i2,
> >>>> @@ -1800,7 +1944,7 @@ ldp_bb_info::fuse_pair (bool load_p,
> >>>>      {
> >>>>        if (dump_file)
> >>>>  	fprintf (dump_file,
> >>>> -		 "  ldp: i%d has wb but subsequent i%d has non-wb "
> >>>> +		 "  load pair: i%d has wb but subsequent i%d has non-wb "
> >>>>  		 "update of base (r%d), dropping wb\n",
> >>>>  		 insns[0]->uid (), insns[1]->uid (), base_regno);
> >>>>        gcc_assert (writeback_effect);
> >>>> @@ -1823,7 +1967,7 @@ ldp_bb_info::fuse_pair (bool load_p,
> >>>>      }
> >>>>  
> >>>>    // If either of the original insns had writeback, but the resulting pair insn
> >>>> -  // does not (can happen e.g. in the ldp edge case above, or if the writeback
> >>>> +  // does not (can happen e.g. in the load pair edge case above, or if the writeback
> >>>>    // effects cancel out), then drop the def(s) of the base register as
> >>>>    // appropriate.
> >>>>    //
> >>>> @@ -1842,7 +1986,7 @@ ldp_bb_info::fuse_pair (bool load_p,
> >>>>    // update of the base register and try and fold it in to make this into a
> >>>>    // writeback pair.
> >>>>    insn_info *trailing_add = nullptr;
> >>>> -  if (aarch64_ldp_writeback > 1
> >>>> +  if (bb_state->pair_trailing_writeback_p ()
> >>>>        && !writeback_effect
> >>>>        && (!load_p || (!refers_to_regno_p (base_regno, base_regno + 1,
> >>>>  					 XEXP (pats[0], 0), nullptr)
> >>>> @@ -1863,14 +2007,14 @@ ldp_bb_info::fuse_pair (bool load_p,
> >>>>      }
> >>>>  
> >>>>    // Now that we know what base mem we're going to use, check if it's OK
> >>>> -  // with the ldp/stp policy.
> >>>> +  // with the pair mem policy.
> >>>>    rtx first_mem = XEXP (pats[0], load_p);
> >>>> -  if (!aarch64_mem_ok_with_ldpstp_policy_model (first_mem,
> >>>> -						load_p,
> >>>> -						GET_MODE (first_mem)))
> >>>> +  if (!bb_state->pair_mem_ok_with_policy (first_mem,
> >>>> +			  load_p,
> >>>> +			  GET_MODE (first_mem)))
> >>>>      {
> >>>>        if (dump_file)
> >>>> -	fprintf (dump_file, "punting on pair (%d,%d), ldp/stp policy says no\n",
> >>>> +	fprintf (dump_file, "punting on pair (%d,%d), pair mem  policy says no\n",
> >>>>  		 i1->uid (), i2->uid ());
> >>>>        return false;
> >>>>      }
> >>>> @@ -1878,21 +2022,10 @@ ldp_bb_info::fuse_pair (bool load_p,
> >>>>    rtx reg_notes = combine_reg_notes (first, second, load_p);
> >>>>  
> >>>>    rtx pair_pat;
> >>>> -  if (writeback_effect)
> >>>> -    {
> >>>> -      auto patvec = gen_rtvec (3, writeback_effect, pats[0], pats[1]);
> >>>> -      pair_pat = gen_rtx_PARALLEL (VOIDmode, patvec);
> >>>> -    }
> >>>> -  else if (load_p)
> >>>> -    pair_pat = aarch64_gen_load_pair (XEXP (pats[0], 0),
> >>>> -				      XEXP (pats[1], 0),
> >>>> -				      XEXP (pats[0], 1));
> >>>> -  else
> >>>> -    pair_pat = aarch64_gen_store_pair (XEXP (pats[0], 0),
> >>>> -				       XEXP (pats[0], 1),
> >>>> -				       XEXP (pats[1], 1));
> >>>>  
> >>>> +  pair_pat = bb_state->gen_mem_pair (pats, writeback_effect, load_p);
> >>>>    insn_change *pair_change = nullptr;
> >>>> +
> >>>>    auto set_pair_pat = [pair_pat,reg_notes](insn_change *change) {
> >>>>      rtx_insn *rti = change->insn ()->rtl ();
> >>>>      validate_unshare_change (rti, &PATTERN (rti), pair_pat, true);
> >>>> @@ -2133,15 +2266,6 @@ load_modified_by_store_p (insn_info *load,
> >>>>    return false;
> >>>>  }
> >>>>  
> >>>> -// Virtual base class for load/store walkers used in alias analysis.
> >>>> -struct alias_walker
> >>>> -{
> >>>> -  virtual bool conflict_p (int &budget) const = 0;
> >>>> -  virtual insn_info *insn () const = 0;
> >>>> -  virtual bool valid () const  = 0;
> >>>> -  virtual void advance () = 0;
> >>>> -};
> >>>> -
> >>>>  // Implement some common functionality used by both store_walker
> >>>>  // and load_walker.
> >>>>  template<bool reverse>
> >>>> @@ -2259,13 +2383,13 @@ public:
> >>>>  //
> >>>>  // We try to maintain the invariant that if a walker becomes invalid, we
> >>>>  // set its pointer to null.
> >>>> -static void
> >>>> -do_alias_analysis (insn_info *alias_hazards[4],
> >>>> +void
> >>>> +pair_fusion_bb_info::do_alias_analysis (insn_info *alias_hazards[4],
> >>>
> >>> It would probably make more sense for this to be a (non-virtual) member
> >>> function of the pair_fusion class, since it doesn't need to access the
> >>> BB-specific state.
> >>>
> >>
> >> Addressed.
> >>>>  		   alias_walker *walkers[4],
> >>>>  		   bool load_p)
> >>>>  {
> >>>>    const int n_walkers = 2 + (2 * !load_p);
> >>>> -  int budget = aarch64_ldp_alias_check_limit;
> >>>> +  int budget = bb_state->pair_mem_alias_check_limit ();
> >>>>  
> >>>>    auto next_walker = [walkers,n_walkers](int current) -> int {
> >>>>      for (int j = 1; j <= n_walkers; j++)
> >>>> @@ -2365,7 +2489,7 @@ get_viable_bases (insn_info *insns[2],
> >>>>      {
> >>>>        const bool is_lower = (i == reversed);
> >>>>        poly_int64 poly_off;
> >>>> -      rtx base = ldp_strip_offset (cand_mems[i], &poly_off);
> >>>> +      rtx base = pair_mem_strip_offset (cand_mems[i], &poly_off);
> >>>>        if (GET_RTX_CLASS (GET_CODE (XEXP (cand_mems[i], 0))) == RTX_AUTOINC)
> >>>>  	writeback |= (1 << i);
> >>>>  
> >>>> @@ -2373,7 +2497,7 @@ get_viable_bases (insn_info *insns[2],
> >>>>  	continue;
> >>>>  
> >>>>        // Punt on accesses relative to eliminable regs.  See the comment in
> >>>> -      // ldp_bb_info::track_access for a detailed explanation of this.
> >>>> +      // pair_fusion_bb_info::track_access for a detailed explanation of this.
> >>>>        if (!reload_completed
> >>>>  	  && (REGNO (base) == FRAME_POINTER_REGNUM
> >>>>  	      || REGNO (base) == ARG_POINTER_REGNUM))
> >>>> @@ -2397,7 +2521,11 @@ get_viable_bases (insn_info *insns[2],
> >>>>        if (!is_lower)
> >>>>  	base_off--;
> >>>>  
> >>>> -      if (base_off < LDP_MIN_IMM || base_off > LDP_MAX_IMM)
> >>>> +      pair_fusion *pfuse;
> >>>> +      aarch64_pair_fusion derived;
> >>>> +      pfuse = &derived;
> >>>
> >>> Again, please don't do this but instead use the same instance
> >>> everywhere.
> >>>
> >>
> >> Addressed.
> >>>> +
> >>>> +      if (pfuse->pair_mem_in_range_p (base_off))
> >>>>  	continue;
> >>>>  
> >>>>        use_info *use = find_access (insns[i]->uses (), REGNO (base));
> >>>> @@ -2454,12 +2582,12 @@ get_viable_bases (insn_info *insns[2],
> >>>>  }
> >>>>  
> >>>>  // Given two adjacent memory accesses of the same size, I1 and I2, try
> >>>> -// and see if we can merge them into a ldp or stp.
> >>>> +// and see if we can merge them into a paired accesses load and store.
> >>>
> >>> "into a load pair or store pair" or "into a paired access" if you
> >>> prefer.
> >>>
> >>
> >> Addressed.
> >>>>  //
> >>>>  // ACCESS_SIZE gives the (common) size of a single access, LOAD_P is true
> >>>>  // if the accesses are both loads, otherwise they are both stores.
> >>>>  bool
> >>>> -ldp_bb_info::try_fuse_pair (bool load_p, unsigned access_size,
> >>>> +pair_fusion_bb_info::try_fuse_pair (bool load_p, unsigned access_size,
> >>>>  			    insn_info *i1, insn_info *i2)
> >>>>  {
> >>>>    if (dump_file)
> >>>> @@ -2494,7 +2622,7 @@ ldp_bb_info::try_fuse_pair (bool load_p, unsigned access_size,
> >>>>      {
> >>>>        if (dump_file)
> >>>>  	fprintf (dump_file,
> >>>> -		 "punting on ldp due to reg conflcits (%d,%d)\n",
> >>>> +		 "punting on pair mem load  due to reg conflcits (%d,%d)\n",
> >>>>  		 insns[0]->uid (), insns[1]->uid ());
> >>>>        return false;
> >>>>      }
> >>>> @@ -2843,7 +2971,7 @@ debug (const insn_list_t &l)
> >>>>  // we can't re-order them anyway, so provided earlier passes have cleaned up
> >>>>  // redundant loads, we shouldn't miss opportunities by doing this.
> >>>>  void
> >>>> -ldp_bb_info::merge_pairs (insn_list_t &left_list,
> >>>> +pair_fusion_bb_info::merge_pairs (insn_list_t &left_list,
> >>>>  			  insn_list_t &right_list,
> >>>>  			  bool load_p,
> >>>>  			  unsigned access_size)
> >>>> @@ -2890,8 +3018,8 @@ ldp_bb_info::merge_pairs (insn_list_t &left_list,
> >>>>  // of accesses.  If we find two sets of adjacent accesses, call
> >>>>  // merge_pairs.
> >>>>  void
> >>>> -ldp_bb_info::transform_for_base (int encoded_lfs,
> >>>> -				 access_group &group)
> >>>> +pair_fusion_bb_info::transform_for_base (int encoded_lfs,
> >>>> +					 access_group &group)
> >>>>  {
> >>>>    const auto lfs = decode_lfs (encoded_lfs);
> >>>>    const unsigned access_size = lfs.size;
> >>>> @@ -2909,7 +3037,7 @@ ldp_bb_info::transform_for_base (int encoded_lfs,
> >>>>  		       access.cand_insns,
> >>>>  		       lfs.load_p,
> >>>>  		       access_size);
> >>>> -	  skip_next = access.cand_insns.empty ();
> >>>> +	  skip_next = bb_state->cand_insns_empty_p (access.cand_insns);
> >>>
> >>> As above, why is this needed?
> >>
> >> For rs6000 we want to return always true. as load store pair
> >> that are to be merged with 8/16 16/32 32/64 is occuring for rs6000.
> >> And we want load store pair to 8/16 32/64. Thats why we want
> >> to generate always true for rs6000 to skip pairs as above.
> > 
> > Hmm, sorry, I'm not sure I follow.  Are you saying that for rs6000 you have
> > load/store pair instructions where the two arms of the access are storing
> > operands of different sizes?  Or something else?
> > 
> > As it stands the logic is to skip the next iteration only if we
> > exhausted all the candidate insns for the current access.  In the case
> > that we didn't exhaust all such candidates, then the idea is that when
> > access becomes prev_access, we can attempt to use those candidates as
> > the "left-hand side" of a pair in the next iteration since we failed to
> > use them as the "right-hand side" of a pair in the current iteration.
> > I don't see why you wouldn't want that behaviour.  Please can you
> > explain?
> > 
> 
> In merge_pair we get the 2 load candiates one load from 0 offset and
> other load is from 16th offset. Then in next iteration we get load
> from 16th offset and other load from 32 offset. In next iteration
> we get load from 32 offset and other load from 48 offset.
> 
> For example:
> 
> Currently we get the load candiates as follows.
> 
> pairs:
> 
> load from 0th offset.
> load from 16th offset.
> 
> next pairs:
> 
> load from 16th offset.
> load from 32th offset.
> 
> next pairs:
> 
> load from 32th offset
> load from 48th offset.
> 
> Instead in rs6000 we should get:
> 
> pairs:
> 
> load from 0th offset
> load from 16th offset.
> 
> next pairs:
> 
> load from 32th offset
> load from 48th offset.

Hmm, so then I guess my question is: why wouldn't you consider merging
the pair with offsets (16,32) for rs6000?  Is it because you have a
stricter alignment requirement on the base pair offsets (i.e. they have
to be a multiple of 32 when the operand size is 16)?  So the pair
offsets have to be a multiple of the entire pair size rather than a
single operand size?

If that is the case then I think it would be better to introduce a
virtual function (say pair_offset_alignment_ok_p) that vets the base
offset of the pair (prev_access->offset in transform_for_base).  I guess
it would also take access_size as a parameter and for aarch64 it should
check:

  multiple_p (offset, access_size)

and for rs6000 it could check:

  multiple_p (offset, access_size * 2)

and we would then incorporate a call to that predicate in the else if
condition of tranform_for_base.

It would have the same limitation whereby we assume that MEM_EXPR offset
alignment is a good proxy for RTL offset alignment, but we already
depend on that with the multiple_p check in track_via_mem_expr.

Thanks,
Alex

> 
> Thanks & Regards
> Ajit
> > Thanks,
> > Alex
> > 
> >>
> >>>
> >>>>  	}
> >>>>        prev_access = &access;
> >>>>      }
> >>>> @@ -2919,7 +3047,7 @@ ldp_bb_info::transform_for_base (int encoded_lfs,
> >>>>  // and remove all the tombstone insns, being sure to reparent any uses
> >>>>  // of mem to previous defs when we do this.
> >>>>  void
> >>>> -ldp_bb_info::cleanup_tombstones ()
> >>>> +pair_fusion_bb_info::cleanup_tombstones ()
> >>>>  {
> >>>>    // No need to do anything if we didn't emit a tombstone insn for this BB.
> >>>>    if (!m_emitted_tombstone)
> >>>> @@ -2947,7 +3075,7 @@ ldp_bb_info::cleanup_tombstones ()
> >>>>  
> >>>>  template<typename Map>
> >>>>  void
> >>>> -ldp_bb_info::traverse_base_map (Map &map)
> >>>> +pair_fusion_bb_info::traverse_base_map (Map &map)
> >>>>  {
> >>>>    for (auto kv : map)
> >>>>      {
> >>>> @@ -2958,7 +3086,7 @@ ldp_bb_info::traverse_base_map (Map &map)
> >>>>  }
> >>>>  
> >>>>  void
> >>>> -ldp_bb_info::transform ()
> >>>> +pair_fusion_bb_info::transform ()
> >>>>  {
> >>>>    traverse_base_map (expr_map);
> >>>>    traverse_base_map (def_map);
> >>>> @@ -3167,14 +3295,13 @@ try_promote_writeback (insn_info *insn)
> >>>>  // for load/store candidates.  If running after RA, also try and promote
> >>>>  // non-writeback pairs to use writeback addressing.  Then try to fuse
> >>>>  // candidates into pairs.
> >>>> -void ldp_fusion_bb (bb_info *bb)
> >>>> +void pair_fusion::ldp_fusion_bb (bb_info *bb)
> >>>>  {
> >>>> -  const bool track_loads
> >>>> -    = aarch64_tune_params.ldp_policy_model != AARCH64_LDP_STP_POLICY_NEVER;
> >>>> -  const bool track_stores
> >>>> -    = aarch64_tune_params.stp_policy_model != AARCH64_LDP_STP_POLICY_NEVER;
> >>>> +  const bool track_loads = track_load_p ();
> >>>> +  const bool track_stores = track_store_p ();
> >>>>  
> >>>> -  ldp_bb_info bb_state (bb);
> >>>
> >>> This:
> >>>
> >>>> +  aarch64_pair_fusion derived;
> >>>
> >>> can be deleted and then:
> >>>
> >>>> +  pair_fusion_bb_info bb_info (bb, &derived);
> >>>
> >>> can just be:
> >>>
> >>>   pair_fusion_bb_info bb_info (bb, this);
> >>>
> >>> (or you can pass *this if you make bb_info take a reference).
> >>>
> >>> I don't think there's a particular need to change the variable name
> >>> (bb_state -> bb_info).  I chose the former because it doens't clash
> >>> with the RTL-SSA structure of the same name as the latter.
> >>>
> >>
> >> Addressed.
> >>>>  
> >>>>    for (auto insn : bb->nondebug_insns ())
> >>>>      {
> >>>> @@ -3184,31 +3311,31 @@ void ldp_fusion_bb (bb_info *bb)
> >>>>  	continue;
> >>>>  
> >>>>        rtx pat = PATTERN (rti);
> >>>> -      if (reload_completed
> >>>> -	  && aarch64_ldp_writeback > 1
> >>>> -	  && GET_CODE (pat) == PARALLEL
> >>>> -	  && XVECLEN (pat, 0) == 2)
> >>>> +      if (pair_mem_promote_writeback_p (pat))
> >>>>  	try_promote_writeback (insn);
> >>>
> >>> It looks like try_promote_writeback itself will need some further work
> >>> to make it target-independent.  I suppose this check:
> >>>
> >>>   auto rti = insn->rtl ();
> >>>   const auto attr = get_attr_ldpstp (rti);
> >>>   if (attr == LDPSTP_NONE)
> >>>     return;
> >>>
> >>>   bool load_p = (attr == LDPSTP_LDP);
> >>>   gcc_checking_assert (load_p || attr == LDPSTP_STP);
> >>>
> >>> will need to become part of the pair_mem_promote_writeback_p hook that you
> >>> added, potentially changing it to return a boolean for load_p.
> >>>
> >>> Then I guess we will need hooks for destructuring the pair insn and
> >>> another hook to wrap aarch64_gen_writeback_pair.
> >>>
> >>
> >> Addressed.
> >>>>  
> >>>>        if (GET_CODE (pat) != SET)
> >>>>  	continue;
> >>>>  
> >>>>        if (track_stores && MEM_P (XEXP (pat, 0)))
> >>>> -	bb_state.track_access (insn, false, XEXP (pat, 0));
> >>>> +	bb_info.track_access (insn, false, XEXP (pat, 0));
> >>>>        else if (track_loads && MEM_P (XEXP (pat, 1)))
> >>>> -	bb_state.track_access (insn, true, XEXP (pat, 1));
> >>>> +	bb_info.track_access (insn, true, XEXP (pat, 1));
> >>>>      }
> >>>>  
> >>>> -  bb_state.transform ();
> >>>> -  bb_state.cleanup_tombstones ();
> >>>> +  bb_info.transform ();
> >>>> +  bb_info.cleanup_tombstones ();
> >>>>  }
> >>>>  
> >>>>  void ldp_fusion ()
> >>>>  {
> >>>>    ldp_fusion_init ();
> >>>> +  pair_fusion *pfuse;
> >>>> +  aarch64_pair_fusion derived;
> >>>> +  pfuse = &derived;
> >>>
> >>> This is indeed the one place where I think it is acceptable to
> >>> instantiate aarch64_pair_fusion.  But again there's no need to create a
> >>> pointer to the parent class, just call any function you like directly.
> >>>
> >>
> >> Addressed.
> >>>>  
> >>>>    for (auto bb : crtl->ssa->bbs ())
> >>>> -    ldp_fusion_bb (bb);
> >>>> +    pfuse->ldp_fusion_bb (bb);
> >>>
> >>> I think even the code to iterate over bbs should itself be a member
> >>> function of pair_fusion (say "run") and then that becomes part of the
> >>> generic code.
> >>>
> >>> So this function would just become:
> >>>
> >>> aarch64_pair_fusion pass;
> >>> pass.run ();
> >>>
> >>> and could be inlined into the caller.
> >>>
> >>
> >> Addressed.
> >>> Perhaps you could also add an early return like:
> >>>
> >>>   if (!track_loads_p () && !track_stores_p ())
> >>>     return;
> >>>
> >>> in pair_fusion::run () and then remove the corresponding code from
> >>> pass_ldp_fusion::gate?
> >>>
> >>
> >> Addressed.
> >>
> >>>>  
> >>>>    ldp_fusion_destroy ();
> >>>>  }
> >>>> -- 
> >>>> 2.39.3
> >>>>
> >>>
> >>> Thanks,
> >>> Alex
> >>
> >> Thanks & Regards
> >> Ajit