Optimize Neon HBD SAD functions - #911
Conversation
Optimize 10-bit and 12-bit pixel_sad_wxh_neon functions by accumulating into 16-bit vectors and widening only at the point of overflow.
Optimize 10-bit and 12-bit sad_x3_wxh_neon and sad_x4_wxh_neon functions by accumulating into 16-bit vectors and widening only at the point of overflow.
|
Could we get a review here please @chenm001? |
chenm001
left a comment
There was a problem hiding this comment.
Looks good to me, need smoke-test check before merge
chenm001
left a comment
There was a problem hiding this comment.
some code need adjustment
| ld1 {v0.8h-v2.8h}, [x0], x7 | ||
| ld1 {v3.8h-v5.8h}, [x1], x5 | ||
| \f v16.8h, v0.8h, v3.8h | ||
| uaba v16.8h, v1.8h, v4.8h |
There was a problem hiding this comment.
I don't like this code style. It mixed UABA and \f instructions, a little difficult readable.
I understand that \f may be replaced with UABD to reduce one zero-clearing instruction.
But there is a paradox here:
Low-end CPUs (such as A53/A55) in-order execute. This instruction has data dependencies and may cause a pipeline stall;
High-end CPUs out-of-order execute, so pre-zeroing register has almost no cost.
Another issue is that, to accommodate UABD, you split the loop into two segments, using UABD for one segment and UABA for the remaining part. The result is that there are additional instructions outside the loop, and the number of loop iterations is reduced by one. The actual runtime cache consumption and impact on branch prediction make the total number of cycles not necessarily less.
So, I still suggest zero v16-v19 in advance to maintain consistency in the code within the loop.
Another patch also has a similar issue, but it unrolls count is higher, so it's not as severe as this one, that one has more cache performance risk.
There was a problem hiding this comment.
This is the existing coding style that we're just adding to in this file - to change it would be to introduce inconsistency between the 8-bit and 10/12-bit implementations. What are the data dependencies introduced by UABD? This instruction is wholly destructive; thus breaking dependencies on previous values. Changing to MOVI #0 followed by UABA introduces an extra data dependency - particularly on in-order cores which don't have the ability to do zero-cost 0 initialization. When benchmarking your suggested change, we observe a ~7% regression on Neoverse V2 and a ~1% regression on Neoverse V3 platforms.
|
In current code, it is We may change to this format may reduce instruction outside of loop. We need do more benchmark for these different methods. |
mcw-Lavanya
left a comment
There was a problem hiding this comment.
Hi @alex-davicenko-arm
Thanks for the context on the 16-bit accumulate / late-widen approach.
We benchmarked master vs this PR using x265 TestBench on LLVM-22 / Neoverse V2, covering sad, sad_x3, and sad_x4 across all block sizes, on both 8-bit and high bit depth (10-bit) builds. We're not able to reproduce the claimed uplift (up to 20% for sad, up to 40% for sad_x3/sad_x4).
What we saw (master -> PR911, vector throughput):
- 8-bit: essentially flat, -2.45% to +3.75% (avg ~0%)
- High bit depth: -2.33% to +6.34%, avg well under 1%
The largest single gain we found anywhere was ~6.3%, and most sizes were within a couple percent of baseline either way.
Could you share the benchmarking setup and results behind the 20%/40% numbers - core pinning/governor settings, run count, and which sizes showed the largest gains? We'd like to make sure we're comparing apples to apples before drawing conclusions from the gap.
Thank you
|
Hi @mcw-Lavanya, what you observe on a Neoverse V2 is broadly what we see too:
However, also we observe that these patches make a significant difference on machines that have only two SIMD pipelines, most notably the Neoverse V3 - for which our original numbers are quoted. Please note that not all Neoverse V3 implementations will have 2 SIMD pipelines - the core can be configured to have 4[1] like the Neoverse V2. |
|
Hi @jwright-arm, Thank you |
Optimize 10-bit and 12-bit Neon SAD functions by accumulating into 16-bit vectors and widening only at the point of
overflow. This provides an uplift of up to 20% for pixel sad and 40% for sad_x3/sad_x4 when measured on a Neoverse V3 platform with LLVM-22.