> -----Original Message-----
> From: Alfie Richards <[email protected]>
> Sent: 17 September 2026 15:40
> To: [email protected]
> Cc: Alex Coplan <[email protected]>; Alice Carlotti
> <[email protected]>; Andrea Pinski
> <[email protected]>; Richard Biener <[email protected]>;
> Tamar Christina <[email protected]>; Zdenek Dvorak <[email protected]>;
> Robin Dapp <[email protected]>; Richard Sandiford
> <[email protected]>; Kyrylo Tkachov <[email protected]>;
> Alfie Richards <[email protected]>
> Subject: [PATCH v3 12/16] aarch64: Make reads of FFR expensive
>
> With later changes to RDFFR to use subregisters for HSSR cases,
> forward prop begins to duplicate FFR reads as the cost penalties of
> reading the FFR register is lost.
>
> This change prevents that incorrect optimization.
Same as last time, do you have a testcase with FFR for which this is needed?
At the moment everyone says they don't know why it's needed, so this is hard
to approve without knowing why...
>
> Splits the sve_ffr sve_type instruction attibute into sve_ffr_write
> and sve_ffr_read to avoid penalizing sve_ffr_read's.
>
I don't think scheduling types should be used for determining RTX costs.
RTX costs should just use the given RTL. After all, they are an optional part
of an INSN whereas the use of the FFR register is not.
So did refers_to_regno_p (FFRT_REGNUM, x)) not work?
> gcc/ChangeLog:
>
> * config/aarch64/aarch64-sve.md:
> (aarch64_wrffr): Change sve_type attribute from sve_ffr to
> sve_ffr_write.
> (aarch64_rdffr): Change sve_type attribute from sve_ffr to
> sve_ffr_read.
> (aarch64_rdffr_z): Likewise.
> (aarch64_rdffr_z_ptest): Likewise.
> (aarch64_rdffr_ptest): Likewise.
> (aarch64_rdffr_z_cc): Likewise.
> (aarch64_rdffr_cc): Likewise.
> * config/aarch64/aarch64.cc (aarch64_insn_cost): Add logic
> to make sve_ffr_read's more expensive.
> * config/aarch64/aarch64.md (sve_type): Replace sve_ffr with
> sve_ffr_write and sve_ffr_read.
> * config/aarch64/neoversev2.md (neoversev2_dispatch): Replace
> sve_ffr with sve_ffr_read or sve_ffr_write.
> * config/aarch64/olympus.md (olympus_dispatch): Likewise.
> ---
> gcc/config/aarch64/aarch64-sve.md | 14 +++++++-------
> gcc/config/aarch64/aarch64.cc | 16 +++++++++++++---
> gcc/config/aarch64/aarch64.md | 2 +-
> gcc/config/aarch64/neoversev2.md | 4 ++--
> gcc/config/aarch64/olympus.md | 4 ++--
> 5 files changed, 25 insertions(+), 15 deletions(-)
>
> diff --git a/gcc/config/aarch64/aarch64-sve.md
> b/gcc/config/aarch64/aarch64-sve.md
> index 4b6d466b824..e7fe0b87978 100644
> --- a/gcc/config/aarch64/aarch64-sve.md
> +++ b/gcc/config/aarch64/aarch64-sve.md
> @@ -1141,7 +1141,7 @@ (define_insn "aarch64_wrffr"
> [ Dm ] setffr
> [ Upa ] wrffr\t%0.b
> }
> - [(set_attr "sve_type" "sve_ffr")]
> + [(set_attr "sve_type" "sve_ffr_write")]
> )
>
> ;; [L2 in the block comment above about FFR handling]
> @@ -1184,7 +1184,7 @@ (define_insn "aarch64_rdffr"
> (reg:VNx16BI FFRT_REGNUM))]
> "TARGET_SVE && TARGET_NON_STREAMING"
> "rdffr\t%0.b"
> - [(set_attr "sve_type" "sve_ffr")]
> + [(set_attr "sve_type" "sve_ffr_read")]
> )
>
> ;; Likewise with zero predication.
> @@ -1199,7 +1199,7 @@ (define_insn "aarch64_rdffr_z"
> [ ?Upa , 0Upa; yes ] ^
> [ Upa , Upa ; no ] ^
> }
> - [(set_attr "sve_type" "sve_ffr")]
> + [(set_attr "sve_type" "sve_ffr_read")]
> )
>
> ;; Read the FFR to test for a fault, without using the predicate result.
> @@ -1220,7 +1220,7 @@ (define_insn "*aarch64_rdffr_z_ptest"
> [ ?Upa , 0Upa; yes ] ^
> [ Upa , Upa ; no ] ^
> }
> - [(set_attr "sve_type" "sve_ffr")]
> + [(set_attr "sve_type" "sve_ffr_read")]
> )
>
> ;; Same for unpredicated RDFFR when tested with a known PTRUE.
> @@ -1239,7 +1239,7 @@ (define_insn "*aarch64_rdffr_ptest"
> [ ?Upa , 0Upa; yes ] ^
> [ Upa , Upa ; no ] ^
> }
> - [(set_attr "sve_type" "sve_ffr")]
> + [(set_attr "sve_type" "sve_ffr_read")]
> )
>
> ;; Read the FFR with zero predication and test the result.
> @@ -1263,7 +1263,7 @@ (define_insn "*aarch64_rdffr_z_cc"
> [ ?Upa , 0Upa; yes ] ^
> [ Upa , Upa ; no ] ^
> }
> - [(set_attr "sve_type" "sve_ffr")]
> + [(set_attr "sve_type" "sve_ffr_read")]
> )
>
> ;; Same for unpredicated RDFFR when tested with a known PTRUE.
> @@ -1283,7 +1283,7 @@ (define_insn "*aarch64_rdffr_cc"
> [ ?Upa , 0Upa; yes ] ^
> [ Upa , Upa ; no ] ^
> }
> - [(set_attr "sve_type" "sve_ffr")]
> + [(set_attr "sve_type" "sve_ffr_read")]
> )
>
> ;; [R3 in the block comment above about FFR handling]
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 00df040c8dc..51c72624bfe 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -17004,8 +17004,18 @@ aarch64_frame_allocation_cost
> (frame_cost_type,
> static int
> aarch64_insn_cost (rtx_insn *insn, bool speed)
> {
> + int extra_cost = 0;
> +
> + const sve_vec_cost *sve_costs = aarch64_tune_params.vec_costs->sve;
> +
You can have SVE code without an SVE cost model.
i.e. -mcpu=neoverse-n1+sve is perfectly valid. https://godbolt.org/z/PfMa38oMa
> + /* 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_WRITE)
> + extra_cost = sve_costs->read_ffr_cost;
> +
We differentiate between RTX costs and statement costs.
If you look at aarch64_rtx_costs and all other RTX costing hooks you'll see
costs are all scaled by COSTS_N_INSNS so it's a multiple of that base.
So this needs to be COSTS_N_INSNS (sve_costs->read_ffr_cost); to be consistent.
> if (rtx set = single_set (insn))
> - return set_rtx_cost (set, speed);
> + return set_rtx_cost (set, speed) + extra_cost;
>
> /* If the instruction does multiple sets in parallel, use the cost
> of the most expensive set. This copes with instructions that set
> @@ -17020,10 +17030,10 @@ aarch64_insn_cost (rtx_insn *insn, bool
> speed)
> if (GET_CODE (x) == SET)
> max_cost = std::max (max_cost, set_rtx_cost (x, speed));
> }
> - return max_cost;
> + return max_cost + extra_cost;
> }
This is weird to me, This means that if the INSN is a parallel, and it's
already
very expensive, we make it extra super expensive. This seems odd compared
to how we normally deal with expensive instructions in parallels, which is to
take the maxmum.
So I think this and the below should be std::max (max_cost, extra_cost);
But you must have a testcase that shows the costing in action no?
Thanks,
Tamar
>
> - return pattern_cost (pat, speed);
> + return pattern_cost (pat, speed) + extra_cost;
> }
>
> /* Implement TARGET_INIT_BUILTINS. */
> diff --git a/gcc/config/aarch64/aarch64.md
> b/gcc/config/aarch64/aarch64.md
> index 302658c970d..932fec20f8c 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -700,7 +700,7 @@ (define_attr "sve_type"
> sve_gatherload_32,sve_gatherload_64,\
>
> sve_store_pred,sve_store_1reg,sve_store_2reg,sve_store_3reg,sve_store_4re
> g,\
> sve_scatterstore_32,sve_scatterstore_64,\
> - sve_ffr,\
> + sve_ffr_write, sve_ffr_read,\
> sve_crypto_sha3"
> (const_string "none"))
>
> diff --git a/gcc/config/aarch64/neoversev2.md
> b/gcc/config/aarch64/neoversev2.md
> index 8dc9b098d09..2cfa39a72fe 100644
> --- a/gcc/config/aarch64/neoversev2.md
> +++ b/gcc/config/aarch64/neoversev2.md
> @@ -49,7 +49,7 @@ (define_attr "neoversev2_dispatch"
> (ior
> (eq_attr "type" "crc,f_cvti2f,mla,neon_from_gp,neon_from_gp_q,\
> sdiv,smlal,udiv,umlal")
> - (eq_attr "sve_type" "sve_ffr,sve_pred_logical"))
> + (eq_attr "sve_type" "sve_ffr_read,sve_ffr_write,sve_pred_logical"))
> (const_string "m0")
> (ior
> (eq_attr "type"
> @@ -189,4 +189,4 @@ (define_attr "neoversev2_dispatch"
> (eq_attr "sve_type" "sve_scatterstore_32,sve_scatterstore_64,\
> sve_store_1reg,sve_store_2reg,sve_store_3reg,sve_store_4reg"))
> (const_string "l01_v")]
> - (const_string "none")))
> \ No newline at end of file
> + (const_string "none")))
> diff --git a/gcc/config/aarch64/olympus.md
> b/gcc/config/aarch64/olympus.md
> index 22b12016ffd..9c1390deef7 100644
> --- a/gcc/config/aarch64/olympus.md
> +++ b/gcc/config/aarch64/olympus.md
> @@ -46,7 +46,7 @@ (define_attr "olympus_dispatch"
> (eq_attr "sve_type" "sve_pred_cnt_ctrl,sve_pred_cnt_scalar,\
> sve_pred_logical,sve_pred_misc"))
> (const_string "m")
> - (eq_attr "sve_type" "sve_ffr")
> + (eq_attr "sve_type" "sve_ffr_write,sve_ffr_read")
> (const_string "m0")
> (ior
> (eq_attr "type" "f_loadd,f_loads,load_4,load_8,load_16,\
> @@ -196,4 +196,4 @@ (define_attr "olympus_dispatch"
> (eq_attr "sve_type"
> "sve_store_2reg,sve_store_3reg,sve_store_4reg,\
> sve_scatterstore_32,sve_scatterstore_64"))
> (const_string "sa_v_v0123")]
> - (const_string "none")))
> \ No newline at end of file
> + (const_string "none")))
> --
> 2.43.0
