[FFmpeg-devel] [PATCH 1/3] lavu/attributes: introduce av_likely, av_unlikely

[Ronald S. Bultje]

Hi,

On Sat, Feb 27, 2016 at 8:11 AM, Ganesh Ajjanagadde <gajjanag at gmail.com>
wrote:

> On Thu, Feb 25, 2016 at 8:55 AM, Hendrik Leppkes <h.leppkes at gmail.com>
> wrote:
> > On Thu, Feb 25, 2016 at 8:27 AM, wm4 <nfxjfg at googlemail.com> wrote:
> >> On Thu, 25 Feb 2016 15:48:17 +1100
> >> Matt Oliver <protogonoi at gmail.com> wrote:
> >>
> >>> On 25 February 2016 at 13:20, Ganesh Ajjanagadde <gajjanag at gmail.com>
> wrote:
> >>>
> >>> > From: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> >>> >
> >>> > These use __builtin_expect, and may be useful for optimizing nearly
> >>> > certain branches, to yield size and/or speed improvements.
> >>> >
> >>> > Note that this is used in the Linux kernel for the same purpose. For
> >>> > some idea as to potential benefits, see e.g
> >>> >
> http://blog.man7.org/2012/10/how-much-do-builtinexpect-likely-and.html.
> >>> >
> >>> > Signed-off-by: Ganesh Ajjanagadde <gajjanagadde at gmail.com>
> >>> > ---
> >>> >  libavutil/attributes.h | 8 ++++++++
> >>> >  1 file changed, 8 insertions(+)
> >>> >
> >>> > diff --git a/libavutil/attributes.h b/libavutil/attributes.h
> >>> > index 5c6b9de..1547033 100644
> >>> > --- a/libavutil/attributes.h
> >>> > +++ b/libavutil/attributes.h
> >>> > @@ -58,6 +58,14 @@
> >>> >  #    define av_warn_unused_result
> >>> >  #endif
> >>> >
> >>> > +#if AV_GCC_VERSION_AT_LEAST(3,0)
> >>> > +#    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
> >>> > +
> >>> >  #if AV_GCC_VERSION_AT_LEAST(3,1)
> >>> >  #    define av_noinline __attribute__((noinline))
> >>> >  #elif defined(_MSC_VER)
> >>> >
> >>>
> >>> Ive used these builtins before and can confirm that they are useful
> >>> (although requires devs to use them obviously). I will also point out
> that
> >>> these are also supported by ICC/ICL as well.
> >>
> >> They also make code ugly as fuck, and are more cargo-cult than anything
> >> else. They might possibly be ok in some very critical parts of the
> >> code, but otherwise not at all.
> >>
> >
> > I agree with wm4 on the ugly aspect, I always hate reading code from
> > projects that use it, its terrible on readability.
> > If its used at all, it should be proven that (a) the function is
> > performance relevant, and (b) that the improvement is actually
> > tangible in the grand scheme.
>
> I mostly agree with the sentiments. Patchset shelved until a
> non-trivial, critical use case can be found.


I can think of a few. E.g. buffer overflow protection in bitstream reader
(get_bits.h), maybe the arith readers (vp56.h, cabac.h) could also use
these. These codepaths are actually quite performance sensitive and I'm not
sure they're all running hand-coded assembly.

Ronald