diff mbox series

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

Message ID 20210110232453.52663-1-Reimar.Doeffinger@gmx.de
State New
Headers show
Series [FFmpeg-devel] libswscale/aarch64/hscale.S: Support more bit-depth variants.
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Reimar Döffinger Jan. 10, 2021, 11:24 p.m. UTC
From: Reimar Döffinger <Reimar.Doeffinger@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(-)

Comments

Martin Storsjö Jan. 15, 2021, 9:10 p.m. UTC | #1
On Mon, 11 Jan 2021, Reimar.Doeffinger@gmx.de wrote:

> From: Reimar Döffinger <Reimar.Doeffinger@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.


>         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...

Adding .if .endif around these instances isn't entirely elegant, but you 
can also make e.g. a macro uxtl_if, which takes a third parameter which is 
a predicate for whether the instruction should be included or not.

>         smlal               v0.4S, v4.4H, v5.4H         // v0 accumulates srcp[filterPos[0] + {0..3}] * filter[{0..3}]
>         smlal2              v0.4S, v4.8H, v5.8H         // v0 accumulates srcp[filterPos[0] + {4..7}] * filter[{4..7}]
> -        ld1                 {v16.8B}, [x0], #8          // srcp[filterPos[2] + {0..7}]
> +        ld1                 {v16.\ldt}, [x0], \lds      // srcp[filterPos[2] + {0..7}]

LLVM's assembler had a bug regarding passing parameters like "8h" as a 
parameter to a macro, which was only fixed in LLVM 8, see 
https://bugs.llvm.org/show_bug.cgi?id=32973.

That bug is easy to avoid by moving the dot into the macro argument, i.e. 
making this "ld1 {v16\ldt}, ..." and passing ".8h" as macro argument.

Additionally - we do have a checkasm test for this function, but not for 
these bitdepth combinations. It'd be great to extend the test to test 
these combinations as well - I'd prefer to not add much more assembly 
without a corresponding checkasm test.

// Martin
Reimar Döffinger Jan. 15, 2021, 9:55 p.m. UTC | #2
> On 15 Jan 2021, at 22:10, Martin Storsjö <martin@martin.st> wrote:
> 
> On Mon, 11 Jan 2021, Reimar.Doeffinger@gmx.de wrote:
> 
>> From: Reimar Döffinger <Reimar.Doeffinger@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.
diff mbox series

Patch

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]
         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
         smlal               v0.4S, v4.4H, v5.4H         // v0 accumulates srcp[filterPos[0] + {0..3}] * filter[{0..3}]
         smlal2              v0.4S, v4.8H, v5.8H         // v0 accumulates srcp[filterPos[0] + {4..7}] * filter[{4..7}]
-        ld1                 {v16.8B}, [x0], #8          // srcp[filterPos[2] + {0..7}]
+        ld1                 {v16.\ldt}, [x0], \lds      // srcp[filterPos[2] + {0..7}]
         ld1                 {v17.8H}, [x13], #16        // load 8x16-bit at filter+2*filterSize
-        uxtl                v6.8H, v6.8B                // unpack part 2 to 16-bit
+\c\c    uxtl                v6.8H, v6.8B                // unpack part 2 to 16-bit
         smlal               v1.4S, v6.4H, v7.4H         // v1 accumulates srcp[filterPos[1] + {0..3}] * filter[{0..3}]
-        uxtl                v16.8H, v16.8B              // unpack part 3 to 16-bit
+\c\c    uxtl                v16.8H, v16.8B              // unpack part 3 to 16-bit
         smlal               v2.4S, v16.4H, v17.4H       // v2 accumulates srcp[filterPos[2] + {0..3}] * filter[{0..3}]
         smlal2              v2.4S, v16.8H, v17.8H       // v2 accumulates srcp[filterPos[2] + {4..7}] * filter[{4..7}]
-        ld1                 {v18.8B}, [x11], #8         // srcp[filterPos[3] + {0..7}]
+        ld1                 {v18.\ldt}, [x11], \lds     // srcp[filterPos[3] + {0..7}]
         smlal2              v1.4S, v6.8H, v7.8H         // v1 accumulates srcp[filterPos[1] + {4..7}] * filter[{4..7}]
         ld1                 {v19.8H}, [x4], #16         // load 8x16-bit at filter+3*filterSize
         subs                w15, w15, #8                // j -= 8: processed 8/filterSize
-        uxtl                v18.8H, v18.8B              // unpack part 4 to 16-bit
+\c\c    uxtl                v18.8H, v18.8B              // unpack part 4 to 16-bit
         smlal               v3.4S, v18.4H, v19.4H       // v3 accumulates srcp[filterPos[3] + {0..3}] * filter[{0..3}]
         smlal2              v3.4S, v18.8H, v19.8H       // v3 accumulates srcp[filterPos[3] + {4..7}] * filter[{4..7}]
         b.gt                2b                          // inner loop if filterSize not consumed completely
@@ -73,8 +77,29 @@  function ff_hscale_8_to_15_neon, export=1
         zip1                v2.4S, v2.4S, v3.4S         // part23 = zip values from part2 and part3
         mov                 v0.d[1], v2.d[0]            // part0123 = zip values from part01 and part23
         subs                w2, w2, #4                  // dstW -= 4
