Re: [FFmpeg-devel] [PATCH 1/3] avutil/attributes: add av_likely and av_unlikely
On Fri, Jan 25, 2019 at 01:28:51AM +0100, Hendrik Leppkes wrote: > On Thu, Jan 24, 2019 at 11:06 PM Michael Niedermayer > wrote: > > > > On Thu, Jan 24, 2019 at 06:08:57PM -0300, James Almer wrote: > > > On 1/24/2019 5:53 PM, Rostislav Pehlivanov wrote: > > > > On Thu, 24 Jan 2019 at 20:38, Marton Balint wrote: > > > > > > > >> Signed-off-by: Marton Balint > > > >> --- > > > >> doc/APIchanges | 3 +++ > > > >> libavutil/attributes.h | 8 > > > >> libavutil/version.h| 2 +- > > > >> 3 files changed, 12 insertions(+), 1 deletion(-) > > > >> > > > >> diff --git a/doc/APIchanges b/doc/APIchanges > > > >> index a39a3ff2ba..38b5b980a6 100644 > > > >> --- a/doc/APIchanges > > > >> +++ b/doc/APIchanges > > > >> @@ -15,6 +15,9 @@ libavutil: 2017-10-21 > > > >> > > > >> API changes, most recent first: > > > >> > > > >> +2019-01-xx - xx - lavu 56.27.100 - attributes.h > > > >> + Add av_likely and av_unlikely > > > >> + > > > >> 2019-01-08 - xx - lavu 56.26.100 - frame.h > > > >>Add AV_FRAME_DATA_REGIONS_OF_INTEREST > > > >> > > > >> diff --git a/libavutil/attributes.h b/libavutil/attributes.h > > > >> index ced108aa2c..60972e5109 100644 > > > >> --- a/libavutil/attributes.h > > > >> +++ b/libavutil/attributes.h > > > >> @@ -164,4 +164,12 @@ > > > >> #define av_noreturn > > > >> #endif > > > >> > > > >> +#if AV_GCC_VERSION_AT_LEAST(2,96) || defined(__clang__) > > > >> +# define av_likely(x) __builtin_expect(!!(x), 1) > > > >> +# define av_unlikely(x)__builtin_expect(!!(x), 0) > > > >> +#else > > > >> +# define av_likely(x) (x) > > > >> +# define av_unlikely(x)(x) > > > >> +#endif > > > >> + > > > >> #endif /* AVUTIL_ATTRIBUTES_H */ > > > >> diff --git a/libavutil/version.h b/libavutil/version.h > > > >> index 1fcdea95bf..12b4f9fc3a 100644 > > > >> --- a/libavutil/version.h > > > >> +++ b/libavutil/version.h > > > >> @@ -79,7 +79,7 @@ > > > >> */ > > > >> > > > >> #define LIBAVUTIL_VERSION_MAJOR 56 > > > >> -#define LIBAVUTIL_VERSION_MINOR 26 > > > >> +#define LIBAVUTIL_VERSION_MINOR 27 > > > >> #define LIBAVUTIL_VERSION_MICRO 100 > > > >> > > > >> #define LIBAVUTIL_VERSION_INT > > > >> AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ > > > >> -- > > > >> 2.16.4 > > > >> > > > >> ___ > > > >> ffmpeg-devel mailing list > > > >> ffmpeg-devel@ffmpeg.org > > > >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > > > > > > > > > > > > NAK, NAK, NAK. > > > > I will NAK the hell out of any patch that does that. They're completely > > > > unnecessary, they're commonly used by complete idiots who add them > > > > thinking > > > > their code will go faster (and it might - only on their antiquated GCC > > > > 3.1), they're religiously used by the same people and won't back down on > > > > using them and finally they're ugly when added to every single bloody > > > > branch and the people who maintain such code will demand they be added > > > > to > > > > every single bloody branch for no reason other that what I've just > > > > iterated > > > > on. > > > > The ONLY way I'll accept them is in platform-specific files, e.g. > > > > libavcodec/ppc/*, and even then only on non-x86 arches (which need them > > > > for > > > > having bad compilers with primitive processors having awful branch > > > > prediction) and even then I'd never accept their inclusioin into > > > > system-installed headers. > > > > > > What about hot loops with branches where you can use these as a hint for > > > the compiler to assume a specific outcome is highly unlikely to happen, > > > like alloc errors, corner cases in some codec, etc, which it simply has > > > no way to correctly guess at compile time? > > > > > > I don't think it should be NAKed because it's misused in other projects. > > > We're not other projects. We should instead simply ask the patch writer > > > to provide numbers that prove using it makes a difference in their code > > > with a recent version of GCC/Clang, and if it's negligible or within the > > > margin of error we'll simply ask to remove it to keep the code clean. > > > > if we want to ensure this, it can be enforced easily > > we have fate-source, that can check litterally for each av_(un)likely > > to contain a comment on the same line listing a non negligible performance > > effect. as in // branch hint increases speed by 5% > > I'm generally not a fan of these hints at all, since the majority of > cases its just noise. The performance impact can vary extremely > between compilers and CPUs used, and in average its probably minimal > at best. > Even if you comment it with some speed number, it'll most of the time > be limited to one specific combination of compiler and CPU, and as > such any documented numbers are mostly meaningless. > > Which is the entire problem with these likely/unlikely hints in the > first place. If you want to enforce using them in places where it > "matters", then how
Re: [FFmpeg-devel] [PATCH 1/3] avutil/attributes: add av_likely and av_unlikely
On 1/24/19 11:53 PM, Rostislav Pehlivanov wrote: On Thu, 24 Jan 2019 at 20:38, Marton Balint wrote: Signed-off-by: Marton Balint --- doc/APIchanges | 3 +++ libavutil/attributes.h | 8 libavutil/version.h| 2 +- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/doc/APIchanges b/doc/APIchanges index a39a3ff2ba..38b5b980a6 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,9 @@ libavutil: 2017-10-21 API changes, most recent first: +2019-01-xx - xx - lavu 56.27.100 - attributes.h + Add av_likely and av_unlikely + 2019-01-08 - xx - lavu 56.26.100 - frame.h Add AV_FRAME_DATA_REGIONS_OF_INTEREST diff --git a/libavutil/attributes.h b/libavutil/attributes.h index ced108aa2c..60972e5109 100644 --- a/libavutil/attributes.h +++ b/libavutil/attributes.h @@ -164,4 +164,12 @@ #define av_noreturn #endif +#if AV_GCC_VERSION_AT_LEAST(2,96) || defined(__clang__) +# define av_likely(x) __builtin_expect(!!(x), 1) +# define av_unlikely(x)__builtin_expect(!!(x), 0) +#else +# define av_likely(x) (x) +# define av_unlikely(x)(x) +#endif + #endif /* AVUTIL_ATTRIBUTES_H */ diff --git a/libavutil/version.h b/libavutil/version.h index 1fcdea95bf..12b4f9fc3a 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -79,7 +79,7 @@ */ #define LIBAVUTIL_VERSION_MAJOR 56 -#define LIBAVUTIL_VERSION_MINOR 26 +#define LIBAVUTIL_VERSION_MINOR 27 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ -- 2.16.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel NAK, NAK, NAK. I will NAK the hell out of any patch that does that. They're completely unnecessary, they're commonly used by complete idiots who add them thinking their code will go faster (and it might - only on their antiquated GCC 3.1), they're religiously used by the same people and won't back down on using them and finally they're ugly when added to every single bloody branch and the people who maintain such code will demand they be added to every single bloody branch for no reason other that what I've just iterated on. There are valid reasons to want to use branch probability hints beyond aiding branch prediction. For example, reducing cache pollution in general and loop sizes in particular. Calling people who care about those things idiots and religiously denying the feature is an idiotic thing to do in its own right. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avutil/attributes: add av_likely and av_unlikely
On Thu, Jan 24, 2019 at 11:06 PM Michael Niedermayer wrote: > > On Thu, Jan 24, 2019 at 06:08:57PM -0300, James Almer wrote: > > On 1/24/2019 5:53 PM, Rostislav Pehlivanov wrote: > > > On Thu, 24 Jan 2019 at 20:38, Marton Balint wrote: > > > > > >> Signed-off-by: Marton Balint > > >> --- > > >> doc/APIchanges | 3 +++ > > >> libavutil/attributes.h | 8 > > >> libavutil/version.h| 2 +- > > >> 3 files changed, 12 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/doc/APIchanges b/doc/APIchanges > > >> index a39a3ff2ba..38b5b980a6 100644 > > >> --- a/doc/APIchanges > > >> +++ b/doc/APIchanges > > >> @@ -15,6 +15,9 @@ libavutil: 2017-10-21 > > >> > > >> API changes, most recent first: > > >> > > >> +2019-01-xx - xx - lavu 56.27.100 - attributes.h > > >> + Add av_likely and av_unlikely > > >> + > > >> 2019-01-08 - xx - lavu 56.26.100 - frame.h > > >>Add AV_FRAME_DATA_REGIONS_OF_INTEREST > > >> > > >> diff --git a/libavutil/attributes.h b/libavutil/attributes.h > > >> index ced108aa2c..60972e5109 100644 > > >> --- a/libavutil/attributes.h > > >> +++ b/libavutil/attributes.h > > >> @@ -164,4 +164,12 @@ > > >> #define av_noreturn > > >> #endif > > >> > > >> +#if AV_GCC_VERSION_AT_LEAST(2,96) || defined(__clang__) > > >> +# define av_likely(x) __builtin_expect(!!(x), 1) > > >> +# define av_unlikely(x)__builtin_expect(!!(x), 0) > > >> +#else > > >> +# define av_likely(x) (x) > > >> +# define av_unlikely(x)(x) > > >> +#endif > > >> + > > >> #endif /* AVUTIL_ATTRIBUTES_H */ > > >> diff --git a/libavutil/version.h b/libavutil/version.h > > >> index 1fcdea95bf..12b4f9fc3a 100644 > > >> --- a/libavutil/version.h > > >> +++ b/libavutil/version.h > > >> @@ -79,7 +79,7 @@ > > >> */ > > >> > > >> #define LIBAVUTIL_VERSION_MAJOR 56 > > >> -#define LIBAVUTIL_VERSION_MINOR 26 > > >> +#define LIBAVUTIL_VERSION_MINOR 27 > > >> #define LIBAVUTIL_VERSION_MICRO 100 > > >> > > >> #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, > > >> \ > > >> -- > > >> 2.16.4 > > >> > > >> ___ > > >> ffmpeg-devel mailing list > > >> ffmpeg-devel@ffmpeg.org > > >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > > > > > > > > NAK, NAK, NAK. > > > I will NAK the hell out of any patch that does that. They're completely > > > unnecessary, they're commonly used by complete idiots who add them > > > thinking > > > their code will go faster (and it might - only on their antiquated GCC > > > 3.1), they're religiously used by the same people and won't back down on > > > using them and finally they're ugly when added to every single bloody > > > branch and the people who maintain such code will demand they be added to > > > every single bloody branch for no reason other that what I've just > > > iterated > > > on. > > > The ONLY way I'll accept them is in platform-specific files, e.g. > > > libavcodec/ppc/*, and even then only on non-x86 arches (which need them > > > for > > > having bad compilers with primitive processors having awful branch > > > prediction) and even then I'd never accept their inclusioin into > > > system-installed headers. > > > > What about hot loops with branches where you can use these as a hint for > > the compiler to assume a specific outcome is highly unlikely to happen, > > like alloc errors, corner cases in some codec, etc, which it simply has > > no way to correctly guess at compile time? > > > > I don't think it should be NAKed because it's misused in other projects. > > We're not other projects. We should instead simply ask the patch writer > > to provide numbers that prove using it makes a difference in their code > > with a recent version of GCC/Clang, and if it's negligible or within the > > margin of error we'll simply ask to remove it to keep the code clean. > > if we want to ensure this, it can be enforced easily > we have fate-source, that can check litterally for each av_(un)likely > to contain a comment on the same line listing a non negligible performance > effect. as in // branch hint increases speed by 5% I'm generally not a fan of these hints at all, since the majority of cases its just noise. The performance impact can vary extremely between compilers and CPUs used, and in average its probably minimal at best. Even if you comment it with some speed number, it'll most of the time be limited to one specific combination of compiler and CPU, and as such any documented numbers are mostly meaningless. Which is the entire problem with these likely/unlikely hints in the first place. If you want to enforce using them in places where it "matters", then how do you define that? One compiler on one system? Two compiler? Two systems? Two architectures even? You easily run into a huge potential for either endless bickering about numbers, or a lot of questionable changes with unreproducable "improvements" - ie. the abuse you see in every other project that
Re: [FFmpeg-devel] [PATCH 1/3] avutil/attributes: add av_likely and av_unlikely
On Thu, Jan 24, 2019 at 06:08:57PM -0300, James Almer wrote: > On 1/24/2019 5:53 PM, Rostislav Pehlivanov wrote: > > On Thu, 24 Jan 2019 at 20:38, Marton Balint wrote: > > > >> Signed-off-by: Marton Balint > >> --- > >> doc/APIchanges | 3 +++ > >> libavutil/attributes.h | 8 > >> libavutil/version.h| 2 +- > >> 3 files changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/doc/APIchanges b/doc/APIchanges > >> index a39a3ff2ba..38b5b980a6 100644 > >> --- a/doc/APIchanges > >> +++ b/doc/APIchanges > >> @@ -15,6 +15,9 @@ libavutil: 2017-10-21 > >> > >> API changes, most recent first: > >> > >> +2019-01-xx - xx - lavu 56.27.100 - attributes.h > >> + Add av_likely and av_unlikely > >> + > >> 2019-01-08 - xx - lavu 56.26.100 - frame.h > >>Add AV_FRAME_DATA_REGIONS_OF_INTEREST > >> > >> diff --git a/libavutil/attributes.h b/libavutil/attributes.h > >> index ced108aa2c..60972e5109 100644 > >> --- a/libavutil/attributes.h > >> +++ b/libavutil/attributes.h > >> @@ -164,4 +164,12 @@ > >> #define av_noreturn > >> #endif > >> > >> +#if AV_GCC_VERSION_AT_LEAST(2,96) || defined(__clang__) > >> +# define av_likely(x) __builtin_expect(!!(x), 1) > >> +# define av_unlikely(x)__builtin_expect(!!(x), 0) > >> +#else > >> +# define av_likely(x) (x) > >> +# define av_unlikely(x)(x) > >> +#endif > >> + > >> #endif /* AVUTIL_ATTRIBUTES_H */ > >> diff --git a/libavutil/version.h b/libavutil/version.h > >> index 1fcdea95bf..12b4f9fc3a 100644 > >> --- a/libavutil/version.h > >> +++ b/libavutil/version.h > >> @@ -79,7 +79,7 @@ > >> */ > >> > >> #define LIBAVUTIL_VERSION_MAJOR 56 > >> -#define LIBAVUTIL_VERSION_MINOR 26 > >> +#define LIBAVUTIL_VERSION_MINOR 27 > >> #define LIBAVUTIL_VERSION_MICRO 100 > >> > >> #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ > >> -- > >> 2.16.4 > >> > >> ___ > >> ffmpeg-devel mailing list > >> ffmpeg-devel@ffmpeg.org > >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > > > > > NAK, NAK, NAK. > > I will NAK the hell out of any patch that does that. They're completely > > unnecessary, they're commonly used by complete idiots who add them thinking > > their code will go faster (and it might - only on their antiquated GCC > > 3.1), they're religiously used by the same people and won't back down on > > using them and finally they're ugly when added to every single bloody > > branch and the people who maintain such code will demand they be added to > > every single bloody branch for no reason other that what I've just iterated > > on. > > The ONLY way I'll accept them is in platform-specific files, e.g. > > libavcodec/ppc/*, and even then only on non-x86 arches (which need them for > > having bad compilers with primitive processors having awful branch > > prediction) and even then I'd never accept their inclusioin into > > system-installed headers. > > What about hot loops with branches where you can use these as a hint for > the compiler to assume a specific outcome is highly unlikely to happen, > like alloc errors, corner cases in some codec, etc, which it simply has > no way to correctly guess at compile time? > > I don't think it should be NAKed because it's misused in other projects. > We're not other projects. We should instead simply ask the patch writer > to provide numbers that prove using it makes a difference in their code > with a recent version of GCC/Clang, and if it's negligible or within the > margin of error we'll simply ask to remove it to keep the code clean. if we want to ensure this, it can be enforced easily we have fate-source, that can check litterally for each av_(un)likely to contain a comment on the same line listing a non negligible performance effect. as in // branch hint increases speed by 5% OTOH, it may make more sense to gather branch statistics at runtime and include that in the next build without smudging the source thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB When you are offended at any man's fault, turn to yourself and study your own failings. Then you will forget your anger. -- Epictetus signature.asc Description: PGP signature ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avutil/attributes: add av_likely and av_unlikely
On Thu, 24 Jan 2019, James Almer wrote: On 1/24/2019 5:53 PM, Rostislav Pehlivanov wrote: On Thu, 24 Jan 2019 at 20:38, Marton Balint wrote: Signed-off-by: Marton Balint --- doc/APIchanges | 3 +++ libavutil/attributes.h | 8 libavutil/version.h| 2 +- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/doc/APIchanges b/doc/APIchanges index a39a3ff2ba..38b5b980a6 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,9 @@ libavutil: 2017-10-21 API changes, most recent first: +2019-01-xx - xx - lavu 56.27.100 - attributes.h + Add av_likely and av_unlikely + 2019-01-08 - xx - lavu 56.26.100 - frame.h Add AV_FRAME_DATA_REGIONS_OF_INTEREST diff --git a/libavutil/attributes.h b/libavutil/attributes.h index ced108aa2c..60972e5109 100644 --- a/libavutil/attributes.h +++ b/libavutil/attributes.h @@ -164,4 +164,12 @@ #define av_noreturn #endif +#if AV_GCC_VERSION_AT_LEAST(2,96) || defined(__clang__) +# define av_likely(x) __builtin_expect(!!(x), 1) +# define av_unlikely(x)__builtin_expect(!!(x), 0) +#else +# define av_likely(x) (x) +# define av_unlikely(x)(x) +#endif + #endif /* AVUTIL_ATTRIBUTES_H */ diff --git a/libavutil/version.h b/libavutil/version.h index 1fcdea95bf..12b4f9fc3a 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -79,7 +79,7 @@ */ #define LIBAVUTIL_VERSION_MAJOR 56 -#define LIBAVUTIL_VERSION_MINOR 26 +#define LIBAVUTIL_VERSION_MINOR 27 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ -- 2.16.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel NAK, NAK, NAK. I will NAK the hell out of any patch that does that. They're completely unnecessary, they're commonly used by complete idiots who add them thinking their code will go faster (and it might - only on their antiquated GCC 3.1), they're religiously used by the same people and won't back down on using them and finally they're ugly when added to every single bloody branch and the people who maintain such code will demand they be added to every single bloody branch for no reason other that what I've just iterated on. The ONLY way I'll accept them is in platform-specific files, e.g. libavcodec/ppc/*, and even then only on non-x86 arches (which need them for having bad compilers with primitive processors having awful branch prediction) and even then I'd never accept their inclusioin into system-installed headers. What about hot loops with branches where you can use these as a hint for the compiler to assume a specific outcome is highly unlikely to happen, like alloc errors, corner cases in some codec, etc, which it simply has no way to correctly guess at compile time? I don't think it should be NAKed because it's misused in other projects. We're not other projects. We should instead simply ask the patch writer to provide numbers that prove using it makes a difference in their code with a recent version of GCC/Clang, and if it's negligible or within the margin of error we'll simply ask to remove it to keep the code clean. Well, the reason behind it is patch 3/3 where it matters accordig to my tests. I wonder if it is only my compiler version where it makes a difference. Regards, Marton ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avutil/attributes: add av_likely and av_unlikely
On 1/24/2019 5:53 PM, Rostislav Pehlivanov wrote: > On Thu, 24 Jan 2019 at 20:38, Marton Balint wrote: > >> Signed-off-by: Marton Balint >> --- >> doc/APIchanges | 3 +++ >> libavutil/attributes.h | 8 >> libavutil/version.h| 2 +- >> 3 files changed, 12 insertions(+), 1 deletion(-) >> >> diff --git a/doc/APIchanges b/doc/APIchanges >> index a39a3ff2ba..38b5b980a6 100644 >> --- a/doc/APIchanges >> +++ b/doc/APIchanges >> @@ -15,6 +15,9 @@ libavutil: 2017-10-21 >> >> API changes, most recent first: >> >> +2019-01-xx - xx - lavu 56.27.100 - attributes.h >> + Add av_likely and av_unlikely >> + >> 2019-01-08 - xx - lavu 56.26.100 - frame.h >>Add AV_FRAME_DATA_REGIONS_OF_INTEREST >> >> diff --git a/libavutil/attributes.h b/libavutil/attributes.h >> index ced108aa2c..60972e5109 100644 >> --- a/libavutil/attributes.h >> +++ b/libavutil/attributes.h >> @@ -164,4 +164,12 @@ >> #define av_noreturn >> #endif >> >> +#if AV_GCC_VERSION_AT_LEAST(2,96) || defined(__clang__) >> +# define av_likely(x) __builtin_expect(!!(x), 1) >> +# define av_unlikely(x)__builtin_expect(!!(x), 0) >> +#else >> +# define av_likely(x) (x) >> +# define av_unlikely(x)(x) >> +#endif >> + >> #endif /* AVUTIL_ATTRIBUTES_H */ >> diff --git a/libavutil/version.h b/libavutil/version.h >> index 1fcdea95bf..12b4f9fc3a 100644 >> --- a/libavutil/version.h >> +++ b/libavutil/version.h >> @@ -79,7 +79,7 @@ >> */ >> >> #define LIBAVUTIL_VERSION_MAJOR 56 >> -#define LIBAVUTIL_VERSION_MINOR 26 >> +#define LIBAVUTIL_VERSION_MINOR 27 >> #define LIBAVUTIL_VERSION_MICRO 100 >> >> #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ >> -- >> 2.16.4 >> >> ___ >> ffmpeg-devel mailing list >> ffmpeg-devel@ffmpeg.org >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > > > NAK, NAK, NAK. > I will NAK the hell out of any patch that does that. They're completely > unnecessary, they're commonly used by complete idiots who add them thinking > their code will go faster (and it might - only on their antiquated GCC > 3.1), they're religiously used by the same people and won't back down on > using them and finally they're ugly when added to every single bloody > branch and the people who maintain such code will demand they be added to > every single bloody branch for no reason other that what I've just iterated > on. > The ONLY way I'll accept them is in platform-specific files, e.g. > libavcodec/ppc/*, and even then only on non-x86 arches (which need them for > having bad compilers with primitive processors having awful branch > prediction) and even then I'd never accept their inclusioin into > system-installed headers. What about hot loops with branches where you can use these as a hint for the compiler to assume a specific outcome is highly unlikely to happen, like alloc errors, corner cases in some codec, etc, which it simply has no way to correctly guess at compile time? I don't think it should be NAKed because it's misused in other projects. We're not other projects. We should instead simply ask the patch writer to provide numbers that prove using it makes a difference in their code with a recent version of GCC/Clang, and if it's negligible or within the margin of error we'll simply ask to remove it to keep the code clean. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avutil/attributes: add av_likely and av_unlikely
On 24/01/2019 20:53, Rostislav Pehlivanov wrote: > NAK, NAK, NAK. Seconded. Please, please, no. - Derek ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Re: [FFmpeg-devel] [PATCH 1/3] avutil/attributes: add av_likely and av_unlikely
On Thu, 24 Jan 2019 at 20:38, Marton Balint wrote: > Signed-off-by: Marton Balint > --- > doc/APIchanges | 3 +++ > libavutil/attributes.h | 8 > libavutil/version.h| 2 +- > 3 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/doc/APIchanges b/doc/APIchanges > index a39a3ff2ba..38b5b980a6 100644 > --- a/doc/APIchanges > +++ b/doc/APIchanges > @@ -15,6 +15,9 @@ libavutil: 2017-10-21 > > API changes, most recent first: > > +2019-01-xx - xx - lavu 56.27.100 - attributes.h > + Add av_likely and av_unlikely > + > 2019-01-08 - xx - lavu 56.26.100 - frame.h >Add AV_FRAME_DATA_REGIONS_OF_INTEREST > > diff --git a/libavutil/attributes.h b/libavutil/attributes.h > index ced108aa2c..60972e5109 100644 > --- a/libavutil/attributes.h > +++ b/libavutil/attributes.h > @@ -164,4 +164,12 @@ > #define av_noreturn > #endif > > +#if AV_GCC_VERSION_AT_LEAST(2,96) || defined(__clang__) > +# define av_likely(x) __builtin_expect(!!(x), 1) > +# define av_unlikely(x)__builtin_expect(!!(x), 0) > +#else > +# define av_likely(x) (x) > +# define av_unlikely(x)(x) > +#endif > + > #endif /* AVUTIL_ATTRIBUTES_H */ > diff --git a/libavutil/version.h b/libavutil/version.h > index 1fcdea95bf..12b4f9fc3a 100644 > --- a/libavutil/version.h > +++ b/libavutil/version.h > @@ -79,7 +79,7 @@ > */ > > #define LIBAVUTIL_VERSION_MAJOR 56 > -#define LIBAVUTIL_VERSION_MINOR 26 > +#define LIBAVUTIL_VERSION_MINOR 27 > #define LIBAVUTIL_VERSION_MICRO 100 > > #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ > -- > 2.16.4 > > ___ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel NAK, NAK, NAK. I will NAK the hell out of any patch that does that. They're completely unnecessary, they're commonly used by complete idiots who add them thinking their code will go faster (and it might - only on their antiquated GCC 3.1), they're religiously used by the same people and won't back down on using them and finally they're ugly when added to every single bloody branch and the people who maintain such code will demand they be added to every single bloody branch for no reason other that what I've just iterated on. The ONLY way I'll accept them is in platform-specific files, e.g. libavcodec/ppc/*, and even then only on non-x86 arches (which need them for having bad compilers with primitive processors having awful branch prediction) and even then I'd never accept their inclusioin into system-installed headers. ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
[FFmpeg-devel] [PATCH 1/3] avutil/attributes: add av_likely and av_unlikely
Signed-off-by: Marton Balint --- doc/APIchanges | 3 +++ libavutil/attributes.h | 8 libavutil/version.h| 2 +- 3 files changed, 12 insertions(+), 1 deletion(-) diff --git a/doc/APIchanges b/doc/APIchanges index a39a3ff2ba..38b5b980a6 100644 --- a/doc/APIchanges +++ b/doc/APIchanges @@ -15,6 +15,9 @@ libavutil: 2017-10-21 API changes, most recent first: +2019-01-xx - xx - lavu 56.27.100 - attributes.h + Add av_likely and av_unlikely + 2019-01-08 - xx - lavu 56.26.100 - frame.h Add AV_FRAME_DATA_REGIONS_OF_INTEREST diff --git a/libavutil/attributes.h b/libavutil/attributes.h index ced108aa2c..60972e5109 100644 --- a/libavutil/attributes.h +++ b/libavutil/attributes.h @@ -164,4 +164,12 @@ #define av_noreturn #endif +#if AV_GCC_VERSION_AT_LEAST(2,96) || defined(__clang__) +# define av_likely(x) __builtin_expect(!!(x), 1) +# define av_unlikely(x)__builtin_expect(!!(x), 0) +#else +# define av_likely(x) (x) +# define av_unlikely(x)(x) +#endif + #endif /* AVUTIL_ATTRIBUTES_H */ diff --git a/libavutil/version.h b/libavutil/version.h index 1fcdea95bf..12b4f9fc3a 100644 --- a/libavutil/version.h +++ b/libavutil/version.h @@ -79,7 +79,7 @@ */ #define LIBAVUTIL_VERSION_MAJOR 56 -#define LIBAVUTIL_VERSION_MINOR 26 +#define LIBAVUTIL_VERSION_MINOR 27 #define LIBAVUTIL_VERSION_MICRO 100 #define LIBAVUTIL_VERSION_INT AV_VERSION_INT(LIBAVUTIL_VERSION_MAJOR, \ -- 2.16.4 ___ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
