Re: [FFmpeg-devel] [PATCH v3 5/7] lavc/apv: AVX2 transquant for x86-64

[Mark Thompson <sw@jkqxz.net>]

On 24/04/2025 03:55, James Almer wrote:
> On 4/23/2025 5:45 PM, Mark Thompson wrote:
>> Typical checkasm result on Alder Lake:
>>
>> decode_transquant_8_c:                                 464.2 ( 1.00x)
>> decode_transquant_8_avx2:                               86.2 ( 5.38x)
>> decode_transquant_10_c:                                481.6 ( 1.00x)
>> decode_transquant_10_avx2:                              83.5 ( 5.77x)
>> ---
>>   libavcodec/apv_dsp.c          |   4 +
>>   libavcodec/apv_dsp.h          |   2 +
>>   libavcodec/x86/Makefile       |   2 +
>>   libavcodec/x86/apv_dsp.asm    | 311 ++++++++++++++++++++++++++++++++++
>>   libavcodec/x86/apv_dsp_init.c |  44 +++++
>>   tests/checkasm/Makefile       |   1 +
>>   tests/checkasm/apv_dsp.c      | 109 ++++++++++++
>>   tests/checkasm/checkasm.c     |   3 +
>>   tests/checkasm/checkasm.h     |   1 +
>>   tests/fate/checkasm.mak       |   1 +
>>   10 files changed, 478 insertions(+)
>>   create mode 100644 libavcodec/x86/apv_dsp.asm
>>   create mode 100644 libavcodec/x86/apv_dsp_init.c
>>   create mode 100644 tests/checkasm/apv_dsp.c
>>
>> ...
>> diff --git a/libavcodec/x86/apv_dsp.asm b/libavcodec/x86/apv_dsp.asm
>> new file mode 100644
>> index 0000000000..12d96481de
>> --- /dev/null
>> +++ b/libavcodec/x86/apv_dsp.asm
>> @@ -0,0 +1,311 @@
>> +;************************************************************************
>> +;* This file is part of FFmpeg.
>> +;*
>> +;* FFmpeg is free software; you can redistribute it and/or
>> +;* modify it under the terms of the GNU Lesser General Public
>> +;* License as published by the Free Software Foundation; either
>> +;* version 2.1 of the License, or (at your option) any later version.
>> +;*
>> +;* FFmpeg is distributed in the hope that it will be useful,
>> +;* but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +;* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> +;* Lesser General Public License for more details.
>> +;*
>> +;* You should have received a copy of the GNU Lesser General Public
>> +;* License along with FFmpeg; if not, write to the Free Software
>> +;* 51, Inc., Foundation Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>> +;******************************************************************************
>> +
>> +%include "libavutil/x86/x86util.asm"
>> +
>> +%if ARCH_X86_64
>> +
>> +SECTION_RODATA 32
>> +
>> +; Full matrix for row transform.
>> +const tmatrix_row
>> +    dw  64,  89,  84,  75,  64,  50,  35,  18
>> +    dw  64, -18, -84,  50,  64, -75, -35,  89
>> +    dw  64,  75,  35, -18, -64, -89, -84, -50
>> +    dw  64, -50, -35,  89, -64, -18,  84, -75
>> +    dw  64,  50, -35, -89, -64,  18,  84,  75
>> +    dw  64, -75,  35,  18, -64,  89, -84,  50
>> +    dw  64,  18, -84, -50,  64,  75, -35, -89
>> +    dw  64, -89,  84, -75,  64, -50,  35, -18
>> +
>> +; Constant pairs for broadcast in column transform.
>> +const tmatrix_col_even
>> +    dw  64,  64,  64, -64
>> +    dw  84,  35,  35, -84
>> +const tmatrix_col_odd
>> +    dw  89,  75,  50,  18
>> +    dw  75, -18, -89, -50
>> +    dw  50, -89,  18,  75
>> +    dw  18, -50,  75, -89
>> +
>> +; Memory targets for vpbroadcastd (register version requires AVX512).
>> +cextern pd_1
>> +const sixtyfour
>> +    dd  64
>> +
>> +SECTION .text
>> +
>> +; void ff_apv_decode_transquant_avx2(void *output,
>> +;                                    ptrdiff_t pitch,
>> +;                                    const int16_t *input,
>> +;                                    const int16_t *qmatrix,
>> +;                                    int bit_depth,
>> +;                                    int qp_shift);
>> +
>> +INIT_YMM avx2
>> +
>> +cglobal apv_decode_transquant, 5, 7, 16, output, pitch, input, qmatrix, bit_depth, qp_shift, tmp
>> +
>> +    ; Load input and dequantise
>> +
>> +    vpbroadcastd  m10, [pd_1]
>> +    lea       tmpd, [bit_depthd - 2]
>> +    movd      xm8, qp_shiftm
>> +    movd      xm9, tmpd
>> +    vpslld    m10, m10, xm9
>> +    vpsrld    m10, m10, 1
>> +
>> +    ; m8  = scalar qp_shift
>> +    ; m9  = scalar bd_shift
>> +    ; m10 = vector 1 << (bd_shift - 1)
>> +    ; m11 = qmatrix load
>> +
>> +%macro LOAD_AND_DEQUANT 2 ; (xmm input, constant offset)
>> +    vpmovsxwd m%1, [inputq   + %2]
>> +    vpmovsxwd m11, [qmatrixq + %2]
>> +    vpmaddwd  m%1, m%1, m11
>> +    vpslld    m%1, m%1, xm8
>> +    vpaddd    m%1, m%1, m10
>> +    vpsrad    m%1, m%1, xm9
>> +    vpackssdw m%1, m%1, m%1
>> +%endmacro
>> +
>> +    LOAD_AND_DEQUANT 0, 0x00
>> +    LOAD_AND_DEQUANT 1, 0x10
>> +    LOAD_AND_DEQUANT 2, 0x20
>> +    LOAD_AND_DEQUANT 3, 0x30
>> +    LOAD_AND_DEQUANT 4, 0x40
>> +    LOAD_AND_DEQUANT 5, 0x50
>> +    LOAD_AND_DEQUANT 6, 0x60
>> +    LOAD_AND_DEQUANT 7, 0x70
>> +
>> +    ; mN = row N words 0 1 2 3 0 1 2 3 4 5 6 7 4 5 6 7
>> +
>> +    ; Transform columns
>> +    ; This applies a 1-D DCT butterfly
>> +
>> +    vpunpcklwd  m12, m0,  m4
>> +    vpunpcklwd  m13, m2,  m6
>> +    vpunpcklwd  m14, m1,  m3
>> +    vpunpcklwd  m15, m5,  m7
>> +
>> +    ; m12 = rows 0 and 4 interleaved
>> +    ; m13 = rows 2 and 6 interleaved
>> +    ; m14 = rows 1 and 3 interleaved
>> +    ; m15 = rows 5 and 7 interleaved
>> +
>> +    lea         tmpq, [tmatrix_col_even]
>> +    vpbroadcastd   m0, [tmpq + 0x00]
>> +    vpbroadcastd   m1, [tmpq + 0x04]
>> +    vpbroadcastd   m2, [tmpq + 0x08]
>> +    vpbroadcastd   m3, [tmpq + 0x0c]
> 
> How about
> 
>     vbroadcasti128   m0, [tmatrix_col_even]
>     pshufd   m1, m0, q1111
>     pshufd   m2, m0, q2222
>     pshufd   m3, m0, q3333
>     pshufd   m0, m0, q0000
> 
> So you remove the lea, and do a single load from memory within a single cross-lane intruction, instead of four of each.
> 
> Same below.

