Re: [FFmpeg-devel] [PATCH 1/2] lavu/intmath.h: Add msvc/icl ctzll optimisations.
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.
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
Re: [FFmpeg-devel] [PATCH 1/2] lavu/intmath.h: Add msvc/icl ctzll optimisations.
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.
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.
On 16 October 2015 at 03:47, James Almer wrote: > 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__. > 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. > > > +#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? > > > +{ > > +#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 *)&v)[1]) > + 32 : _bit_scan_forward((uint32_t)v); > > (uint32_t)(v >> 32) > > > +#endif > > +#else > > +unsigned long c; > > +#if ARCH_X86_64 > > +_BitScanForward64(&c, v); > > +#else > > +if ((uint32_t)v == 0) { > > +_BitScanForward(&c, ((uint32_t *)&v)[1]); > > Same > > > +c += 32; > > +} else { > > +_BitScanForward(&c, (uint32_t)v); > > +} > > +#endif > > +return c; > > +#endif > > +} > > + > > #endif /* __GNUC__ */ > > > > #endif /* AVUTIL_X86_INTMATH_H */ > > -- 1.9.5.github.0 > > > The other suggestions look ok so ill rewrite a new patch later today. ___ 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.
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 *)&v)[1]) + 32 > : _bit_scan_forward((uint32_t)v); (uint32_t)(v >> 32) > +#endif > +#else > +unsigned long c; > +#if ARCH_X86_64 > +_BitScanForward64(&c, v); > +#else > +if ((uint32_t)v == 0) { > +_BitScanForward(&c, ((uint32_t *)&v)[1]); Same > +c += 32; > +} else { > +_BitScanForward(&c, (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
[FFmpeg-devel] [PATCH 1/2] lavu/intmath.h: Add msvc/icl ctzll optimisations.
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