<div data-ntes="ntes_mail_body_root" style="line-height:1.7;color:#000000;font-size:14px;font-family:Arial"><div id="spnEditorContent"><p style="margin: 0;">Hi Hari,</p><p style="margin: 0;"><br></p><p style="margin: 0;">Thank for the update, it looks good to me now.</p><p style="margin: 0;"><br></p><p style="margin: 0;">For the DCT optimize, we may keep your version, if we write hand-write version future, we may try again.</p><p style="margin: 0;"><br></p></div><div style="position:relative;zoom:1"></div><div id="divNeteaseMailCard"></div><div style="margin: 0;">Regards,</div><div style="margin: 0;">Chen</div><pre><br>At 2024-08-27 23:10:18, "Hari Limaye" <hari.limaye@arm.com> wrote:
>Hi Chen,
>
>Thank you for reviewing the patches. Please see replies below - we have pushed this updated series to address your comments.
>
>>1. These function will share for 8/10/12 bpp, if move into X265_NS, it will make duplicated copy
>
>If I understand correctly, you are referring to the duplication that will occur when building the "Multi-library interface"? I think this duplication exists already in the current multi bitdepth build, its just that the symbols are defined with internal linkage. By moving things into the X265_NS we don't add any duplicated copies, although we do at least make the functions reference-able from other code. Also if I understand correctly, this should not be an issue for the C implementations - as when building the Multi-library interface you should set EXPORT_C_API=OFF for all but one of the builds.
>
>>2. add new section "namespace X265_NS" before these functions are better than move, it affects code history record.
>
>Updated patch to do this.
>
>>partialButterfly8_neon
>For size 8, butterfly E & O is necessary, but EE/EO is not a good idea, Odd spends 8 operators per line, Even spends 4 operators plus 1 temporary store and 2 prepare operators, totally 7 operators with dependency link, looks no performance benefits, especally SVE SVDOT may get more performance with Odd method.
>
>Regarding EE/EO not being a good idea in 8x8: register pressure is also a consideration here, as well as how the end of the first pass interacts with the start of the second (in terms of data positioning once they have been inlined - since everything stays in registers rather than being stored to/loaded from memory.) The current implementation performs better than the suggested implementations as computing EE and EO reduces register pressure (an issue 16-bit dot product cannot mitigate) and also allows the compiler to optimize more effectively across both passes after they have been inlined.
>
>>Code style mismatch in different code section, one line is better.
>>+        int32x4_t t01 = vpaddq_s32(vmull_s16(c1, O[j + 0]),
>>+                                   vmull_s16(c1, O[j + 1]));
>>*** vs
>>+        t01 = vpaddq_s32(vmull_s16(c3, O[j + 0]), vmull_s16(c3, O[j + 1]));
>
>The reason for the mismatch here is because the second line goes over the character limit of 80. This formatting is produced automatic formatting software (uncrustify).
>
>>dct8_neon
>>Better inline two of partialButterfly8_neon, it reduce some operators, such as int32x4_t c0 = vld1q_s32(t8_even[0]);
>>16x16 and 32x2 are similar
>
>The compiler currently inlines these - we have updated patches to be more explicit by using the `inline` keyword for the butterfly helper functions.
>
>>const table may share in between Neon and Sve code
>
>Updated with this done.
>
>Many thanks,
>Hari
>
>Hari Limaye (5):
>  Move C DCT implementations into X265_NS
>  AArch64: Move Neon DCT implementations into X265_NS
>  AArch64: Optimise partialButterfly8_neon
>  AArch64: Optimise partialButterfly16_neon
>  AArch64: Optimise partialButterfly32_neon
>
>Jonathan Wright (4):
>  AArch64: Move Neon-SVE bridge helpers into dedicated header
>  AArch64: Add SVE implementation of 8x8 DCT
>  AArch64: Add SVE implementation of 16x16 DCT
>  AArch64: Add SVE implementation of 32x32 DCT
>
> source/common/CMakeLists.txt             |   2 +-
> source/common/aarch64/asm-primitives.cpp |   1 +
> source/common/aarch64/dct-prim-sve.cpp   | 491 ++++++++++++++++++++
> source/common/aarch64/dct-prim.cpp       | 557 +++++++++++++++--------
> source/common/aarch64/dct-prim.h         |  40 ++
> source/common/aarch64/neon-sve-bridge.h  |  67 +++
> source/common/aarch64/sao-prim.h         |  32 +-
> source/common/dct.cpp                    |  22 +-
> 8 files changed, 980 insertions(+), 232 deletions(-)
> create mode 100644 source/common/aarch64/dct-prim-sve.cpp
> create mode 100644 source/common/aarch64/neon-sve-bridge.h
>
>-- 
>2.42.1
</pre></div>