[FFmpeg-devel] [PATCH] libswscale/aarch64/hscale.S: Support more bit-depth variants.

[Reimar Döffinger]

> On 15 Jan 2021, at 22:10, Martin Storsjö <martin at martin.st> wrote:
> 
> On Mon, 11 Jan 2021, Reimar.Doeffinger at gmx.de wrote:
> 
>> From: Reimar Döffinger <Reimar.Doeffinger at gmx.de>
>> 
>> Trivially expand hscale assembler to support > 8 bit formats
>> both for input and output.
>> 16-bit input is not supported as I am not certain how to
>> get sufficient test coverage.
>> ---
>> libswscale/aarch64/hscale.S  | 53 ++++++++++++++++++++++++++----------
>> libswscale/aarch64/swscale.c | 49 +++++++++++++++++++++++++++++++--
>> 2 files changed, 85 insertions(+), 17 deletions(-)
>> 
>> diff --git a/libswscale/aarch64/hscale.S b/libswscale/aarch64/hscale.S
>> index af55ffe2b7..3b42d39dac 100644
>> --- a/libswscale/aarch64/hscale.S
>> +++ b/libswscale/aarch64/hscale.S
>> @@ -20,7 +20,11 @@
>> #include "libavutil/aarch64/asm.S"
>> -function ff_hscale_8_to_15_neon, export=1
>> +.macro hscale srcbits, dstbits, ldt, lds, c
>> +function ff_hscale_\srcbits\()_to_\dstbits\()_neon, export=1
>> +.if \dstbits >= 16
>> +        movi                v20.4S, #(0x1 << (\dstbits - 16)), msl #16
>> +.endif
>>        sbfiz               x7, x6, #1, #32           // filterSize*2 (*2 because int16)
>> 1:      ldr                 w8, [x5], #4             // filterPos[idx]
>>        ldr                 w0, [x5], #4              // filterPos[idx + 1]
>> @@ -34,30 +38,30 @@ function ff_hscale_8_to_15_neon, export=1
>>        movi                v1.2D, #0                 // val sum part 2 (for dst[1])
>>        movi                v2.2D, #0                 // val sum part 3 (for dst[2])
>>        movi                v3.2D, #0                 // val sum part 4 (for dst[3])
>> -        add                 x17, x3, w8, UXTW       // srcp + filterPos[0]
>> -        add                 x8,  x3, w0, UXTW       // srcp + filterPos[1]
>> -        add                 x0, x3, w11, UXTW       // srcp + filterPos[2]
>> -        add                 x11, x3, w9, UXTW       // srcp + filterPos[3]
>> +        add                 x17, x3, w8, UXTW #!!(\srcbits > 8) // srcp + filterPos[0]
>> +        add                 x8,  x3, w0, UXTW #!!(\srcbits > 8) // srcp + filterPos[1]
>> +        add                 x0, x3, w11, UXTW #!!(\srcbits > 8) // srcp + filterPos[2]
>> +        add                 x11, x3, w9, UXTW #!!(\srcbits > 8) // srcp + filterPos[3]
> 
> This construct breaks with llvm's assembler and armasm64 - they don't evaluate !! here, ending up with errors like these:
> 
> <instantiation>:19:31: error: expected integer shift amount
>        add x8, x3, w0, UXTW #!!(8 > 8)
> 
> libswscale\aarch64\hscale.o.asm(3093) : error A2007: wrong operand type for :LNOT:
>        add                 x17, x3, w8, UXTW #!!0
> 
> While less elegant, I guess this can be handled easily by adding a macro parameter that represents the evaluated value of \srcbits > 8.

It’s actually easier. I just need 0 or 1 here, the thing I was missing
that caused me to add the !! is that a true condition does not evaluate
to 1 but to -1, so (\srcbits <= 8) + 1 works with both.


>>        mov                 w15, w6                   // filterSize counter
>> -2:      ld1                 {v4.8B}, [x17], #8      // srcp[filterPos[0] + {0..7}]
>> +2:      ld1                 {v4.\ldt}, [x17], \lds // srcp[filterPos[0] + {0..7}]
>>        ld1                 {v5.8H}, [x16], #16       // load 8x16-bit filter values, part 1
>> -        ld1                 {v6.8B}, [x8], #8       // srcp[filterPos[1] + {0..7}]
>> +        ld1                 {v6.\ldt}, [x8], \lds   // srcp[filterPos[1] + {0..7}]
>>        ld1                 {v7.8H}, [x12], #16       // load 8x16-bit at filter+filterSize
>> -        uxtl                v4.8H, v4.8B            // unpack part 1 to 16-bit
>> +\c\c    uxtl                v4.8H, v4.8B            // unpack part 1 to 16-bit
> 
> With gas-preprocessor and armasm64, // isn't considered a comment char by armasm64, only by the C preprocessor invoked by gas-preprocessor, so it doesn't strip out these lines properly. I guess one could add more code to gas-preprocessor to handle that, but that's probably a bit overkill...

That is a whole another fun topic, I’ll be sending patches related to gas-preprocessor.
Admittedly might consider some of the issues my fault for not cross-compiling and also preferring WSL over msys2 or such...
And then I might get around to addressing the rest.