Re: [Patch, rs6000, aarch64, middle-end] Add implementation for different targets for pair mem fusion

[Richard Sandiford <richard.sandiford@arm.com>]

Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
> Hello All:
>
> Common infrastructure using generic code for pair mem fusion of different
> targets.
>
> rs6000 target specific specific code implements virtual functions defined
> by generic code.
>
> Code is implemented with pure virtual functions to interface with target
> code.
>
> Target specific code are added in rs6000-mem-fusion.cc and additional virtual
> function implementation required for rs6000 are added in aarch64-ldp-fusion.cc.
>
> Bootstrapped and regtested for aarch64-linux-gnu and powerpc64-linux-gnu.
>
> Thanks & Regards
> Ajit
>
>
> aarch64, rs6000, middle-end: Add implementation for different targets for pair mem fusion
>
> Common infrastructure using generic code for pair mem fusion of different
> targets.
>
> rs6000 target specific specific code implements virtual functions defined
> by generic code.
>
> Code is implemented with pure virtual functions to interface with target
> code.
>
> Target specific code are added in rs6000-mem-fusion.cc and additional virtual
> function implementation required for rs6000 are added in aarch64-ldp-fusion.cc.
>
> 2024-05-31  Ajit Kumar Agarwal  <aagarwa1@linux.ibm.com>
>
> gcc/ChangeLog:
>
> 	* config/aarch64/aarch64-ldp-fusion.cc: Add target specific
> 	implementation of additional virtual functions added in pair_fusion
> 	struct.
> 	* config/rs6000/rs6000-passes.def: New mem fusion pass
> 	before pass_early_remat.
> 	* config/rs6000/rs6000-mem-fusion.cc: Add new pass.
> 	Add target specific implementation using pure virtual
> 	functions.
> 	* config.gcc: Add new object file.
> 	* config/rs6000/rs6000-protos.h: Add new prototype for mem
> 	fusion pass.
> 	* config/rs6000/t-rs6000: Add new rule.
> 	* rtl-ssa/accesses.h: Moved set_is_live_out_use as public
> 	from private.
>
> gcc/testsuite/ChangeLog:
>
> 	* g++.target/powerpc/me-fusion.C: New test.
> 	* g++.target/powerpc/mem-fusion-1.C: New test.
> 	* gcc.target/powerpc/mma-builtin-1.c: Modify test.
> ---

This isn't a complete review, just some initial questions & comments
about selected parts.

> [...]
> +/* Check whether load can be fusable or not.
> +   Return true if dependent use is UNSPEC otherwise false.  */
> +bool
> +rs6000_pair_fusion::fuseable_load_p (insn_info *info)
> +{
> +  rtx_insn *insn = info->rtl ();
> +
> +  for (rtx note = REG_NOTES (insn); note; note = XEXP (note, 1))
> +    if (REG_NOTE_KIND (note) == REG_EQUAL
> +	|| REG_NOTE_KIND (note) == REG_EQUIV)
> +      return false;

It's unusual to punt on an optimisation because of a REG_EQUAL/EQUIV
note.  What's the reason for doing this?  Are you trying to avoid
fusing pairs before reload that are equivalent to a MEM (i.e. have
a natural spill slot)?  I think Alex hit a similar situation.

> +
> +  for (auto def : info->defs ())
> +    {
> +      auto set = dyn_cast<set_info *> (def);
> +      if (set && set->has_any_uses ())
> +	{
> +	  for (auto use : set->all_uses())

Nit: has_any_uses isn't necessary: the inner loop will simply do nothing
in that case.  Also, we can/should restrict the scan to non-debug uses.

This can then be:

  for (auto def : info->defs ())
    if (auto set = dyn_cast<set_info *> (def))
      for (auto use : set->nondebug_insn_uses())

> +	    {
> +	      if (use->insn ()->is_artificial ())
> +		return false;
> +
> +	       insn_info *info = use->insn ();
> +
> +	       if (info
> +		   && info->rtl ()

This test shouldn't be necessary.

> +		   && info->is_real ())
> +		  {
> +		    rtx_insn *rtl_insn = info->rtl ();
> +		    rtx set = single_set (rtl_insn);
> +
> +		    if (set == NULL_RTX)
> +		      return false;
> +
> +		    rtx op0 = SET_SRC (set);
> +		    if (GET_CODE (op0) != UNSPEC)
> +		      return false;

What's the motivation for rejecting unspecs?  It's unsual to treat
all unspecs as a distinct group.

Also, using single_set means that the function still lets through
parallels of two sets in which the sources are unspecs.  Is that
intentional?

The reasons behind things like the REG_EQUAL/EQUIV and UNSPEC decisions
need to be described in comments, so that other people coming to this
code later can understand the motivation.  The same thing applies to
other decisions in the patch.

> +		  }
> +	      }
> +	  }
> +    }
> +  return true;
> +}
> [...]
> diff --git a/gcc/pair-fusion.cc b/gcc/pair-fusion.cc
> index 9f897ac04e2..2dbe9f854ef 100644
> --- a/gcc/pair-fusion.cc
> +++ b/gcc/pair-fusion.cc
> @@ -312,7 +312,7 @@ static int
>  encode_lfs (lfs_fields fields)
>  {
>    int size_log2 = exact_log2 (fields.size);
> -  gcc_checking_assert (size_log2 >= 2 && size_log2 <= 4);
> +  gcc_checking_assert (size_log2 >= 2 && size_log2 <= 6);
>    return ((int)fields.load_p << 3)
>      | ((int)fields.fpsimd_p << 2)
>      | (size_log2 - 2);

The point of the assert is to make sure that "size_log2 - 2"
fits within 2 bits, and so does not interfere with the fpsimd_p
and load_p parts of the encoding.  If rs6000 needs size_log2 == 6,
the shift amounts should be increased by 1 to compensate.

If we do bump the shifts by 1, the new range can be:

  gcc_checking_assert (size_log2 >= 2 && size_log2 <= 9);

> [...]
> +  // Given insn_info pair I1 and I2, return true if offsets are in order.
> +  virtual bool should_handle_unordered_insns (rtl_ssa::insn_info *i1,
> +					      rtl_ssa::insn_info *i2) = 0;
> +

This name seems a bit misleading.  The function is used in:

@@ -2401,6 +2405,9 @@ pair_fusion_bb_info::try_fuse_pair (bool load_p, unsigned access_size,
       reversed = true;
     }
 
+  if (!m_pass->should_handle_unordered_insns (i1, i2))
+    return false;
+
   rtx cand_mems[2];
   rtx reg_ops[2];
   rtx pats[2];

and so it acts as a general opt-out.  The insns aren't known to be unordered.

It looks like the rs6000 override requires the original insns to be
in offset order.  Could you say why that's necessary?  (Both in email
and as a comment in the code.)

If we do need a general opt-out like this, it should probably go at the
very start of try_fuse_pair, before even the dump message.  (Alternatively,
it could go after the dump message, but then the "return false" would need
a dump message of its own to explain the failure.)

Thanks,
Richard