[1/8] Add GCC_LIKELY and GCC_UNLIKELY

Message ID 1529008375-20819-2-git-send-email-dmalcolm@redhat.com
State New
Headers show
Series
  • v2 of optimization records patch kit
Related show

Commit Message

David Malcolm June 14, 2018, 8:32 p.m.
The idea is to later use these macros to mark the
  if (dump_enabled_p ())
parts of the compiler as cold, in the hope of helping non-PGO builds
of gcc.

I haven't measured it yet, though.

gcc/ChangeLog:
	* system.h (GCC_LIKELY, GCC_UNLIKELY): New macros, adapted from
	libgfortran.h.
---
 gcc/system.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

-- 
1.8.5.3

Comments

Alexander Monakov June 15, 2018, 6:08 a.m. | #1
On Thu, 14 Jun 2018, David Malcolm wrote:

> The idea is to later use these macros to mark the

>   if (dump_enabled_p ())

> parts of the compiler as cold, in the hope of helping non-PGO builds

> of gcc.


I think a cleaner way to achieve that would be to mark a function called
under the predicate with ATTRIBUTE_COLD: if you have

    if (dump_enabled_p ())
      {
        ...
        dump_something ();
	...
      }

then declaring dump_something with __attribute__((cold)) informs the
compiler that the branch leading to it is unlikely. It also moves the
function body to the "cold" text region, but that may be acceptable.

Or, perhaps even simpler, you can

    #define dump_enabled_p() __builtin_expect (dump_enabled_p (), 0)

> 	* system.h (GCC_LIKELY, GCC_UNLIKELY): New macros, adapted from

> 	libgfortran.h.


I like the idea of these macros, but can their spelling use lowercase please,
like the Linux kernel, Glibc (and even libgfortran.h) spell their variants.

> +/* The following macros can be used to annotate conditions which are likely or

> +   unlikely to be true.  Avoid using them when a condition is only slightly

> +   more likely/less unlikely than average to avoid the performance penalties of

> +   branch misprediction. In addition, as __builtin_expect overrides the compiler

> +   heuristic, do not use in conditions where one of the branches ends with a

> +   call to a function with __attribute__((noreturn)): the compiler internal

> +   heuristic will mark this branch as much less likely as GCC_UNLIKELY() would

> +   do.  */


I realize this is copied verbatim from libgfortran.h, but I think the comment
is not accurately worded and should not be duplicated. I'd suggest simply

/* Shorthands for informing compiler's static branch prediction.  */

> +#define GCC_LIKELY(X)       __builtin_expect(!!(X), 1)

> +#define GCC_UNLIKELY(X)     __builtin_expect(!!(X), 0)


The '!!'s are not necessary here, as far as I can tell.

Alexander
Jeff Law June 15, 2018, 4:31 p.m. | #2
On 06/14/2018 02:32 PM, David Malcolm wrote:
> The idea is to later use these macros to mark the

>   if (dump_enabled_p ())

> parts of the compiler as cold, in the hope of helping non-PGO builds

> of gcc.

> 

> I haven't measured it yet, though.

> 

> gcc/ChangeLog:

> 	* system.h (GCC_LIKELY, GCC_UNLIKELY): New macros, adapted from

> 	libgfortran.h.

ISTM if we're going to bother with this stuff that we should try to be
consistent between glibc and gcc.  Anything else seems like utter
madness to me.

Jeff
Jakub Jelinek June 15, 2018, 4:35 p.m. | #3
On Fri, Jun 15, 2018 at 10:31:26AM -0600, Jeff Law wrote:
> On 06/14/2018 02:32 PM, David Malcolm wrote:

> > The idea is to later use these macros to mark the

> >   if (dump_enabled_p ())

> > parts of the compiler as cold, in the hope of helping non-PGO builds

> > of gcc.

> > 

> > I haven't measured it yet, though.

> > 

> > gcc/ChangeLog:

> > 	* system.h (GCC_LIKELY, GCC_UNLIKELY): New macros, adapted from

> > 	libgfortran.h.

> ISTM if we're going to bother with this stuff that we should try to be

> consistent between glibc and gcc.  Anything else seems like utter

> madness to me.