The broadcasts from memory are not slow, they don't read from either lane.

I can't measure a diffrence but instruction tables have vpbroadcastd as 1/3 and pshufd as 1/2 so I think I'll take that as a tie-break?  (lea is free and they will all load together, the vbroadcasti128 load is unaligned but pretty sure that is irrelevant.)

>> +
>> +    vpmaddwd  m4,  m12, m0
>> +    vpmaddwd  m5,  m12, m1
>> +    vpmaddwd  m6,  m13, m2
>> +    vpmaddwd  m7,  m13, m3
>> +    vpaddd    m8,  m4,  m6
>> +    vpaddd    m9,  m5,  m7
>> +    vpsubd    m10, m5,  m7
>> +    vpsubd    m11, m4,  m6
>> +
>> +    lea         tmpq, [tmatrix_col_odd]
>> +    vpbroadcastd   m0, [tmpq + 0x00]
>> +    vpbroadcastd   m1, [tmpq + 0x04]
>> +    vpbroadcastd   m2, [tmpq + 0x08]
>> +    vpbroadcastd   m3, [tmpq + 0x0c]
>> +
>> +    vpmaddwd  m4,  m14, m0
>> +    vpmaddwd  m5,  m15, m1
>> +    vpmaddwd  m6,  m14, m2
>> +    vpmaddwd  m7,  m15, m3
>> +    vpaddd    m12, m4,  m5
>> +    vpaddd    m13, m6,  m7
>> +
>> +    vpbroadcastd   m0, [tmpq + 0x10]
>> +    vpbroadcastd   m1, [tmpq + 0x14]
>> +    vpbroadcastd   m2, [tmpq + 0x18]
>> +    vpbroadcastd   m3, [tmpq + 0x1c]
>> +
>> +    vpmaddwd  m4,  m14, m0
>> +    vpmaddwd  m5,  m15, m1
>> +    vpmaddwd  m6,  m14, m2
>> +    vpmaddwd  m7,  m15, m3
>> +    vpaddd    m14, m4,  m5
>> +    vpaddd    m15, m6,  m7
>> +
>> +    vpaddd    m0,  m8,  m12
>> +    vpaddd    m1,  m9,  m13
>> +    vpaddd    m2,  m10, m14
>> +    vpaddd    m3,  m11, m15
>> +    vpsubd    m4,  m11, m15
>> +    vpsubd    m5,  m10, m14
>> +    vpsubd    m6,  m9,  m13
>> +    vpsubd    m7,  m8,  m12
>> +
>> +    ; Mid-transform normalisation
>> +    ; Note that outputs here are fitted to 16 bits
>> +
>> +    vpbroadcastd  m8, [sixtyfour]
>> +
>> +%macro NORMALISE 1
>> +    vpaddd    m%1, m%1, m8
>> +    vpsrad    m%1, m%1, 7
>> +    vpackssdw m%1, m%1, m%1
>> +    vpermq    m%1, m%1, q3120
>> +%endmacro
>> +
>> +    NORMALISE 0
>> +    NORMALISE 1
>> +    NORMALISE 2
>> +    NORMALISE 3
>> +    NORMALISE 4
>> +    NORMALISE 5
>> +    NORMALISE 6
>> +    NORMALISE 7
>> +
>> +    ; mN = row N words 0 1 2 3 4 5 6 7 0 1 2 3 4 5 6 7
>> +
>> +    ; Transform rows
>> +    ; This multiplies the rows directly by the transform matrix,
>> +    ; avoiding the need to transpose anything
>> +
>> +    lea       tmpq, [tmatrix_row]
>> +    mova      m12, [tmpq + 0x00]
>> +    mova      m13, [tmpq + 0x20]
>> +    mova      m14, [tmpq + 0x40]
>> +    mova      m15, [tmpq + 0x60]
>> +
>> +%macro TRANS_ROW_STEP 1
>> +    vpmaddwd  m8,  m%1, m12
>> +    vpmaddwd  m9,  m%1, m13
>> +    vpmaddwd  m10, m%1, m14
>> +    vpmaddwd  m11, m%1, m15
>> +    vphaddd   m8,  m8,  m9
>> +    vphaddd   m10, m10, m11
>> +    vphaddd   m%1, m8,  m10
>> +%endmacro
>> +
>> +    TRANS_ROW_STEP 0
>> +    TRANS_ROW_STEP 1
>> +    TRANS_ROW_STEP 2
>> +    TRANS_ROW_STEP 3
>> +    TRANS_ROW_STEP 4
>> +    TRANS_ROW_STEP 5
>> +    TRANS_ROW_STEP 6
>> +    TRANS_ROW_STEP 7
>> +
>> +    ; Renormalise, clip and store output
>> +
>> +    vpbroadcastd  m14, [pd_1]
>> +    mov       tmpd, 20
>> +    sub       tmpd, bit_depthd
>> +    movd      xm9, tmpd
>> +    dec       tmpd
>> +    movd      xm13, tmpd
>> +    movd      xm15, bit_depthd
>> +    vpslld    m8,  m14, xm13
>> +    vpslld    m12, m14, xm15
>> +    vpsrld    m10, m12, 1
>> +    vpsubd    m12, m12, m14
>> +    vpxor     m11, m11, m11
>> +
>> +    ; m8  = vector 1 << (bd_shift - 1)
>> +    ; m9  = scalar bd_shift
>> +    ; m10 = vector 1 << (bit_depth - 1)
>> +    ; m11 = zero
>> +    ; m12 = vector (1 << bit_depth) - 1
>> +
>> +    cmp       bit_depthd, 8
>> +    jne       store_10
>> +
>> +    lea       tmpq, [pitchq + 2*pitchq]
>> +%macro NORMALISE_AND_STORE_8 4
>> +    vpaddd    m%1, m%1, m8
>> +    vpaddd    m%2, m%2, m8
>> +    vpaddd    m%3, m%3, m8
>> +    vpaddd    m%4, m%4, m8
>> +    vpsrad    m%1, m%1, xm9
>> +    vpsrad    m%2, m%2, xm9
>> +    vpsrad    m%3, m%3, xm9
>> +    vpsrad    m%4, m%4, xm9
>> +    vpaddd    m%1, m%1, m10
>> +    vpaddd    m%2, m%2, m10
>> +    vpaddd    m%3, m%3, m10
>> +    vpaddd    m%4, m%4, m10
>> +    ; m%1 = A0-3 A4-7
>> +    ; m%2 = B0-3 B4-7
>> +    ; m%3 = C0-3 C4-7
>> +    ; m%4 = D0-3 D4-7
>> +    vpackusdw m%1, m%1, m%2
>> +    vpackusdw m%3, m%3, m%4
>> +    ; m%1 = A0-3 B0-3 A4-7 B4-7
>> +    ; m%2 = C0-3 D0-3 C4-7 D4-7
>> +    vpermq    m%1, m%1, q3120
>> +    vpermq    m%2, m%3, q3120
>> +    ; m%1 = A0-3 A4-7 B0-3 B4-7
>> +    ; m%2 = C0-3 C4-7 D0-3 D4-7
>> +    vpackuswb m%1, m%1, m%2
>> +    ; m%1 = A0-3 A4-7 C0-3 C4-7 B0-3 B4-7 D0-3 D4-7
>> +    vextracti128  xm%2, m%1, 1
>> +    vpsrldq   xm%3, xm%1, 8
>> +    vpsrldq   xm%4, xm%2, 8
>> +    vmovq     [outputq],            xm%1
>> +    vmovq     [outputq + pitchq],   xm%2
>> +    vmovq     [outputq + 2*pitchq], xm%3
>> +    vmovq     [outputq + tmpq],     xm%4
> 
>     vextracti128  xm%2, m%1, 1
>     vmovq     [outputq],            xm%1
>     vmovq     [outputq + pitchq],   xm%2
>     vpextrq   [outputq + 2*pitchq], xm%1, 1
>     vpextrq   [outputq + tmpq],     xm%2, 1
> 
> Saves you two vpsrldq, and may or may not be faster. Feel free to bench or ignore.