-        sqshrn              v0.4H, v0.4S, #7            // shift and clip the 2x16-bit final values
+.if \dstbits < 16
+        sqshrn              v0.4H, v0.4S, #(14 + \srcbits - \dstbits)  // shift and clip the 2x16-bit final values
         st1                 {v0.4H}, [x1], #8           // write to destination part0123
+.else
+        srshr               v0.4S, v0.4S, #(14 + \srcbits - \dstbits)
+        smin                v0.4S, V0.4S, V20.4S
+        st1                 {v0.4S}, [x1], #16
+.endif
         b.gt                1b                          // loop until end of line
         ret
 endfunc
+.endm
+
+hscale 8, 15, 8B, #8
+hscale 9, 15, 8H, #16, /
+hscale 10, 15, 8H, #16, /
+hscale 12, 15, 8H, #16, /
+hscale 14, 15, 8H, #16, /
+// hscale 16, 15, 8H, #16, / // needs testing and possibly some special-cases
+
+hscale 8 19, 8B, #8
+hscale 9, 19, 8H, #16, /
+hscale 10, 19, 8H, #16, /
+hscale 12, 19, 8H, #16, /
+hscale 14, 19, 8H, #16, /
+// hscale 16, 19, 8H, #16, / // needs testing and possibly some special-cases
diff --git a/libswscale/aarch64/swscale.c b/libswscale/aarch64/swscale.c
index eecbea88ca..4c88f929d7 100644
--- a/libswscale/aarch64/swscale.c
+++ b/libswscale/aarch64/swscale.c
@@ -24,6 +24,33 @@ 
 void ff_hscale_8_to_15_neon(SwsContext *c, int16_t *dst, int dstW,
                             const uint8_t *src, const int16_t *filter,
                             const int32_t *filterPos, int filterSize);
+void ff_hscale_9_to_15_neon(SwsContext *c, int16_t *dst, int dstW,
+                            const uint8_t *src, const int16_t *filter,
+                            const int32_t *filterPos, int filterSize);
+void ff_hscale_10_to_15_neon(SwsContext *c, int16_t *dst, int dstW,
+                            const uint8_t *src, const int16_t *filter,
+                            const int32_t *filterPos, int filterSize);
+void ff_hscale_12_to_15_neon(SwsContext *c, int16_t *dst, int dstW,
+                            const uint8_t *src, const int16_t *filter,
+                            const int32_t *filterPos, int filterSize);
+void ff_hscale_14_to_15_neon(SwsContext *c, int16_t *dst, int dstW,
+                            const uint8_t *src, const int16_t *filter,
+                            const int32_t *filterPos, int filterSize);
+void ff_hscale_8_to_19_neon(SwsContext *c, int16_t *dst, int dstW,
+                            const uint8_t *src, const int16_t *filter,
+                            const int32_t *filterPos, int filterSize);
+void ff_hscale_9_to_19_neon(SwsContext *c, int16_t *dst, int dstW,
+                            const uint8_t *src, const int16_t *filter,
+                            const int32_t *filterPos, int filterSize);
+void ff_hscale_10_to_19_neon(SwsContext *c, int16_t *dst, int dstW,
+                            const uint8_t *src, const int16_t *filter,
+                            const int32_t *filterPos, int filterSize);
+void ff_hscale_12_to_19_neon(SwsContext *c, int16_t *dst, int dstW,
+                            const uint8_t *src, const int16_t *filter,
+                            const int32_t *filterPos, int filterSize);
+void ff_hscale_14_to_19_neon(SwsContext *c, int16_t *dst, int dstW,
+                            const uint8_t *src, const int16_t *filter,
+                            const int32_t *filterPos, int filterSize);
 
 void ff_yuv2planeX_8_neon(const int16_t *filter, int filterSize,
                           const int16_t **src, uint8_t *dest, int dstW,
@@ -34,11 +61,27 @@  av_cold void ff_sws_init_swscale_aarch64(SwsContext *c)
     int cpu_flags = av_get_cpu_flags();
 
     if (have_neon(cpu_flags)) {
-        if (c->srcBpc == 8 && c->dstBpc <= 14 &&
-            (c->hLumFilterSize % 8) == 0 &&
+        if ((c->hLumFilterSize % 8) == 0 &&
             (c->hChrFilterSize % 8) == 0)
         {
-            c->hyScale = c->hcScale = ff_hscale_8_to_15_neon;
+            int largedst = c->dstBpc > 14;
+            switch (c->srcBpc) {
+            case 8:
+                c->hyScale = c->hcScale = largedst ? ff_hscale_8_to_19_neon : ff_hscale_8_to_15_neon;
+                break;
+            case 9:
+                c->hyScale = c->hcScale = largedst ? ff_hscale_9_to_19_neon : ff_hscale_9_to_15_neon;
+                break;
+            case 10:
+                c->hyScale = c->hcScale = largedst ? ff_hscale_10_to_19_neon : ff_hscale_10_to_15_neon;
+                break;
+            case 12:
+                c->hyScale = c->hcScale = largedst ? ff_hscale_12_to_19_neon : ff_hscale_12_to_15_neon;
+                break;
+            case 14:
+                c->hyScale = c->hcScale = largedst ? ff_hscale_14_to_19_neon : ff_hscale_14_to_15_neon;
+                break;
+            }
         }
         if (c->dstBpc == 8) {
             c->yuv2planeX = ff_yuv2planeX_8_neon;