| Message ID | 20190124203801.18484-1-cus@passwd.hu |
|---|---|
| State | Withdrawn |
| Headers | show |
On Thu, 24 Jan 2019 at 20:38, Marton Balint <cus@passwd.hu> wrote: > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > 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 - xxxxxxxxxx - lavu 56.27.100 - attributes.h > + Add av_likely and av_unlikely > + > 2019-01-08 - xxxxxxxxxx - 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.
On 24/01/2019 20:53, Rostislav Pehlivanov wrote:
> NAK, NAK, NAK.
Seconded. Please, please, no.
- Derek
On 1/24/2019 5:53 PM, Rostislav Pehlivanov wrote: > On Thu, 24 Jan 2019 at 20:38, Marton Balint <cus@passwd.hu> wrote: > >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> 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 - xxxxxxxxxx - lavu 56.27.100 - attributes.h >> + Add av_likely and av_unlikely >> + >> 2019-01-08 - xxxxxxxxxx - 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.
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 <cus@passwd.hu> wrote: >> >>> Signed-off-by: Marton Balint <cus@passwd.hu> >>> --- >>> 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 - xxxxxxxxxx - lavu 56.27.100 - attributes.h >>> + Add av_likely and av_unlikely >>> + >>> 2019-01-08 - xxxxxxxxxx - 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
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 <cus@passwd.hu> wrote: > > > >> Signed-off-by: Marton Balint <cus@passwd.hu> > >> --- > >> 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 - xxxxxxxxxx - lavu 56.27.100 - attributes.h > >> + Add av_likely and av_unlikely > >> + > >> 2019-01-08 - xxxxxxxxxx - 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 [...]
On Thu, Jan 24, 2019 at 11:06 PM Michael Niedermayer <michael@niedermayer.cc> 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 <cus@passwd.hu> wrote: > > > > > >> Signed-off-by: Marton Balint <cus@passwd.hu> > > >> --- > > >> 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 - xxxxxxxxxx - lavu 56.27.100 - attributes.h > > >> + Add av_likely and av_unlikely > > >> + > > >> 2019-01-08 - xxxxxxxxxx - 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 has them. > > OTOH, it may make more sense to gather branch statistics at runtime and > include that in the next build without smudging the source > PGO (Profile Guided Optimizations) is of course something that exists today, but pulling it off on a broad scale with a project like ffmpeg would be quite a challenge. Of course if all you care about is that one specific component is fast, then it can probably be done. - Hendrik
On 1/24/19 11:53 PM, Rostislav Pehlivanov wrote: > On Thu, 24 Jan 2019 at 20:38, Marton Balint <cus@passwd.hu> wrote: > >> Signed-off-by: Marton Balint <cus@passwd.hu> >> --- >> 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 - xxxxxxxxxx - lavu 56.27.100 - attributes.h >> + Add av_likely and av_unlikely >> + >> 2019-01-08 - xxxxxxxxxx - 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.
On Fri, Jan 25, 2019 at 01:28:51AM +0100, Hendrik Leppkes wrote: > On Thu, Jan 24, 2019 at 11:06 PM Michael Niedermayer > <michael@niedermayer.cc> 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 <cus@passwd.hu> wrote: > > > > > > > >> Signed-off-by: Marton Balint <cus@passwd.hu> > > > >> --- > > > >> 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 - xxxxxxxxxx - lavu 56.27.100 - attributes.h > > > >> + Add av_likely and av_unlikely > > > >> + > > > >> 2019-01-08 - xxxxxxxxxx - 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 has > them. I have no oppinion about these branch hints but this is starting to feel alot more like a debate about code indention or some religious debate than a technical feature. The arguments ive seen (and remember) ATM basically states or feel as: * "we cannot as a group benchmark code in a usefull way", even though we seem able to do that in other cases * "It will spread" even though it hasnt for example the (un)likely() macros in our own alpha code have been there peacefully for a very long term. * Theres bad code "out there" using them so all use of them is bad or leads to bad code. compare "goto" use where similar arguments exists, we use gotos i think in many cases our use of gotos makes code clearer while you can do really really bad code with gotos if used unwisely and no iam not suggesting we start using these branch hints more, i just think they should be treated like any other tool or feature. thanks [...]
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 - xxxxxxxxxx - lavu 56.27.100 - attributes.h + Add av_likely and av_unlikely + 2019-01-08 - xxxxxxxxxx - 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, \
Signed-off-by: Marton Balint <cus@passwd.hu> --- doc/APIchanges | 3 +++ libavutil/attributes.h | 8 ++++++++ libavutil/version.h | 2 +- 3 files changed, 12 insertions(+), 1 deletion(-)