Tamar Christina <[email protected]> writes:
>> -----Original Message-----
>> From: Alfie Richards <[email protected]>
>> Sent: 08 August 2026 00:11
>> To: [email protected]
>> Cc: Alex Coplan <[email protected]>; Alice Carlotti
>> <[email protected]>; [email protected];
>> [email protected]; Tamar Christina <[email protected]>;
>> [email protected]; [email protected]; [email protected]; Alfie
>> Richards <[email protected]>
>> Subject: [PATCH v2 02/13] aarch64: Make reads/writes to FFR expensive
>>
>> With later changes to RDFFR to use subregisters we see forward prop
>> begin to duplicate FFR reads. This change prevents that.
>>
>
> Do you have a testcase that shows this?
>
>> gcc/ChangeLog:
>>
>> * config/aarch64/aarch64.cc (aarch64_insn_cost): Add extra costs
>> for FFR reads/writes.
>> ---
>> gcc/config/aarch64/aarch64.cc | 14 +++++++++++---
>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
>> index 3041a6ee62a..e7522e49efd 100644
>> --- a/gcc/config/aarch64/aarch64.cc
>> +++ b/gcc/config/aarch64/aarch64.cc
>> @@ -16989,8 +16989,16 @@ aarch64_frame_allocation_cost
>> (frame_cost_type,
>> static int
>> aarch64_insn_cost (rtx_insn *insn, bool speed)
>> {
>> + int extra_cost = 0;
>> +
>> + /* Reads and sets to FFR are expensive. Mark them as such to prevent
>> + duplication. */
>> + if (recog_memoized (insn) != -1
>> + && get_attr_sve_type (insn) == SVE_TYPE_SVE_FFR)
>> + extra_cost = 100;
>> +
>
> That seems quite expensive. Does it really need to be that much?
> A testcase would be nice :)
>
> I can certainly believe the cost of RDFFR should not be the default cost,
> but I think this is in the wrong place and it shouldn't use the scheduling
> type
> attributes. The above for instance also makes setffr more expensive.
Agreed that this should be unidirectional FWIW. It's unfortunate that
the (already high) move costs aren't enough for move instructions,
and that we need this other mechanism as well.
> Instead I think this should be in aarch64_rtx_costs in the branch that's
> costing a full INSN, (i.e. the case SET: ) but to identify an rdffr as an
> instruction with a predicate destination and FFRT_REGNUM mentioned
> in the source.
>
> Then just use the normal COSTS_N_INSN macro to scale costs to insn costs
> and use the same turning param you introduce later on. So something like
>
> static int
> aarch64_ffr_op_cost (rtx set)
> {
> if (GET_CODE (set) != SET)
> return 0;
>
> rtx dest = SET_DEST (set);
> rtx x = SET_SRC (set);
>
> if (GET_CODE (x) == UNSPEC
> && XINT (x, 1) == UNSPEC_PTEST
> && refers_to_regno_p (FFRT_REGNUM, x))
> return COSTS_N_INSNS (aarch64_tune_params.vec_costs->sve->rdffr_cost);
>
> if (REG_P (dest)
> && aarch64_sve_pred_mode_p (GET_MODE (dest))
> && refers_to_regno_p (FFRT_REGNUM, x))
> return COSTS_N_INSNS (aarch64_tune_params.vec_costs->sve->rdffr_cost);
>
> return 0;
> }
FWIW, I think insn_costs is preferred over rtx_costs for this kind of thing.
The hook was specifically added to work around problems with rtx_costs,
and also to enable attributes to be used in preference to manually
reproducing parts of genrecog.
Thanks,
Richard
