> -----Original Message-----
> From: Kyrylo Tkachov <ktkac...@nvidia.com>
> Sent: Wednesday, August 28, 2024 8:55 AM
> To: Tamar Christina <tamar.christ...@arm.com>
> Cc: Richard Sandiford <richard.sandif...@arm.com>; Jennifer Schmitz
> <jschm...@nvidia.com>; gcc-patches@gcc.gnu.org; Kyrylo Tkachov
> <ktkac...@exchange.nvidia.com>
> Subject: Re: [RFC][PATCH] AArch64: Remove
> AARCH64_EXTRA_TUNE_USE_NEW_VECTOR_COSTS
>
> Hi all,
>
> Thanks to Jennifer for proposing a patch and Tamar and Richard for digging
> into it.
>
> > On 27 Aug 2024, at 13:16, Tamar Christina <tamar.christ...@arm.com> wrote:
> >
> > External email: Use caution opening links or attachments
> >
> >
> >> -----Original Message-----
> >> From: Richard Sandiford <richard.sandif...@arm.com>
> >> Sent: Tuesday, August 27, 2024 11:46 AM
> >> To: Tamar Christina <tamar.christ...@arm.com>
> >> Cc: Jennifer Schmitz <jschm...@nvidia.com>; gcc-patches@gcc.gnu.org; Kyrylo
> >> Tkachov <ktkac...@exchange.nvidia.com>
> >> Subject: Re: [RFC][PATCH] AArch64: Remove
> >> AARCH64_EXTRA_TUNE_USE_NEW_VECTOR_COSTS
> >>
> >> Tamar Christina <tamar.christ...@arm.com> writes:
> >>> Hi Jennifer,
> >>>
> >>>> -----Original Message-----
> >>>> From: Jennifer Schmitz <jschm...@nvidia.com>
> >>>> Sent: Friday, August 23, 2024 1:07 PM
> >>>> To: gcc-patches@gcc.gnu.org
> >>>> Cc: Richard Sandiford <richard.sandif...@arm.com>; Kyrylo Tkachov
> >>>> <ktkac...@exchange.nvidia.com>
> >>>> Subject: [RFC][PATCH] AArch64: Remove
> >>>> AARCH64_EXTRA_TUNE_USE_NEW_VECTOR_COSTS
> >>>>
> >>>> This patch removes the AARCH64_EXTRA_TUNE_USE_NEW_VECTOR_COSTS
> >>>> tunable and
> >>>> use_new_vector_costs entry in aarch64-tuning-flags.def and makes the
> >>>> AARCH64_EXTRA_TUNE_USE_NEW_VECTOR_COSTS paths in the backend
> the
> >>>> default.
> >>
> >> Thanks for doing this. This has been on my TODO list ever since the
> >> tunable was added.
> >>
> >> The history is that these "new" costs were originally added in stage 4
> >> of GCC 11 for Neoverse V1. Since the costs were added so late, it wasn't
> >> appropriate to change the behaviour for any other core. All the new code
> >> was therefore gated on this option.
> >>
> >> The new costs do two main things:
> >>
> >> (1) use throughput-based calculations where available, including to choose
> >> between Advanced SIMD and SVE
> >>
> >> (2) try to make the latency-based costs more precise, by looking more
> >> closely
> >> at the provided stmt_info
> >>
> >> Old cost models won't be affected by (1) either way, since they don't
> >> provide any throughput information. But they should in principle benefit
> >> from (2). So...
> >>
> >>>> To that end, the function aarch64_use_new_vector_costs_p and its uses
> were
> >>>> removed. Additionally, guards were added prevent nullpointer dereferences
> of
> >>>> fields in cpu_vector_cost.
> >>>>
> >>>
> >>> I'm not against this change, but it does mean that we now switch old Adv.
> SIMD
> >>> cost models as well to the new throughput based cost models. That means
> that
> >>> -mcpu=generic now behaves differently, and -mcpu=neoverse-n1 and I think
> >>> some distros explicitly use this (I believe yocto for instance does).
> >>
> >> ...it shouldn't mean that we start using throughput-based models for
> >> cortexa53 etc., since there's no associated issue info.
> >
> > Yes, I was using throughput based model as a name. But as you indicated in
> > (2)
> > it does change the latency calculation.
> >
> > My question was because of things in e.g. aarch64_adjust_stmt_cost and
> friends,
> > e.g. aarch64_multiply_add_p changes the cost between FMA SIMD vs scalar.
> >
> > So my question..
> >
> >>
> >>> Have we validated that the old generic cost model still behaves sensibly
> >>> with
> this
> >> change?
> >
> > is still valid I think, we *are* changing the cost for all models,
> > and while they should indeed be more accurate, there could be knock on
> > effects.
> >
>
> We can run SPEC on a Grace system with -mcpu=generic to see what the effect
> is,
> but wider benchmarking would be more appropriate. Can you help with that
> Tamar once we agree on the other implementation details in this patch?
>
Sure that's not a problem. Just ping me when you have a patch you want me to
test :)
>
> > Thanks,
> > Tamar
> >
> >>>
> >>>> The patch was bootstrapped and regtested on aarch64-linux-gnu:
> >>>> No problems bootstrapping, but several test files (in aarch64-sve.exp:
> >>>> gather_load_extend_X.c
> >>>> where X is 1 to 4, strided_load_2.c, strided_store_2.c) fail because of
> >>>> small
> >>>> differences
> >>>> in codegen that make some of the scan-assembler-times tests fail.
> >>>>
> >>>> Kyrill suggested to add a -fvect-cost-model=unlimited flag to these
> >>>> tests and
> >> add
> >>>> some
> >>>
> >>> I don't personally like unlimited here as unlimited means just vectorize
> >>> at any
> >>> cost. This means that costing between modes are also disabled. A lot of
> >>> these
> >>> testcases are intended to test not just that we vectorize but that we
> >>> vectorize
> >>> with efficient code.
> >>>
> >>> I'd prefer to use -fvect-cost-model=dynamic if that fixes the testcases.
> >>
> >> Yeah, I don't think -fvect-cost-model=unlimited would work for the
> >> gather_load_extend_X.c tests, since we require the cost model to decide
> >> when to use extending loads vs loading a full vector and unpacking.
>
> I had suggested using -fvect-cost-model=unlimited here because I thought these
> tests wanted to test the capability of GCC to detect and generate the
> particular SVE
> feature (gather load extends) for all supported data types regardless of
> profitability.
I think the problem only specifically for the load_extend tests, because
unlimited also
disables SVE vs SVE comparisons wrt to changes to VF, so not just Adv. SIMD vs
SVE.
That is while it would generate a gather, it may choose to instead of gather
from
B -> D, do B -> S and then extend the results. Because this has a higher VF.
Without the cross SVE mode comparisons it wouldn't know that the extensions
would
actually slow it down.
> If the tests are intended to also make the right profitability decisions for
> the generic
> tuning model then I agree using -fvect-cost-model=unlimited is not appropriate
> here, though I do think that it’d be useful to fix the backend vector cost
> model
> hooks to honor -fvect-cost-model=unlimited and not limit generation of
> gather/scatter in that case. What it should do for the SVE vs Neon decisions
> is an
> open question.
>
>
> >>
> >> [...tries patch...]
> >>
> >> It seems like three main things are contributing to the difference:
> >>
> >> 1. we now cost 0 for a scalar prologue extension from a loaded value
> >> 2. we now cost gathers & scatters based on gather_load_x32/x64_cost
> >> 3. we now apply a large one-off cost for gathers (Tamar's recent change)
> >>
> >> (1) is expected.
> >>
> >> (2) partly looks like a latent bug. We're using the x32 cost for
> >> VNx2QI->VNx2SI, even though those are really .B->.D loads.
> >>
> >> @@ -16819,7 +16811,7 @@ aarch64_detect_vector_stmt_subtype (vec_info
> >> *vinfo, vect_cost_for_stmt kind,
> >> && STMT_VINFO_MEMORY_ACCESS_TYPE (stmt_info) ==
> >> VMAT_GATHER_SCATTER)
> >> {
> >> unsigned int nunits = vect_nunits_for_cost (vectype);
> >> - if (GET_MODE_UNIT_BITSIZE (TYPE_MODE (vectype)) == 64)
> >> + if (known_eq (GET_MODE_NUNITS (TYPE_MODE (vectype)),
> >> aarch64_sve_vg))
> >> return { sve_costs->gather_load_x64_cost, nunits };
> >> return { sve_costs->gather_load_x32_cost, nunits };
> >> }
> >>
> >> fixes that.
>
> Would you be willing to test and push that to trunk to get it out of the way?
>
>
> >>
> >> (3) is interesting. generic_vector_cost isn't used by default for any
> >> SVE CPU, or any -march=...+sve. So the question is: should we treat it
> >> as "architectural intent"/somewhat idealised? Or should we try to make
> >> it produce good code for existing SVE cores, in which case it would
> >> overlap quite a lot with generic_armv8_a and generic_armv9_a.
>
> >>
> >> If the former, we could set gather_load_x32_init_cost and
> >> gather_load_x64_init_cost to 0 for generic_sve_vector_cost
> >> (and nothing else, so that generic_armv*_a are unaffected).
>
> I don’t have strong opinions on this point but if Tamar is concerned about
> deviating too much from the known-good Advanced SIMD generic tuning we have
> now then we should aim for minimal codegen changes in that?
No I'm fine with Richard's suggestion. The only way you'd be able to get this
model
to fire is with -march=xxx=sve -mtune=generic, at which point I'm fine with
assuming
gathers have no initial overhead.
Cheers,
Tamar
> Thanks again,
> Kyrill
>
>
> >>
> >> On the patch:
> >>
> >>> @@ -16733,7 +16723,8 @@ aarch64_in_loop_reduction_latency (vec_info
> >> *vinfo, stmt_vec_info stmt_info,
> >>> {
> >>> const cpu_vector_cost *vec_costs = aarch64_tune_params.vec_costs;
> >>> const sve_vec_cost *sve_costs = nullptr;
> >>> - if (vec_flags & VEC_ANY_SVE)
> >>> + if (vec_costs->sve
> >>> + && vec_flags & VEC_ANY_SVE)
> >>> sve_costs = aarch64_tune_params.vec_costs->sve;
> >>
> >> This doesn't seem necessary. I agree we might as well reuse the
> >> vec_costs local variable in the "if" body though.
> >>
> >>> [...]
> >>> @@ -16794,9 +16785,10 @@ aarch64_detect_vector_stmt_subtype
> (vec_info
> >> *vinfo, vect_cost_for_stmt kind,
> >>> fractional_cost stmt_cost)
> >>> {
> >>> const simd_vec_cost *simd_costs = aarch64_simd_vec_costs (vectype);
> >>> + const cpu_vector_cost *vec_costs = aarch64_tune_params.vec_costs;
> >>> const sve_vec_cost *sve_costs = nullptr;
> >>> - if (aarch64_sve_mode_p (TYPE_MODE (vectype)))
> >>> - sve_costs = aarch64_tune_params.vec_costs->sve;
> >>> + if (aarch64_sve_mode_p (TYPE_MODE (vectype)) && vec_costs->sve)
> >>> + sve_costs = vec_costs->sve;
> >>
> >> Similarly here, this doesn't seem necessary.
> >>
> >>> [...]
> >>> @@ -17301,11 +17293,13 @@ aarch64_vector_costs::add_stmt_cost (int
> >> count, vect_cost_for_stmt kind,
> >>> where, stmt_cost);
> >>
> >>> /* Check if we've seen an SVE gather/scatter operation and which
> >>> size. */
> >>> + const cpu_vector_cost *vec_costs = aarch64_tune_params.vec_costs;
> >>> if (kind == scalar_load
> >>> && aarch64_sve_mode_p (TYPE_MODE (vectype))
> >>> - && STMT_VINFO_MEMORY_ACCESS_TYPE (stmt_info) ==
> >> VMAT_GATHER_SCATTER)
> >>> + && STMT_VINFO_MEMORY_ACCESS_TYPE (stmt_info) ==
> >> VMAT_GATHER_SCATTER
> >>> + && vec_costs->sve)
> >>> {
> >>> - const sve_vec_cost *sve_costs = aarch64_tune_params.vec_costs->sve;
> >>> + const sve_vec_cost *sve_costs = vec_costs->sve;
> >>> if (sve_costs)
> >>> {
> >>> if (GET_MODE_UNIT_BITSIZE (TYPE_MODE (vectype)) == 64)
> >>
> >> Here too.
> >>
> >> Thanks,
> >> Richard
