mbox series

[FFmpeg-devel,v2,0/1] lavc/aarch64: add some neon pix_abs functions

Message ID 679fceeff0bc4fdca9e0fe17412d006c@EX13D07UWB004.ant.amazon.com
Headers show
Series lavc/aarch64: add some neon pix_abs functions | expand

Message

Swinney, Jonathan April 14, 2022, 4:22 p.m. UTC 
Thanks Martin for the review. I made some updates according to the suggestions you made.

I added a checkasm function, but I'm new to the test framework, so it may need some work still.

> Would it be beneficial to not do the addv here on each iteration, but 
> accumulate in v18.8h, and the just do one single addv at the end?

I did some testing with this, and it is, indeed, faster, however the problem I ran into is that the bounds for the loop can be higher than the capacity of a 16-bit unsigned integer used for accumulating in v16.8h. With the addv inside the loop, this register can hold the maximum possible value without overflowing before we increase the width of the field with the add instruction. If you have a trick or there are restrictions on the inputs that I am not aware of, let me know.

> This version most certainly would be slower indeed. If we could be ok with 
> doing a bit of overread, the simplest version might be to load e.g. "ld1 
> {v4.16b, v5.16b}, [x2]" followed by "ext v5.16b, v4.16b, v5.16b, #1" to 
> shift it. But doing an overlapping unaligned load probably is fine too.

I experimented with these instructions and found that it was marginally faster on Graviton 2, but it was slower on Graviton 3. Graviton 3 has more vector pipelines than Graviton 2 and is therefore a better candidate for video encoding workloads, so I chose to prefer Graviton 3. I didn't test it other chips.

Thank you for reviewing the patch!

Jonathan Swinney

Comments

Martin Storsjö April 15, 2022, 8:45 p.m. UTC | #1 
On Thu, 14 Apr 2022, Swinney, Jonathan wrote:

> Thanks Martin for the review. I made some updates according to the 
> suggestions you made.
>
> I added a checkasm function, but I'm new to the test framework, so it 
> may need some work still.

Thanks for putting in the effort to make a test - that adds a lot of value 
to the project! I'll follow up with a review of the newly added test.

>> Would it be beneficial to not do the addv here on each iteration, but
>> accumulate in v18.8h, and the just do one single addv at the end?
>
> I did some testing with this, and it is, indeed, faster, however the 
> problem I ran into is that the bounds for the loop can be higher than 
> the capacity of a 16-bit unsigned integer used for accumulating in 
> v16.8h. With the addv inside the loop, this register can hold the 
> maximum possible value without overflowing before we increase the width 
> of the field with the add instruction. If you have a trick or there are 
> restrictions on the inputs that I am not aware of, let me know.

Ok, I see! Can the accumulation be done with widening, i.e. uaddw v18.4s, 
v18.4s, v16.4h, uaddw2 v19.4s, v19.4s, v16.8h? (Or you could add both 
halves into the same v18.4s, but using two destination registers there 
probably helps.) That would keep the logic more SIMD-like while doing the 
flattening to one scalar value only once at the end.

>> This version most certainly would be slower indeed. If we could be ok with
>> doing a bit of overread, the simplest version might be to load e.g. "ld1
>> {v4.16b, v5.16b}, [x2]" followed by "ext v5.16b, v4.16b, v5.16b, #1" to
>> shift it. But doing an overlapping unaligned load probably is fine too.
>
> I experimented with these instructions and found that it was marginally 
> faster on Graviton 2, but it was slower on Graviton 3. Graviton 3 has 
> more vector pipelines than Graviton 2 and is therefore a better 
> candidate for video encoding workloads, so I chose to prefer Graviton 3. 
> I didn't test it other chips.

Ok - I guess the current implementation of that aspect is fine then!

// Martin
Martin Storsjö April 16, 2022, 8:46 p.m. UTC | #2 
On Fri, 15 Apr 2022, Martin Storsjö wrote:

> On Thu, 14 Apr 2022, Swinney, Jonathan wrote:
>
>> Thanks Martin for the review. I made some updates according to the 
>> suggestions you made.
>> 
>> I added a checkasm function, but I'm new to the test framework, so it may 
>> need some work still.
>
> Thanks for putting in the effort to make a test - that adds a lot of value to 
> the project! I'll follow up with a review of the newly added test.

Btw, for a guide on how checkasm tests, see 
https://ffmpeg.org/pipermail/ffmpeg-devel/2022-March/294400.html as a bit 
of a walkthrough.

// Martin