Tamar Christina <[email protected]> writes: >> -----Original Message----- >> From: Richard Sandiford <[email protected]> >> Sent: 08 September 2026 09:31 >> To: Tamar Christina <[email protected]> >> Cc: Alfie Richards <[email protected]>; [email protected]; Alex >> Coplan <[email protected]>; Alice Carlotti <[email protected]>; >> [email protected]; [email protected]; [email protected]; >> [email protected] >> Subject: Re: [PATCH v2 02/13] aarch64: Make reads/writes to FFR expensive >> >> 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. > > Sure, happy to do insn_costs if you think it's better, I asked to move it > since > doing it in insn_costs you do have to be careful about costing it inside a > parallel where you likely want the existing behavior of rtx_costs which takes > the maximum. > > So not hard to do, but looked like it was re-doing the work of rtx_costs.
But I think the idea is that insn_costs should use attributes, as the patch did. That takes the whole instruction into account, including any parallels. One of it's benefits is that it doesn't get caught overcosting unusually complex parallels. In other words, the problem IMO isn't that the patch was using attributes, but rather that sve_ffr would first need to be split into read and write halves. Thanks, Richard
