Re: [FFmpeg-devel] [PATCH 1/2] lavu/intmath.h: Add msvc/icl ctzll optimisations.

[James Almer]

On 10/16/2015 3:03 AM, Matt Oliver wrote:
> Take 3. Updated the patch based on comments so that it correctly supports
> msvc, icl and icc

LGTM.

How does the assembly ICL generates for the x86_32 version look like?
ICC seems to create a branch for the v == 0 check, so a handwritten
inline assembly version using cmov for the cases where HAVE_FAST_CMOV
is true may be faster.
Not for this patch anyway. Just something to check and benchmark later
if anyone cares enough about it.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] lavu/intmath.h: Add msvc/icl ctzll optimisations.

[Matt Oliver]

On 16 October 2015 at 12:25, Matt Oliver  wrote:

> On 16 October 2015 at 09:46, James Almer  wrote:
>
>> On 10/15/2015 7:25 PM, Matt Oliver wrote:
>> > ICC defines __GNUC__ but ICL does not! ICC (being the linux variant)
>> > supports all the same functions as gcc so theres no need in this case
>> for
>> > special handling. However ICL defines _MSC_VER so this is correct.
>>
>> Ah, good to know then.
>>
>> >> >
>> >>> > > +#define ff_ctzll ff_ctzll_c
>> >>> > > +static av_always_inline av_const int ff_ctzll_c(long long v)
>> >> >
>> >> > nit: ff_ctzll_x86 for the inline version. And you need to check
>> >> > HAVE_INLINE_ASM for it as
>> >> > well.
>> >> >
>> > I didnt put the HAVE_INLINE_ASM check as at this point we know its ICL
>> and
>> > as ICL always has inline support so I assumed it was redundant. Given
>> that
>> > is there actually a need to check it?
>>
>> You can disable it during configure with --disable-inline-asm.
>> But since it's apparently mainly meant for lavc/lavfi simd (judging by
>> other
>> lavu arch headers like intreadwrite.h that don't bother checking it) you
>> can
>> leave it as is.
>>
>>
> Updated patch attached.
>

Take 3. Updated the patch based on comments so that it correctly supports
msvc, icl and icc


0001-lavu-intmath.h-Add-msvc-icl-ctzll-optimisations.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

[FFmpeg-devel] [PATCH 1/2] lavu/intmath.h: Add msvc/icl ctzll optimisations.

[Matt Oliver]

This patch just adds the msvc and icl equivalent ctzll optimisations to
correspond with the recently added gcc variant


0001-lavu-intmath.h-Add-msvc-icl-ctzll-optimisations.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] lavu/intmath.h: Add msvc/icl ctzll optimisations.

[Matt Oliver]

On 16 October 2015 at 09:46, James Almer  wrote:

> On 10/15/2015 7:25 PM, Matt Oliver wrote:
> > ICC defines __GNUC__ but ICL does not! ICC (being the linux variant)
> > supports all the same functions as gcc so theres no need in this case for
> > special handling. However ICL defines _MSC_VER so this is correct.
>
> Ah, good to know then.
>
> >> >
> >>> > > +#define ff_ctzll ff_ctzll_c
> >>> > > +static av_always_inline av_const int ff_ctzll_c(long long v)
> >> >
> >> > nit: ff_ctzll_x86 for the inline version. And you need to check
> >> > HAVE_INLINE_ASM for it as
> >> > well.
> >> >
> > I didnt put the HAVE_INLINE_ASM check as at this point we know its ICL
> and
> > as ICL always has inline support so I assumed it was redundant. Given
> that
> > is there actually a need to check it?
>
> You can disable it during configure with --disable-inline-asm.
> But since it's apparently mainly meant for lavc/lavfi simd (judging by
> other
> lavu arch headers like intreadwrite.h that don't bother checking it) you
> can
> leave it as is.
>
>
Updated patch attached.


