| Message ID | 1529008375-20819-2-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | v2 of optimization records patch kit | expand |
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
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
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
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
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
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.