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?
> 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.
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?
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
