[FFmpeg-devel] [PATCH 1/4] all: Replace if (ARCH_FOO) checks by #if ARCH_FOO, part 2

[Andreas Rheinhardt]

L. E. Segovia:
> Hi!
> 
> Thanks for the review. The comments will be fixed in the next version of
> the patchset. Just a couple remarks:
> 
> re dcadsp_init -- this one comes from 4.4.x having a clause there to
> support SSE, which also checks for the x86-32 architecture [1]. I've
> removed it for the current version.
> 
> re flacdsp_init -- ff_flac_decorrelate_indep8_16_avx and
> ff_flac_decorrelate_indep8_32_avx are from their inception only
> generated for x86-64 [2][3]. This check was missing from the C side,
> implying it relied on DCE to pass linking.
> 

Sorry, I somehow did not recognize that there were already ARCH_X86_64
checks for these flacdsp functions.

> [1]:
> https://github.com/FFmpeg/FFmpeg/blob/release/4.4/libavcodec/x86/dcadsp_init.c#L40-L41
> 
> [2]:
> https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/x86/flacdsp.asm#L315-L318
> 
> [3]:
> https://github.com/FFmpeg/FFmpeg/blob/master/libavcodec/x86/flacdsp.asm#L323-L326
> 

Another idea that I had is as follows: Instead of using if
(EXTERNAL_MMX(cpu_flags)) { ... } we add a macro
IF_EXTERNAL_MMX(cpu_flags, ...) that expands to "if (cpu_flags &
AV_CPU_FLAG_MMX) { __VA_ARGS__ }" if HAVE_EXTERNAL_MMX is true and to
nothing (or a null statement) of it is not.

Advantages of this approach are that it does not involve sprinkling the
codebase with many #ifs and that it automatically checks the right stuff
-- namely the corresponding HAVE_* instead of just HAVE_X86ASM as you do
in the last patch; notice that if an assembler is present, we expect it
to be able to assemble MMX(EXT), SSE etc., but not AVX, and consequently
mostly don't check for it in the .asm files, but as has been said AVX(2)
and XOP are an exception to this, so that one would still get linker
errors with your patchset if one used an assembler that does not support
AVX(2) (or if one disabled it via configure).

Disadvantages of it are that one can not add more #if checks to the
"..." of these macros and that there would be some calls to
av_get_cpu_flags() where the return value would be unused (we might have
to mark the variable as av_unused or mark said function as av_pure to
make the compiler optimize that away and not emit warnings for it).
But it would still be usable for the common case where there are no
further #if checks.

- Andreas