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 Richard:
> On 31/05/24 8:08 pm, Richard Sandiford wrote:
>> Ajit Agarwal <aagarwa1@linux.ibm.com> writes:
>>> On 31/05/24 3:23 pm, Richard Sandiford wrote:
>>>> 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.
>>>>
>>>
>>> We have used the above check because of some SPEC benchmarks failing with
>>> with MEM pairs having REG_EQUAL/EQUIV notes.
>>>
>>> By adding the checks the benchmarks passes and also it improves the
>>> performance.
>>>
>>> This checks were added during initial implementation of pair fusion
>>> pass.
>>>
>>> I will investigate further if this check is still required or not.
>> 
>> Thanks.  If it does affect SPEC results, it would be good to look
>> at the underlying reason, as a justification for the check.
>> 
>> AIUI, the case Alex was due to the way that the RA recognises:
>> 
>>   (set (reg R) (mem address-of-a-stack-variable))
>>     REG_EQUIV: (mem address-of-a-stack-variable)
>> 
>> where the REG_EQUIV is either explicit or detected by the RA.
>> If R needs to be spilled, it can then be spilled to its existing
>> location on the stack.  And if R needs to be spilled in the
>> instruction above (because of register pressure before the first
>> use of R), the RA is able to delete the instruction.
>> 
>> But if that is the reason, the condition should be restricted
>> to cases in which the note is a memory.
>> 
>> I think Alex had tried something similar and found that it wasn't
>> always effective.
>> 
>
> Sure I will check.
>>> [...]
>>>>> +		   && 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.
>>>>
>>>
>>> Adjacent load pair fusion with 256 bit OOmode is seen and valid with use of load
>>> in UNSPEC. Thats why this check is added.
>> 
>> Can you give an example (in terms of rtl)?
>
>
> (insn 23 72 25 4 (set (reg:OO 124 [ vect__1.24 ])
>         (mem:OO (reg:DI 118 [ ivtmp.73 ]) [0 MEM <vector(16) unsigned char> [(unsigned char *)_65]+0 S16 A8]))  {*movoo}
>      (nil))
> (insn 25 23 103 4 (set (reg:OO 174 [ vect__1.26 ])
>         (mem:OO (plus:DI (reg:DI 118 [ ivtmp.73 ])
>                 (const_int 32 [0x20])) [0 MEM <vector(16) unsigned char> [(unsigned char *)_65 + 32B]+0 S16 A8]))  {*movoo}
>      (nil))
> (insn 103 25 27 4 (set (reg:DI 118 [ ivtmp.73 ])
>         (plus:DI (reg:DI 118 [ ivtmp.73 ])
>             (const_int 64 [0x40]))) 66 {*adddi3}
>      (nil))
> (insn 27 103 29 4 (set (reg:V16QI 176 [ vect_perm_even_123 ])
>         (unspec:V16QI [
>                 (subreg:V16QI (reg:OO 124 [ vect__1.24 ]) 0)
>                 (subreg:V16QI (reg:OO 124 [ vect__1.24 ]) 16)
>                 (reg:V16QI 300)
>             ] UNSPEC_VPERM))  {altivec_vperm_v16qi_direct}
>      (nil))
>
> This is the example where UNSPEC is used with 256 bit OOmode.

But what (conceptually) makes the vperm instruction interesting?
I.e. what is it about the instruction that makes you want to look for
it?

Is it that the instruction uses both halves of the fused load?  If so,
I don't think the test in the patch achieves that.  ISTM that the patch
checks both loads individually, and so would also match cases where one
load is used by one vperm and the other load is used by a different vperm.
Is that the intention?

It would also match cases where the vperm inputs are the other around,
so that the first operand is loaded from a higher address than the
second operand.  Is that ok, or is it a problem?

Similarly, there are other instructions that have unspecs, such as:

