diff mbox series

[FFmpeg-devel,4/5] avcodec/fft_template: Only check for FF_FFT_PERM_AVX on ARCH_X86

Message ID 20210106231308.2952217-4-andreas.rheinhardt@gmail.com
State Accepted
Commit a454a0c14fa2c2bf712f282a7fcc574bdc90a327
Headers show
Series [FFmpeg-devel,1/5] avcodec/tableprint: Don't include mem_internal.h | expand

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

Andreas Rheinhardt Jan. 6, 2021, 11:13 p.m. UTC
Also do it for FFT_FLOAT only, as this is the only combination for which
it can be set.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/fft_template.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Lynne Jan. 7, 2021, 3:47 p.m. UTC | #1
Jan 7, 2021, 00:13 by andreas.rheinhardt@gmail.com:

> Also do it for FFT_FLOAT only, as this is the only combination for which
> it can be set.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/fft_template.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/fft_template.c b/libavcodec/fft_template.c
> index 9d125de073..ddde63714e 100644
> --- a/libavcodec/fft_template.c
> +++ b/libavcodec/fft_template.c
> @@ -251,7 +251,7 @@ av_cold int ff_fft_init(FFTContext *s, int nbits, int inverse)
>  #endif /* FFT_FIXED_32 */
>  
>  
> -    if (s->fft_permutation == FF_FFT_PERM_AVX) {
> +    if (ARCH_X86 && FFT_FLOAT && s->fft_permutation == FF_FFT_PERM_AVX) {
>  fft_perm_avx(s);
>  } else {
>  #define PROCESS_FFT_PERM_SWAP_LSBS(num) do {\
>

LGTM. Maybe mark fft_perm_avx as inline too if you can be bothered to amend the patch.
Andreas Rheinhardt Jan. 7, 2021, 4:38 p.m. UTC | #2
Lynne:
> Jan 7, 2021, 00:13 by andreas.rheinhardt@gmail.com:
> 
>> Also do it for FFT_FLOAT only, as this is the only combination for which
>> it can be set.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavcodec/fft_template.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/fft_template.c b/libavcodec/fft_template.c
>> index 9d125de073..ddde63714e 100644
>> --- a/libavcodec/fft_template.c
>> +++ b/libavcodec/fft_template.c
>> @@ -251,7 +251,7 @@ av_cold int ff_fft_init(FFTContext *s, int nbits, int inverse)
>>  #endif /* FFT_FIXED_32 */
>>  
>>  
>> -    if (s->fft_permutation == FF_FFT_PERM_AVX) {
>> +    if (ARCH_X86 && FFT_FLOAT && s->fft_permutation == FF_FFT_PERM_AVX) {
>>  fft_perm_avx(s);
>>  } else {
>>  #define PROCESS_FFT_PERM_SWAP_LSBS(num) do {\
>>
> 
> LGTM. Maybe mark fft_perm_avx as inline too if you can be bothered to amend the patch.

I don't see a reason to interfere in the compiler's inlining decision
here. It is a static function only called once, so it will be inlined
anyway.

- Andreas
Lynne Jan. 7, 2021, 5:50 p.m. UTC | #3
Jan 7, 2021, 17:38 by andreas.rheinhardt@gmail.com:

> Lynne:
>
>> Jan 7, 2021, 00:13 by andreas.rheinhardt@gmail.com:
>>
>>> Also do it for FFT_FLOAT only, as this is the only combination for which
>>> it can be set.
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>> ---
>>>  libavcodec/fft_template.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/fft_template.c b/libavcodec/fft_template.c
>>> index 9d125de073..ddde63714e 100644
>>> --- a/libavcodec/fft_template.c
>>> +++ b/libavcodec/fft_template.c
>>> @@ -251,7 +251,7 @@ av_cold int ff_fft_init(FFTContext *s, int nbits, int inverse)
>>>  #endif /* FFT_FIXED_32 */
>>>  
>>>  
>>> -    if (s->fft_permutation == FF_FFT_PERM_AVX) {
>>> +    if (ARCH_X86 && FFT_FLOAT && s->fft_permutation == FF_FFT_PERM_AVX) {
>>>  fft_perm_avx(s);
>>>  } else {
>>>  #define PROCESS_FFT_PERM_SWAP_LSBS(num) do {\
>>>
>>
>> LGTM. Maybe mark fft_perm_avx as inline too if you can be bothered to amend the patch.
>>
>
> I don't see a reason to interfere in the compiler's inlining decision
> here. It is a static function only called once, so it will be inlined
> anyway.
>

Fair enough.
diff mbox series

Patch

diff --git a/libavcodec/fft_template.c b/libavcodec/fft_template.c
index 9d125de073..ddde63714e 100644
--- a/libavcodec/fft_template.c
+++ b/libavcodec/fft_template.c
@@ -251,7 +251,7 @@  av_cold int ff_fft_init(FFTContext *s, int nbits, int inverse)
 #endif /* FFT_FIXED_32 */
 
 
-    if (s->fft_permutation == FF_FFT_PERM_AVX) {
+    if (ARCH_X86 && FFT_FLOAT && s->fft_permutation == FF_FFT_PERM_AVX) {
         fft_perm_avx(s);
     } else {
 #define PROCESS_FFT_PERM_SWAP_LSBS(num) do {\