This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [patch] Finish __builtin_expect -> __glibc_{un}likely cleanup in elf/dl-load.c
- From: OndÅej BÃlka <neleai at seznam dot cz>
- To: Paul Pluzhnikov <ppluzhnikov at google dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Thu, 27 Mar 2014 12:42:01 +0100
- Subject: Re: [patch] Finish __builtin_expect -> __glibc_{un}likely cleanup in elf/dl-load.c
- Authentication-results: sourceware.org; auth=none
- References: <ye6qeh1o7ab8 dot fsf at elbrus2 dot mtv dot corp dot google dot com>
On Wed, Mar 26, 2014 at 05:30:03PM -0700, Paul Pluzhnikov wrote:
> Greetings,
>
> This patch cleans up remaining uses of __builtin_expect.
>
> It does change elf/dl-load.o in non-trivial ways, but
> - the tests still pass on Linux/x86_64 and
> - is much smaller, and easier to verify.
>
Looks mostly ok.
> @@ -2104,8 +2103,7 @@ _dl_map_object (struct link_map *loader, const char *name,
> /* If the requested name matches the soname of a loaded object,
> use that object. Elide this check for names that have not
> yet been opened. */
> - if (__glibc_unlikely (l->l_faked != 0)
> - || __builtin_expect (l->l_removed, 0) != 0)
> + if (__glibc_unlikely (l->l_faked != 0 || l->l_removed != 0))
> continue;
> if (!_dl_name_match_p (name, l))
> {
Here it might decrease performance (see below), do you have profile data it migth
make sense even to drop this one.
As written now its optimized to
if (__glibc_unlikely (l->l_faked != 0)
continue;
if (__builtin_expect (l->l_removed, 0) != 0)
continue;
with your change compiler could assume that first part happens in 49%
time and 49% for other part. So its better to trasform that expression
that requires only one jump, here best one is
if (__glibc_unlikely (l->l_faked | l->l_removed != 0))
continue;
> @@ -2230,7 +2228,7 @@ _dl_map_object (struct link_map *loader, const char *name,
>
> #ifdef USE_LDCONFIG
> if (fd == -1
> - && (__builtin_expect (! (mode & __RTLD_SECURE), 1)
> + && (__glibc_likely ((mode & __RTLD_SECURE) == 0)
> || ! INTUSE(__libc_enable_secure))
> && __glibc_likely (GLRO(dl_inhibit_cache) == 0))
> {
Extra parens here.