(define_insn "*insert4b_internal"
  [(set (match_operand:V16QI 0 "vsx_register_operand" "=wa")
	(unspec:V16QI [(match_operand:V4SI 1 "vsx_register_operand" "wa")
		       (match_operand:V16QI 2 "vsx_register_operand" "0")
		       (match_operand:QI 3 "const_0_to_12_operand" "n")]
		   UNSPEC_XXINSERTW))]

(to pick an arbitrary example).  It looks like the patch would also
accept uses by this instruction.  Is that intentional?

If it is intentional, what distinguishes things like vperm and xxinsertw
(and all other unspecs) from plain addition?

  [(set (match_operand:VSX_F 0 "vsx_register_operand" "=wa")
        (plus:VSX_F (match_operand:VSX_F 1 "vsx_register_operand" "wa")
		    (match_operand:VSX_F 2 "vsx_register_operand" "wa")))]

This is why the intention behind the patch is important.  As it stands,
it isn't clear what criteria the patch is using to distinguish "valid"
fuse candidates from "invalid" ones.

>>>>> [...]
>>>>> +  // 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.)
>>>>
>>>
>>> Yes rs6000 requires the original load insns to be in offset order.
>>> Some regression tests like vect-outer-4f fails if we do load pair
>>> fusion with load offsets are not in offset order as this breaks lxvp 
>>> semantics.
>> 
>> How does it break the semantics though?  In principle, the generic code
>> only fuses if it has "proved" that the loads can happen in either order.
>> So it shouldn't matter which order the hardware does things in.
>> 
>> Could you give an example of the kind of situation that you want
>> to avoid, and why it generates the wrong result?
>>
>
> (insn 31 62 32 2 (set (reg:V16QI 177 [ MEM <vector(8) short unsigned int> [(short unsigned int *)vectp.62_36 + 64B] ])
>         (mem:V16QI (plus:DI (reg/f:DI 121 [ vectp.62 ])
>                 (const_int 64 [0x40])) [1 MEM <vector(8) short unsigned int> [(short unsigned int *)vectp.62_36 + 64B]+0 S16 A16]))  {vsx_movv16qi_64bit}
>      (nil))
> (insn 32 31 16 2 (set (reg:V16QI 178 [ MEM <vector(8) short unsigned int> [(short unsigned int *)vectp.62_36 + 80B] ])
>         (mem:V16QI (plus:DI (reg/f:DI 121 [ vectp.62 ])
>                 (const_int 80 [0x50])) [1 MEM <vector(8) short unsigned int> [(short unsigned int *)vectp.62_36 + 80B]+0 S16 A16]))  {vsx_movv16qi_64bit}
>      (nil))
> (insn 16 32 21 2 (set (reg:V16QI 159 [ MEM <vector(8) short unsigned int> [(short unsigned int *)vectp.62_36 + 16B] ])
>         (mem:V16QI (plus:DI (reg/f:DI 121 [ vectp.62 ])
>                 (const_int 16 [0x10])) [1 MEM <vector(8) short unsigned int> [(short unsigned int *)vectp.62_36 + 16B]+0 S16 A16]))  {vsx_movv16qi_64bit}
>      (nil))
> (insn 21 16 22 2 (set (reg:V16QI 165 [ MEM <vector(8) short unsigned int> [(short unsigned int *)vectp.62_36 + 32B] ])
>         (mem:V16QI (plus:DI (reg/f:DI 121 [ vectp.62 ])
>                 (const_int 32 [0x20])) [1 MEM <vector(8) short unsigned int> [(short unsigned int *)vectp.62_36 + 32B]+0 S16 A16])) {vsx_movv16qi_64bit}
>      (nil))
> (insn 22 21 37 2 (set (reg:V16QI 166 [ MEM <vector(8) short unsigned int> [(short unsigned int *)vectp.62_36 + 48B] ])
>         (mem:V16QI (plus:DI (reg/f:DI 121 [ vectp.62 ])
>                 (const_int 48 [0x30])) [1 MEM <vector(8) short unsigned int> [(short unsigned int *)vectp.62_36 + 48B]+0 S16 A16])) {vsx_movv16qi_64bit}
>      (nil))
>
> insn 22 and insn 31 is merged in the failure case and breaks the code.

What specifically goes wrong though?  This is just a sequence of loads
from the same base pointer, with no interdependencies, so it should be
possible to do the loads in any order.

Thanks,
Richard