Re: [FFmpeg-devel] [PATCH 1/2] libavutil/cpu: Add AV_CPU_FLAG_SLOW_GATHER.

From: Lynne <dev@lynne.ee>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH 1/2] libavutil/cpu: Add AV_CPU_FLAG_SLOW_GATHER.
Date: Mon, 20 Dec 2021 15:47:13 +0100 (CET)
Message-ID: <MrNItEU--3-2@lynne.ee> (raw)
In-Reply-To: <20211220144312.738559-1-alankelly@google.com>

20 Dec 2021, 15:43 by alankelly-at-google.com@ffmpeg.org:

> This flag is set on Haswell and earlier and all AMD cpus.
> ---
>  Removes unnecessary indentation, clarifies comment and only sets flag on AMD
>  cpus with AVX2.
>  libavutil/cpu.h     |  1 +
>  libavutil/x86/cpu.c | 14 +++++++++++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/libavutil/cpu.h b/libavutil/cpu.h
> index ae443eccad..ce9bf14bf7 100644
> --- a/libavutil/cpu.h
> +++ b/libavutil/cpu.h
> @@ -54,6 +54,7 @@
>  #define AV_CPU_FLAG_BMI1        0x20000 ///< Bit Manipulation Instruction Set 1
>  #define AV_CPU_FLAG_BMI2        0x40000 ///< Bit Manipulation Instruction Set 2
>  #define AV_CPU_FLAG_AVX512     0x100000 ///< AVX-512 functions: requires OS support even if YMM/ZMM registers aren't used
> +#define AV_CPU_FLAG_SLOW_GATHER  0x2000000 ///< CPU has slow gathers.
>  
>  #define AV_CPU_FLAG_ALTIVEC      0x0001 ///< standard
>  #define AV_CPU_FLAG_VSX          0x0002 ///< ISA 2.06
> diff --git a/libavutil/x86/cpu.c b/libavutil/x86/cpu.c
> index bcd41a50a2..563984f234 100644
> --- a/libavutil/x86/cpu.c
> +++ b/libavutil/x86/cpu.c
> @@ -146,8 +146,16 @@ int ff_get_cpu_flags_x86(void)
>  if (max_std_level >= 7) {
>  cpuid(7, eax, ebx, ecx, edx);
>  #if HAVE_AVX2
> -        if ((rval & AV_CPU_FLAG_AVX) && (ebx & 0x00000020))
> +        if ((rval & AV_CPU_FLAG_AVX) && (ebx & 0x00000020)) {
>  rval |= AV_CPU_FLAG_AVX2;
> +            cpuid(1, eax, ebx, ecx, std_caps);
> +            family = ((eax >> 8) & 0xf) + ((eax >> 20) & 0xff);
> +            model  = ((eax >> 4) & 0xf) + ((eax >> 12) & 0xf0);
> +            /* Haswell has slow gather */
> +            if(family == 6 && model < 70)
> +                rval |= AV_CPU_FLAG_SLOW_GATHER;
> +        }
> +
>  #if HAVE_AVX512 /* F, CD, BW, DQ, VL */
>  if ((xcr0_lo & 0xe0) == 0xe0) { /* OPMASK/ZMM state */
>  if ((rval & AV_CPU_FLAG_AVX2) && (ebx & 0xd0030000) == 0xd0030000)
> @@ -196,6 +204,10 @@ int ff_get_cpu_flags_x86(void)
>  used unless explicitly disabled by checking AV_CPU_FLAG_AVXSLOW. */
>  if ((family == 0x15 || family == 0x16) && (rval & AV_CPU_FLAG_AVX))
>  rval |= AV_CPU_FLAG_AVXSLOW;
> +
> +        /* AMD cpus have slow gather */
> +            if(rval & AV_CPU_FLAG_AVX2)
> +                rval |= AV_CPU_FLAG_SLOW_GATHER;
>  } 
>

No, I'd rather limit AMD CPUs to all currently released CPUs.
Future ones are getting AVX512, which did speed up gathers on
Intel CPUs, as the ISA extension extended gathers and addded
scatters.

Also your previous patch introduces ff_shuffle_filter_coefficients()
which is so bad it pretty much needs a complete rewrite.
You're also not detecting malloc errors or propagating them back.

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