Re: [PATCH][1/4][committed] aarch64: Add support for Armv8.8-a memory operations and memcpy expansion

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

Kyrylo Tkachov <Kyrylo.Tkachov@arm.com> writes:
> Hi Richard,
>
> Sorry for the delay in getting back to this. I'm now working on a patch to adjust this.
>
>> -----Original Message-----
>> From: Richard Sandiford <richard.sandiford@arm.com>
>> Sent: Tuesday, December 14, 2021 10:48 AM
>> To: Kyrylo Tkachov via Gcc-patches <gcc-patches@gcc.gnu.org>
>> Cc: Kyrylo Tkachov <Kyrylo.Tkachov@arm.com>
>> Subject: Re: [PATCH][1/4][committed] aarch64: Add support for Armv8.8-a
>> memory operations and memcpy expansion
>> 
>> Kyrylo Tkachov via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> > @@ -23568,6 +23568,28 @@
>> aarch64_copy_one_block_and_progress_pointers (rtx *src, rtx *dst,
>> >    *dst = aarch64_progress_pointer (*dst);
>> >  }
>> >
>> > +/* Expand a cpymem using the MOPS extension.  OPERANDS are taken
>> > +   from the cpymem pattern.  Return true iff we succeeded.  */
>> > +static bool
>> > +aarch64_expand_cpymem_mops (rtx *operands)
>> > +{
>> > +  if (!TARGET_MOPS)
>> > +    return false;
>> > +  rtx addr_dst = XEXP (operands[0], 0);
>> > +  rtx addr_src = XEXP (operands[1], 0);
>> > +  rtx sz_reg = operands[2];
>> > +
>> > +  if (!REG_P (sz_reg))
>> > +    sz_reg = force_reg (DImode, sz_reg);
>> > +  if (!REG_P (addr_dst))
>> > +    addr_dst = force_reg (DImode, addr_dst);
>> > +  if (!REG_P (addr_src))
>> > +    addr_src = force_reg (DImode, addr_src);
>> > +  emit_insn (gen_aarch64_cpymemdi (addr_dst, addr_src, sz_reg));
>> > +
>> > +  return true;
>> > +}
>> 
>> On this, I think it would be better to adjust the original src and dst
>> MEMs if possible, since they contain metadata about the size of the
>> access and alias set information.  It looks like the code above
>> generates an instruction with a wild read and a wild write instead.
>> 
>
> Hmm, do you mean adjust the address of the MEMs in operands with something like replace_equiv_address_nv?

Yeah.

>> It should be possible to do that with a define_expand/define_insn
>> pair, where the define_expand takes two extra operands for the MEMs,
>> but the define_insn contains the same operands as now.
>
> Could you please expand on this a bit? This path is reached from the cpymemdi expander that already takes the two MEMs as operands and generates the aarch64_cpymemdi define_insn that uses just the address registers as operands. Should we carry the MEMs around in the define_insn as well after expand?

It could be a second expander.  E.g.:

(define_expand "aarch64_cpymemdi"
  [(parallel
     [(set (match_operand 2) (const_int 0))
      (clobber (match_operand 0))
      (clobber (match_operand 1))
      (set (match_operand 3)
           (unspec:BLK [(match_operand 4) (match_dup 2)] UNSPEC_CPYMEM))])]
  "TARGET_MOPS"
)

with the existing define_insn maybe becoming *aarch64_cpymemdi.
(The define_insn doesn't need the outer parallel.)

Thanks,
Richard