Kyrylo Tkachov <kyrylo.tkac...@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.sandif...@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.tkac...@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