My measurements are not accurate enough to see a difference but instruction tables say it should win, so I'll take it.

>> +    lea       outputq, [outputq + 4*pitchq]
>> +%endmacro
>> +
>> +    NORMALISE_AND_STORE_8 0, 1, 2, 3
>> +    NORMALISE_AND_STORE_8 4, 5, 6, 7
>> +
>> +    RET
>> +
>> +store_10:
>> +
>> +%macro NORMALISE_AND_STORE_10 2
>> +    vpaddd    m%1, m%1, m8
>> +    vpaddd    m%2, m%2, m8
>> +    vpsrad    m%1, m%1, xm9
>> +    vpsrad    m%2, m%2, xm9
>> +    vpaddd    m%1, m%1, m10
>> +    vpaddd    m%2, m%2, m10
>> +    vpmaxsd   m%1, m%1, m11
>> +    vpmaxsd   m%2, m%2, m11
>> +    vpminsd   m%1, m%1, m12
>> +    vpminsd   m%2, m%2, m12
>> +    ; m%1 = A0-3 A4-7
>> +    ; m%2 = B0-3 B4-7
>> +    vpackusdw m%1, m%1, m%2
>> +    ; m%1 = A0-3 B0-3 A4-7 B4-7
>> +    vpermq    m%1, m%1, q3120
>> +    ; m%1 = A0-3 A4-7 B0-3 B4-7
>> +    vextracti128  [outputq],          m%1, 0
> 
> mova [outputq], xm%1
> 
> There's pretty much never a good reason to use extract for the lower bits.