0001-lavu-intmath.h-Add-msvc-icl-ctzll-optimisations.patch
Description: Binary data
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] lavu/intmath.h: Add msvc/icl ctzll optimisations.

[James Almer]

On 10/15/2015 7:25 PM, Matt Oliver wrote:
> ICC defines __GNUC__ but ICL does not! ICC (being the linux variant)
> supports all the same functions as gcc so theres no need in this case for
> special handling. However ICL defines _MSC_VER so this is correct.

Ah, good to know then.

>> >
>>> > > +#define ff_ctzll ff_ctzll_c
>>> > > +static av_always_inline av_const int ff_ctzll_c(long long v)
>> >
>> > nit: ff_ctzll_x86 for the inline version. And you need to check
>> > HAVE_INLINE_ASM for it as
>> > well.
>> >
> I didnt put the HAVE_INLINE_ASM check as at this point we know its ICL and
> as ICL always has inline support so I assumed it was redundant. Given that
> is there actually a need to check it?

You can disable it during configure with --disable-inline-asm.
But since it's apparently mainly meant for lavc/lavfi simd (judging by other
lavu arch headers like intreadwrite.h that don't bother checking it) you can
leave it as is.
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH 1/2] lavu/intmath.h: Add msvc/icl ctzll optimisations.

[James Almer]

On 10/15/2015 11:07 AM, Matt Oliver wrote:
> This patch just adds the msvc and icl equivalent ctzll optimisations to
> correspond with the recently added gcc variant
> 
> 
> 0001-lavu-intmath.h-Add-msvc-icl-ctzll-optimisations.patch
> 
> 
> From 2b8e7757cdb8595181a01bb18756d60662c41fde Mon Sep 17 00:00:00 2001
> From: Matt Oliver 
> Date: Thu, 15 Oct 2015 19:42:31 +1100
> Subject: [PATCH 1/2] lavu/intmath.h: Add msvc/icl ctzll optimisations.
> 
> Signed-off-by: Matt Oliver 
> ---
>  libavutil/x86/intmath.h | 28 
>  1 file changed, 28 insertions(+)
> 
> diff --git a/libavutil/x86/intmath.h b/libavutil/x86/intmath.h
> index fefad20..613903b 100644
> --- a/libavutil/x86/intmath.h
> +++ b/libavutil/x86/intmath.h
> @@ -60,6 +60,34 @@ static av_always_inline av_const unsigned 
> av_mod_uintp2_bmi2(unsigned a, unsigne
>  
>  #endif /* __BMI2__ */
>  
> +#elif defined(_MSC_VER)

Don't use elif if this code is also supposed to work on icl. That compiler 
defines __GNUC__.

> +#define ff_ctzll ff_ctzll_c
> +static av_always_inline av_const int ff_ctzll_c(long long v)

nit: ff_ctzll_x86 for the inline version. And you need to check HAVE_INLINE_ASM 
for it as
well.

> +{
> +#if defined(__INTEL_COMPILER) 
> +#if ARCH_X86_64
> +uint64_t c;
> +__asm__("bsfq %1,%0" : "=r" (c) : "r" (v));
> +return c;
> +#else
> +return ((uint32_t)v == 0) ? _bit_scan_forward(((uint32_t *))[1]) + 32 
> : _bit_scan_forward((uint32_t)v);

(uint32_t)(v >> 32)

> +#endif
> +#else
> +unsigned long c;
> +#if ARCH_X86_64
> +_BitScanForward64(, v);
> +#else
> +if ((uint32_t)v == 0) {
> +_BitScanForward(, ((uint32_t *))[1]);

Same

> +c += 32;
> +} else {
> +_BitScanForward(, (uint32_t)v);
> +}
> +#endif
> +return c;
> +#endif
> +}
> +
>  #endif /* __GNUC__ */
>  
>  #endif /* AVUTIL_X86_INTMATH_H */
> -- 1.9.5.github.0
> 
> 
> 
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel