AArch64: Improve DCT performance #901
Conversation
|
Could we get a review here please, @chenm001? |
|
Hi @georges-arm , thank you. |
chenm001
left a comment
There was a problem hiding this comment.
looks good to me, need smoke-test before merge
chenm001
left a comment
There was a problem hiding this comment.
looks good to me, need smoke-test before merge
chenm001
left a comment
There was a problem hiding this comment.
looks good to me, need smoke-test before merge
Hi @mcw-Lavanya , Benchmarks run on Neoverse V2, compiled with LLVM 22. Benchmark results from Before: After: |
|
Hi @georges-arm, |
mcw-Lavanya
left a comment
There was a problem hiding this comment.
Hi @georges-arm ,
According to this PR, with LLVM 21 and LLVM 20 the expected performance improvement is 7–12% for DCT16x16 and 0–9% for DCT32x32. Our LLVM 21 results are consistent with these expectations.
However, when testing with GCC, we observed significantly different behaviour:
GCC 14 shows much larger improvements than expected for both DCT16x16 and DCT32x32.
GCC 13 shows only about 2% improvement for DCT16x16, but DCT32x32 regresses by nearly 86%.
We wanted to understand whether the new intrinsic or code restructuring are expected to expose any performance weakness or compiler code-generation issue with older compilers (specifically GCC versions prior to 14), particularly for the DCT32x32 kernel. Has this behaviour been observed before, or would you expect GCC 13 to perform similarly to newer compilers?
Merge the outer loops to avoid the need to maintain O/EO/EEE/EEO arrays on the stack and consistently process four rows at a time. Also fix the `rev16` and `rev32` helper functions to use TBL instead of TBX (TBL has slightly better performance on some CPUs). Benchmarking on a range of Neoverse server platforms with LLVM 21, this change shows a reduction in observed time spent in DCT16 by 10-15%.
Merge the outer loops to avoid the need to maintain O/EO/EEE/EEO arrays on the stack and consistently process four rows at a time. Benchmarking on a range of Neoverse server platforms with LLVM 21, this change shows a reduction in observed time spent in DCT32 by 0-15%.
Merge the first and second pass functions to avoid duplication. Merge the outer loops to avoid the need to maintain O/EO/EEE/EEO arrays on the stack and consistently process four rows at a time. Benchmarking on a range of Neoverse server platforms with LLVM 22, this change shows a 0-9% reduction in observed time spent in Neon DCT16, and a 2-17% reduction in Neon DCT32. Since the Neon intrinsics implementation is now faster than the assembly implementation, update `setupDCTPrimitives_neon` to use the intrinsics version instead.
The `aarch64/dct.S` file contains assembly implementations of DCT16 and IDCT16, however neither of these are actually used in practice since the existing intrinsics-based versions are faster and therefore preferred. Since this code is unused, delete it from the library.
1c8e9cb to
e6bccb5
Compare
Hi @mcw-Lavanya , Thanks for the report. I have reproduced your performance with GCC 13 and adjusted the Neon implementation in Performance on a Neoverse V2 when built with LLVM 22, according to |
mcw-Lavanya
left a comment
There was a problem hiding this comment.
The updated patch seems to be fine and we are able to see good results across different llvm versions and gcc versions.
Improve the existing AArch64 Neon and SVE DCT 16x16 and 32x32 implementations:
For Neon: merge the first and second pass functions to avoid duplication.
For Neon and SVE: merge the outer loops to avoid the need to maintain
O/EO/EEE/EEOarrays on the stack and consistently process four rows at a time.Fix the
rev16andrev32helper functions to useTBLinstead ofTBXinstructions (TBLhas slightly better performance on some CPUs).Benchmarking on a range of Neoverse server platforms with LLVM 21, this change shows (depending on exact CPU micro-architecture):
With this change the Neon intrinsics implementation is now faster than the assembly implementation, so update
setupDCTPrimitives_neonto use the intrinsics version instead.The
aarch64/dct.Sfile contains assembly implementations of DCT16 and IDCT16, however these are now unused since the existing intrinsics-based versions are faster and therefore preferred. Since this code is unused, delete it from the library.