Re: [PATCH] Use __builtin_expect() within gcc_assert().

[David Daney <ddaney at caviumnetworks dot com>]

David Daney wrote:
> Andrew Pinski wrote:
> > This should not be needed if fancy_abort was marked as noreturn.
>
> It is probably documented somewhere that noreturn functions influence 
> branch prediction in this manner.
>
> I withdraw the patch.

Actually, maybe I don't.

If the patch were a truly doing nothing, one would expect the generated 
code to be identical.  It is not.  There are many objects in gcc that 
show different sizes from the size command.

David Daney

> David Daney
>
>
> > Thanks,
> > Andrew Pinski
> >
> > Sent from my iPhone
> >
> > On Jul 1, 2009, at 9:29 AM, David Daney <ddaney@caviumnetworks.com> 
> > wrote:
> >
> > > This patch adds a __builtin_expect() to the gcc_assert macro.  We
> > > expect that all assertions will be true, so it seems plausible to use
> > > __builtin_expect here.
> > >
> > > Just above the patched chunk, we already add a definition of
> > > __builtin_expect for compilers that don't implement it, so this should
> > > not adversely affect the ability to bootstrap with non-GCC (or very
> > > old GCC) compilers.
> > >
> > > Before the patch in gcc/ (best of three consecutive runs):
> > >
> > > $ rm *.o
> > > $ time make cc1
> > > .
> > > .
> > > .
> > > real    6m7.041s
> > > user    5m43.602s
> > > sys    0m21.602s
> > >
> > > After the patch
> > >
> > > real    6m6.291s
> > > user    5m42.921s
> > > sys    0m21.763s
> > >
> > >
> > > A savings of 0.681s of user time or 0.681/342 = 0.2%.  Not a lot but
> > > it seems to be consistently be a very small amount faster.  However, I
> > > don't discount the possibility that it could all be measurement noise
> > > either.
> > >
> > > Bootstrapped and tested on x86_64-pc-linux-gnu.
> > >
> > > OK to commit?
> > >
> > > 2009-07-01  David Daney  <ddaney@caviumnetworks.com>
> > >
> > >    * system.h (gcc_assert): Use __builtin_expect.
> > > Index: system.h
> > > ===================================================================
> > > --- system.h    (revision 149061)
> > > +++ system.h    (working copy)
> > > @@ -575,7 +575,8 @@ extern void fancy_abort (const char *, i
> > > /* Use gcc_assert(EXPR) to test invariants.  */
> > > #if ENABLE_ASSERT_CHECKING
> > > #define gcc_assert(EXPR)                        \
> > > -   ((void)(!(EXPR) ? fancy_abort (__FILE__, __LINE__, __FUNCTION__), 
> > > 0 : 0))
> > > +  ((void)(__builtin_expect (!(EXPR), 0) ?                \
> > > +      fancy_abort (__FILE__, __LINE__, __FUNCTION__), 0 : 0))
> > > #else
> > > /* Include EXPR, so that unused variable warnings do not occur.  */
> > > #define gcc_assert(EXPR) ((void)(0 && (EXPR)))