> -----Original Message----- > From: Kyrylo Tkachov <ktkac...@nvidia.com> > Sent: Monday, July 21, 2025 11:36 AM > To: Tamar Christina <tamar.christ...@arm.com> > Cc: GCC Patches <gcc-patches@gcc.gnu.org>; Richard Sandiford > <richard.sandif...@arm.com>; Andrew Pinski <pins...@gmail.com>; Alex Coplan > <alex.cop...@arm.com>; Remi Machet <rmac...@nvidia.com>; Jennifer Schmitz > <jschm...@nvidia.com> > Subject: Re: [PATCH 2/2] aarch64: Allow CPU tuning to avoid INS-(W|X)ZR > instructions > > > > > On 21 Jul 2025, at 11:43, Kyrylo Tkachov <ktkac...@nvidia.com> wrote: > > > > Hi Tamar, > > > >> On 21 Jul 2025, at 11:12, Tamar Christina <tamar.christ...@arm.com> wrote: > >> > >> Hi Kyrill, > >> > >>> -----Original Message----- > >>> From: Kyrylo Tkachov <ktkac...@nvidia.com> > >>> Sent: Friday, July 18, 2025 10:40 AM > >>> To: GCC Patches <gcc-patches@gcc.gnu.org> > >>> Cc: Tamar Christina <tamar.christ...@arm.com>; Richard Sandiford > >>> <richard.sandif...@arm.com>; Andrew Pinski <pins...@gmail.com>; Alex > Coplan > >>> <alex.cop...@arm.com> > >>> Subject: [PATCH 2/2] aarch64: Allow CPU tuning to avoid INS-(W|X)ZR > >>> instructions > >>> > >>> Hi all, > >>> > >>> For inserting zero into a vector lane we usually use an instruction like: > >>> ins v0.h[2], wzr > >>> > >>> This, however, has not-so-great performance on some CPUs. > >>> On Grace, for example it has a latency of 5 and throughput 1. > >>> The alternative sequence: > >>> movi v31.8b, #0 > >>> ins v0.h[2], v31.h[0] > >>> is prefereble bcause the MOVI-0 is often a zero-latency operation that is > >>> eliminated by the CPU frontend and the lane-to-lane INS has a latency of > >>> 2 and > >>> throughput of 4. > >>> We can avoid the merging of the two instructions into the > >>> aarch64_simd_vec_set_zero<mode> > >>> insn through rtx costs. We just need to handle the right VEC_MERGE form > >>> in > >>> aarch64_rtx_costs. The new CPU-specific cost field ins_gp is introduced to > describe > >>> this operation. > >>> According to a similar LLVM PR: https://github.com/llvm/llvm- > >>> project/pull/146538 > >>> and looking at some Arm SWOGs I expect the Neoverse-derived cores to > benefit > >>> from this, > >>> whereas little cores like Cortex-A510 won't (INS-WZR has a respectable > >>> latency > >>> 3 in Cortex-A510). > >>> > >> > >> I'm not really sure about the Cortex-A510, the entries are a bit suspect. > >> I think > they > >> haven't added an entry for INS from GPR, which is an alias for MOV element, > GPR. > >> And the throughput for the INS they did add (element to element) seems > suspect. > >> > >> If I assume the same latency as an FMOV for INS GPR Wzr, then yeah the MOVI > is > >> slower (at least on paper) but I'd expect in actual code the throughput to > >> matter > more. > >> > >> So I think we should just do this for every core and by default. I have a > >> couple > of > >> arguments for this: > >> > >> 1. We already do this for every constant but 0, since we prefer the > >> throughput > >> advantage here. See https://godbolt.org/z/9PKhEax8G where if you change > your > >> testcase to anything but 0 you get the movi. Even though strictly > >> speaking the > movi > >> is more expensive than the mov. > >> > >> 2. I've ran this sequence change over various cores, and they all showed > >> either > an improvement > >> or very little difference. i.e. Cortex-A510 shows > >> > >> cortex-a510 │ little │ -0.1% > >> cortex-a520 │ little │ -0.09% > >> cortex-a710 │ mid │ -0.03% > >> > >> etc, this is comparing using movi over the INS, wzr. > > > > Thanks for the broader benchmarking! > > > > > >> > >> However the advantage grows significantly when there is more than one of > >> the > same constant to insert. > >> e.g. > >> > >> __attribute__((noipa)) > >> uint16x8_t foo2(uint16x8_t a) { > >> a[2] = 0; > >> a[5] = 0; > >> return a; > >> } > >> > >> Now the movi pays for itself and the INS wzr is serial over most cores. > >> > >> Benchmarking this gives you as expected for inorder cores: > >> > >> cortex-a510 │ little │ -0.1% > >> cortex-a520 │ little │ -0.05% > >> but then out of order cores > >> > >> cortex-a710 │ mid │ ✔ -49.49% > >> neoverse v2 │ big │ ✔ -49.57% > >> > >> etc. > >> > >> Showing that I think we should do this by default. > > > > Yeah that’s fine with me. > > > > > >> > >> As a side note, the current patch only seems to work outside loops. e.g. > >> > >> __attribute__((noipa)) > >> uint16x8_t foos(uint16x8_t a, int n) { > >> for (int i = 0; i < n; i++) > >> a[2] = 0; > >> return a; > >> } > >> > >> Does not seem to make the movi and I think that's because after rejecting > >> the movi in the vec_merge and floating it in the loop preheader, combine > >> now > >> tries to force it down and ultimately succeeds. > >> > >> Given the above, what do you think about instead just disabling the > >> aarch64_simd_vec_set_zero pattern when not compiling for size? > >> > > > > Yes, that would make for a simpler patch and wouldn’t rely on the rtx cost > > users > doing the right thing. > > I’ll respin accordingly. > > This patch passes bootstrap and test on aarch64-none-linux-gnu.
Thanks! Ok with me unless someone disagrees. Cheers, Tamar > Thanks, > Kyrill > > > > Thanks, > > Kyrill > > > >> That should do what we want in all cases. > >> > >> Thanks, > >> Tamar > >> > >>> Practically, a value of COSTS_N_INSNS (2) and higher for ins_gp causes > >>> the split > >>> into two instructions, values lower than that retain the INS-WZR form. > >>> cortexa76_extra_costs, from which Grace and many other Neoverse cores > derive > >>> from, > >>> sets ins_gp to COSTS_N_INSNS (3) to reflect a latency of 5 cycles. 3 is > >>> the > number > >>> of cycles above the normal cheapest SIMD instruction on such cores (which > take 2 > >>> cycles > >>> for the cheapest one). > >>> > >>> cortexa53_extra_costs and all other costs set ins_gp to COSTS_N_INSNS (1) > >>> to > >>> preserve the current codegen, though I'd be happy to increase it for > >>> generic > tuning. > >>> > >>> For -Os we don't add any extra cost so the shorter INS-WZR form is still > >>> generated always. > >>> > >>> Bootstrapped and tested on aarch64-none-linux-gnu. > >>> Ok for trunk? > >>> Thanks, > >>> Kyrill > >>> > >>> Signed-off-by: Kyrylo Tkachov <ktkac...@nvidia.com> > >>> > >>> gcc/ > >>> > >>> * config/arm/aarch-common-protos.h (vector_cost_table): Add ins_gp > >>> field. Add comments to other vector cost fields. > >>> * config/aarch64/aarch64.cc (aarch64_rtx_costs): Handle VEC_MERGE > >>> case. > >>> * config/aarch64/aarch64-cost-tables.h (qdf24xx_extra_costs, > >>> thunderx_extra_costs, thunderx2t99_extra_costs, > >>> thunderx3t110_extra_costs, tsv110_extra_costs, a64fx_extra_costs, > >>> ampere1_extra_costs, ampere1a_extra_costs, ampere1b_extra_costs): > >>> Specify ins_gp cost. > >>> * config/arm/aarch-cost-tables.h (generic_extra_costs, > >>> cortexa53_extra_costs, cortexa57_extra_costs, cortexa76_extra_costs, > >>> exynosm1_extra_costs, xgene1_extra_costs): Likewise. > >>> > >>> gcc/testsuite/ > >>> > >>> * gcc.target/aarch64/simd/mf8_data_1.c (test_set_lane4, > >>> test_setq_lane4): Relax allowed assembly. >