Do we really need these macros at all?  We are already using
__builtin_expect directly in gcc/ subdirectory (with system.h providing
a dummy macro if not supported by the host compiler).
And I bet GCC developers are all familiar with __builtin_expect.

	Jakub
Jeff Law June 15, 2018, 4:36 p.m. | #4
On 06/15/2018 10:35 AM, Jakub Jelinek wrote:
> On Fri, Jun 15, 2018 at 10:31:26AM -0600, Jeff Law wrote:

>> On 06/14/2018 02:32 PM, David Malcolm wrote:

>>> The idea is to later use these macros to mark the

>>>   if (dump_enabled_p ())

>>> parts of the compiler as cold, in the hope of helping non-PGO builds

>>> of gcc.

>>>

>>> I haven't measured it yet, though.

>>>

>>> gcc/ChangeLog:

>>> 	* system.h (GCC_LIKELY, GCC_UNLIKELY): New macros, adapted from

>>> 	libgfortran.h.

>> ISTM if we're going to bother with this stuff that we should try to be

>> consistent between glibc and gcc.  Anything else seems like utter

>> madness to me.

> 

> Do we really need these macros at all? 

I certainly question this as well.


 We are already using
> __builtin_expect directly in gcc/ subdirectory (with system.h providing

> a dummy macro if not supported by the host compiler).

> And I bet GCC developers are all familiar with __builtin_expect.

Yup.  But I doubt we want to litter the sources with references to them.

jeff
David Malcolm June 15, 2018, 8:04 p.m. | #5
On Fri, 2018-06-15 at 10:36 -0600, Jeff Law wrote:
> On 06/15/2018 10:35 AM, Jakub Jelinek wrote:

> > On Fri, Jun 15, 2018 at 10:31:26AM -0600, Jeff Law wrote:

> > > On 06/14/2018 02:32 PM, David Malcolm wrote:

> > > > The idea is to later use these macros to mark the

> > > >   if (dump_enabled_p ())

> > > > parts of the compiler as cold, in the hope of helping non-PGO

> > > > builds

> > > > of gcc.

> > > > 

> > > > I haven't measured it yet, though.

> > > > 

> > > > gcc/ChangeLog:

> > > > 	* system.h (GCC_LIKELY, GCC_UNLIKELY): New macros,

> > > > adapted from

> > > > 	libgfortran.h.

> > > 

> > > ISTM if we're going to bother with this stuff that we should try

> > > to be

> > > consistent between glibc and gcc.  Anything else seems like utter

> > > madness to me.

> > 

> > Do we really need these macros at all? 

> 

> I certainly question this as well.

> 

> 

>  We are already using

> > __builtin_expect directly in gcc/ subdirectory (with system.h

> > providing

> > a dummy macro if not supported by the host compiler).

> > And I bet GCC developers are all familiar with __builtin_expect.

> 

> Yup.  But I doubt we want to litter the sources with references to

> them.


Indeed.  There's only one place where I was thinking of adding it
(within dump_enabled_p), and as I said I haven't measured the impact
yet.
 
I'll drop this patch from the v3 patch kit.

Dave

Patch

diff --git a/gcc/system.h b/gcc/system.h
index 88dffcc..195acb8 100644
--- a/gcc/system.h
+++ b/gcc/system.h
@@ -720,6 +720,18 @@  extern int vsnprintf (char *, size_t, const char *, va_list);
 #define __builtin_expect(a, b) (a)
 #endif
 
+/* The following macros can be used to annotate conditions which are likely or
+   unlikely to be true.  Avoid using them when a condition is only slightly
+   more likely/less unlikely than average to avoid the performance penalties of
+   branch misprediction. In addition, as __builtin_expect overrides the compiler
+   heuristic, do not use in conditions where one of the branches ends with a
+   call to a function with __attribute__((noreturn)): the compiler internal
+   heuristic will mark this branch as much less likely as GCC_UNLIKELY() would
+   do.  */
+
+#define GCC_LIKELY(X)       __builtin_expect(!!(X), 1)
+#define GCC_UNLIKELY(X)     __builtin_expect(!!(X), 0)
+
 /* Some of the headers included by <memory> can use "abort" within a
    namespace, e.g. "_VSTD::abort();", which fails after we use the
    preprocessor to redefine "abort" as "fancy_abort" below.