[FFmpeg-devel,1/3] avutil/attributes: add av_likely and av_unlikely

Submitted by Marton Balint on Jan. 24, 2019, 8:37 p.m.

Details

Message ID 20190124203801.18484-1-cus@passwd.hu
State New
Headers show

Commit Message

Marton Balint Jan. 24, 2019, 8:37 p.m.
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(-)

Comments

Rostislav Pehlivanov Jan. 24, 2019, 8:53 p.m.
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.
Derek Buitenhuis Jan. 24, 2019, 9:01 p.m.
On 24/01/2019 20:53, Rostislav Pehlivanov wrote:
> NAK, NAK, NAK.

Seconded. Please, please, no.

- Derek
James Almer Jan. 24, 2019, 9:08 p.m.
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.
Marton Balint Jan. 24, 2019, 9:45 p.m.
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
Michael Niedermayer Jan. 24, 2019, 10:05 p.m.
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

[...]
Hendrik Leppkes Jan. 25, 2019, 12:28 a.m.
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
Andrey Semashev Jan. 25, 2019, 8:13 a.m.
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.
Michael Niedermayer Jan. 25, 2019, 8:30 a.m.
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

[...]

Patch hide | download patch | download mbox

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, \