Yep.  Nicely symmetrical code is not enough of a good reason :(

>> +    vextracti128  [outputq + pitchq], m%1, 1
>> +    lea       outputq, [outputq + 2*pitchq]
>> +%endmacro
>> +
>> +    NORMALISE_AND_STORE_10 0, 1
>> +    NORMALISE_AND_STORE_10 2, 3
>> +    NORMALISE_AND_STORE_10 4, 5
>> +    NORMALISE_AND_STORE_10 6, 7
>> +
>> +    RET
>> +
>> +%endif ; ARCH_X86_64
>> ...
>> diff --git a/tests/checkasm/apv_dsp.c b/tests/checkasm/apv_dsp.c
>> new file mode 100644
>> index 0000000000..b3adb8ca06
>> --- /dev/null
>> +++ b/tests/checkasm/apv_dsp.c
>> @@ -0,0 +1,109 @@
>> +/*
>> + * This file is part of FFmpeg.
>> + *
>> + * FFmpeg is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * FFmpeg is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with FFmpeg; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>> + */
>> +
>> +#include <stdint.h>
>> +
>> +#include "checkasm.h"
>> +
>> +#include "libavutil/attributes.h"
>> +#include "libavutil/mem_internal.h"
>> +#include "libavcodec/apv_dsp.h"
>> +
>> +
>> +static void check_decode_transquant_8(void)
>> +{
>> +    LOCAL_ALIGNED_16(int16_t, input,      [64]);
>> +    LOCAL_ALIGNED_16(int16_t, qmatrix,    [64]);
>> +    LOCAL_ALIGNED_16(uint8_t, new_output, [64]);
>> +    LOCAL_ALIGNED_16(uint8_t, ref_output, [64]);
>> +
>> +    declare_func(void,
>> +                 uint8_t *output,
> 
> nit: this parameter is void*, so maybe just use that instead.

Fair.  I was wondering whether to template two functions which lets it keep the type, but the fact that the branch is completely predictable makes it seem like a waste,

>> +                 ptrdiff_t pitch,
>> +                 const int16_t *input,
>> +                 const int16_t *qmatrix,
>> +                 int bit_depth,
>> +                 int qp_shift);
>> +
>> +    for (int i = 0; i < 64; i++) {
>> +        // Any signed 12-bit integer.
>> +        input[i] = rnd() % 2048 - 1024;
>> +
>> +        // qmatrix input is premultiplied by level_scale, so
>> +        // range is 1 to 255 * 71.  Interesting values are all
>> +        // at the low end of that, though.
>> +        qmatrix[i] = rnd() % 16 + 16;
>> +    }
>> +
>> +    call_ref(ref_output, 8, input, qmatrix, 8, 4);
>> +    call_new(new_output, 8, input, qmatrix, 8, 4);
>> +
>> +    if (memcmp(new_output, ref_output, 64 * sizeof(*ref_output)))
>> +        fail();
>> +
>> +    bench_new(new_output, 8, input, qmatrix, 8, 4);
>> +}
>> +
>> +static void check_decode_transquant_10(void)
>> +{
>> +    LOCAL_ALIGNED_16( int16_t, input,      [64]);
>> +    LOCAL_ALIGNED_16( int16_t, qmatrix,    [64]);
>> +    LOCAL_ALIGNED_16(uint16_t, new_output, [64]);
>> +    LOCAL_ALIGNED_16(uint16_t, ref_output, [64]);
>> +
>> +    declare_func(void,
>> +                 uint16_t *output,
> 
> Ditto.
> 
>> +                 ptrdiff_t pitch,
>> +                 const int16_t *input,
>> +                 const int16_t *qmatrix,
>> +                 int bit_depth,
>> +                 int qp_shift);
>> +
>> +    for (int i = 0; i < 64; i++) {
>> +        // Any signed 14-bit integer.
>> +        input[i] = rnd() % 16384 - 8192;
>> +
>> +        // qmatrix input is premultiplied by level_scale, so
>> +        // range is 1 to 255 * 71.  Interesting values are all
>> +        // at the low end of that, though.
>> +        qmatrix[i] = 16; //rnd() % 16 + 16;
>> +    }
>> +
>> +    call_ref(ref_output, 16, input, qmatrix, 10, 4);
>> +    call_new(new_output, 16, input, qmatrix, 10, 4);
>> +
>> +    if (memcmp(new_output, ref_output, 64 * sizeof(*ref_output)))
>> +        fail();
>> +
>> +    bench_new(new_output, 16, input, qmatrix, 10, 4);
>> +}
>> +
>> +void checkasm_check_apv_dsp(void)
>> +{
>> +    APVDSPContext dsp;
>> +
>> +    ff_apv_dsp_init(&dsp);
>> +
>> +    if (check_func(dsp.decode_transquant, "decode_transquant_8"))
>> +        check_decode_transquant_8();
>> +
>> +    if (check_func(dsp.decode_transquant, "decode_transquant_10"))
>> +        check_decode_transquant_10();
>> +
>> +    report("apv_dsp");
> 
> report("decode_transquant");
> 
> So you get
> 
>  - apv_dsp.decode_transquant [OK]
> 
> instead of
> 
>  - apv_dsp.apv_dsp [OK]
> 
> In the checkasm output.

Ah, I hadn't connected that output to the string here.  Fixed.

>> +}
>> diff --git a/tests/checkasm/checkasm.c b/tests/checkasm/checkasm.c
>> index 412b8b2cd1..3bb82ed0e5 100644
>> --- a/tests/checkasm/checkasm.c
>> +++ b/tests/checkasm/checkasm.c
>> @@ -129,6 +129,9 @@ static const struct {
>>       #if CONFIG_ALAC_DECODER
>>           { "alacdsp", checkasm_check_alacdsp },
>>       #endif
>> +    #if CONFIG_APV_DECODER
>> +        { "apv_dsp", checkasm_check_apv_dsp },
>> +    #endif
>>       #if CONFIG_AUDIODSP
>>           { "audiodsp", checkasm_check_audiodsp },
>>       #endif
>> ...

Thanks,

- Mark

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".