diff mbox

[3/3] elf/dl-lookup.c: Use __glibc_likely and __glibc_unlikely

Message ID 1397230188-14581-3-git-send-email-will.newton@linaro.org
State Accepted
Commit cc7d044752ded296cf737c311f786e87b88180ec
Headers show

Commit Message

Will Newton April 11, 2014, 3:29 p.m. UTC 
Convert all uses of __builtin_expect to __glibc_likely and
__glibc_unlikely. Most of these are trivial boolean expressions
but a few were not. In particular the use of __builtin_expect in
the switch expression in do_lookup_x has been removed. Verified
that there are no code changes on x86_64 and ARM aside from line
numbers.

ChangeLog:

2014-04-11  Will Newton  <will.newton@linaro.org>

	* elf/dl-lookup.c: Use __glibc_unlikely and __glibc_likely
	rather than __builtin_expect.
---
 elf/dl-lookup.c | 43 +++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 22 deletions(-)

Comments

Ondřej Bílka April 11, 2014, 9:15 p.m. UTC | #1 
On Fri, Apr 11, 2014 at 04:29:48PM +0100, Will Newton wrote:
> Convert all uses of __builtin_expect to __glibc_likely and
> __glibc_unlikely. Most of these are trivial boolean expressions
> but a few were not. In particular the use of __builtin_expect in
> the switch expression in do_lookup_x has been removed. Verified
> that there are no code changes on x86_64 and ARM aside from line
> numbers.
>
mostly ok, with following nits

>  {
>    unsigned int stt = ELFW(ST_TYPE) (sym->st_info);
>    assert (ELF_RTYPE_CLASS_PLT == 1);
> -  if (__builtin_expect ((sym->st_value == 0 /* No value.  */
> -			 && stt != STT_TLS)
> -			|| ELF_MACHINE_SYM_NO_MATCH (sym)
> -			|| (type_class & (sym->st_shndx == SHN_UNDEF)),
> -			0))
> +  if (__glibc_unlikely((sym->st_value == 0 /* No value.  */
> +			&& stt != STT_TLS)
> +		       || ELF_MACHINE_SYM_NO_MATCH (sym)
> +		       || (type_class & (sym->st_shndx == SHN_UNDEF))))
>      return NULL;
space before (.

>  	{
>  	found_it:
> -	  switch (__builtin_expect (ELFW(ST_BIND) (sym->st_info), STB_GLOBAL))
> +	  switch (ELFW(ST_BIND) (sym->st_info))

why did you dropped expect?
Will Newton April 14, 2014, 9:23 a.m. UTC | #2 
On 11 April 2014 22:15, Ondřej Bílka <neleai@seznam.cz> wrote:
> On Fri, Apr 11, 2014 at 04:29:48PM +0100, Will Newton wrote:
>> Convert all uses of __builtin_expect to __glibc_likely and
>> __glibc_unlikely. Most of these are trivial boolean expressions
>> but a few were not. In particular the use of __builtin_expect in
>> the switch expression in do_lookup_x has been removed. Verified
>> that there are no code changes on x86_64 and ARM aside from line
>> numbers.
>>
> mostly ok, with following nits
>
>>  {
>>    unsigned int stt = ELFW(ST_TYPE) (sym->st_info);
>>    assert (ELF_RTYPE_CLASS_PLT == 1);
>> -  if (__builtin_expect ((sym->st_value == 0 /* No value.  */
>> -                      && stt != STT_TLS)
>> -                     || ELF_MACHINE_SYM_NO_MATCH (sym)
>> -                     || (type_class & (sym->st_shndx == SHN_UNDEF)),
>> -                     0))
>> +  if (__glibc_unlikely((sym->st_value == 0 /* No value.  */
>> +                     && stt != STT_TLS)
>> +                    || ELF_MACHINE_SYM_NO_MATCH (sym)
>> +                    || (type_class & (sym->st_shndx == SHN_UNDEF))))
>>      return NULL;
> space before (.

Thanks, will fix.

>>       {
>>       found_it:
>> -       switch (__builtin_expect (ELFW(ST_BIND) (sym->st_info), STB_GLOBAL))
>> +       switch (ELFW(ST_BIND) (sym->st_info))
>
> why did you dropped expect?

It doesn't cause any code generation changes on the compilers I have
tested with and as far as I can tell gcc doesn't make use of
prediction information for switch statements of this size so it has
never done anything. If we really want to special case the branch we
should probably write an explicit if/else condition after profiling
it.
diff mbox

Patch

diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
index 1c6ca89..7e7ab22 100644
--- a/elf/dl-lookup.c
+++ b/elf/dl-lookup.c
@@ -92,11 +92,10 @@  check_match (const char *const undef_name,
 {
   unsigned int stt = ELFW(ST_TYPE) (sym->st_info);
   assert (ELF_RTYPE_CLASS_PLT == 1);
-  if (__builtin_expect ((sym->st_value == 0 /* No value.  */
-			 && stt != STT_TLS)
-			|| ELF_MACHINE_SYM_NO_MATCH (sym)
-			|| (type_class & (sym->st_shndx == SHN_UNDEF)),
-			0))
+  if (__glibc_unlikely((sym->st_value == 0 /* No value.  */
+			&& stt != STT_TLS)
+		       || ELF_MACHINE_SYM_NO_MATCH (sym)
+		       || (type_class & (sym->st_shndx == SHN_UNDEF))))
     return NULL;
 
   /* Ignore all but STT_NOTYPE, STT_OBJECT, STT_FUNC,
@@ -405,8 +404,8 @@  do_lookup_x (const char *undef_name, uint_fast32_t new_hash,
 	  unsigned int hashbit2 = ((new_hash >> map->l_gnu_shift)
 				   & (__ELF_NATIVE_CLASS - 1));
 
-	  if (__builtin_expect ((bitmask_word >> hashbit1)
-				& (bitmask_word >> hashbit2) & 1, 0))
+	  if (__glibc_unlikely ((bitmask_word >> hashbit1)
+				& (bitmask_word >> hashbit2) & 1))
 	    {
 	      Elf32_Word bucket = map->l_gnu_buckets[new_hash
 						     % map->l_nbuckets];
@@ -461,7 +460,7 @@  do_lookup_x (const char *undef_name, uint_fast32_t new_hash,
       if (sym != NULL)
 	{
 	found_it:
-	  switch (__builtin_expect (ELFW(ST_BIND) (sym->st_info), STB_GLOBAL))
+	  switch (ELFW(ST_BIND) (sym->st_info))
 	    {
 	    case STB_WEAK:
 	      /* Weak definition.  Use this value if we don't find another.  */
@@ -495,7 +494,7 @@  do_lookup_x (const char *undef_name, uint_fast32_t new_hash,
       /* If this current map is the one mentioned in the verneed entry
 	 and we have not found a weak entry, it is a bug.  */
       if (symidx == STN_UNDEF && version != NULL && version->filename != NULL
-	  && __builtin_expect (_dl_name_match_p (version->filename, map), 0))
+	  && __glibc_unlikely (_dl_name_match_p (version->filename, map)))
 	return -1;
     }
   while (++i < n);
@@ -776,7 +775,7 @@  _dl_lookup_symbol_x (const char *undef_name, struct link_map *undef_map,
       if (res > 0)
 	break;
 
-      if (__builtin_expect (res, 0) < 0 && skip_map == NULL)
+      if (__glibc_unlikely (res < 0) && skip_map == NULL)
 	{
 	  /* Oh, oh.  The file named in the relocation entry does not
 	     contain the needed symbol.  This code is never reached
@@ -857,7 +856,7 @@  _dl_lookup_symbol_x (const char *undef_name, struct link_map *undef_map,
      in the global scope which was dynamically loaded.  In this case
      we have to prevent the latter from being unloaded unless the
      UNDEF_MAP object is also unloaded.  */
-  if (__builtin_expect (current_value.m->l_type == lt_loaded, 0)
+  if (__glibc_unlikely (current_value.m->l_type == lt_loaded)
       /* Don't do this for explicit lookups as opposed to implicit
 	 runtime lookups.  */
       && (flags & DL_LOOKUP_ADD_DEPENDENCY) != 0
@@ -874,8 +873,8 @@  _dl_lookup_symbol_x (const char *undef_name, struct link_map *undef_map,
   if (__glibc_unlikely (current_value.m->l_used == 0))
     current_value.m->l_used = 1;
 
-  if (__builtin_expect (GLRO(dl_debug_mask)
-			& (DL_DEBUG_BINDINGS|DL_DEBUG_PRELINK), 0))
+  if (__glibc_unlikely (GLRO(dl_debug_mask)
+			& (DL_DEBUG_BINDINGS|DL_DEBUG_PRELINK)))
     _dl_debug_bindings (undef_name, undef_map, ref,
 			&current_value, version, type_class, protected);
 
@@ -892,9 +891,9 @@  _dl_setup_hash (struct link_map *map)
 {
   Elf_Symndx *hash;
 
-  if (__builtin_expect (map->l_info[DT_ADDRTAGIDX (DT_GNU_HASH) + DT_NUM
+  if (__glibc_likely (map->l_info[DT_ADDRTAGIDX (DT_GNU_HASH) + DT_NUM
 				    + DT_THISPROCNUM + DT_VERSIONTAGNUM
-				    + DT_EXTRANUM + DT_VALNUM] != NULL, 1))
+				    + DT_EXTRANUM + DT_VALNUM] != NULL))
     {
       Elf32_Word *hash32
 	= (void *) D_PTR (map, l_info[DT_ADDRTAGIDX (DT_GNU_HASH) + DT_NUM
@@ -973,10 +972,10 @@  _dl_debug_bindings (const char *undef_name, struct link_map *undef_map,
 		       type_class, undef_map);
 	  if (val.s != value->s || val.m != value->m)
 	    conflict = 1;
-	  else if (__builtin_expect (undef_map->l_symbolic_in_local_scope, 0)
+	  else if (__glibc_unlikely (undef_map->l_symbolic_in_local_scope)
 		   && val.s
-		   && __builtin_expect (ELFW(ST_BIND) (val.s->st_info),
-					STB_GLOBAL) == STB_GNU_UNIQUE)
+		   && __glibc_unlikely (ELFW(ST_BIND) (val.s->st_info)
+					== STB_GNU_UNIQUE))
 	    {
 	      /* If it is STB_GNU_UNIQUE and undef_map's l_local_scope
 		 contains any DT_SYMBOLIC libraries, unfortunately there
@@ -1010,11 +1009,11 @@  _dl_debug_bindings (const char *undef_name, struct link_map *undef_map,
 
       if (value->s)
 	{
-	  if (__builtin_expect (ELFW(ST_TYPE) (value->s->st_info)
-				== STT_TLS, 0))
+	  if (__glibc_unlikely (ELFW(ST_TYPE) (value->s->st_info)
+				== STT_TLS))
 	    type_class = 4;
-	  else if (__builtin_expect (ELFW(ST_TYPE) (value->s->st_info)
-				     == STT_GNU_IFUNC, 0))
+	  else if (__glibc_unlikely (ELFW(ST_TYPE) (value->s->st_info)
+				     == STT_GNU_IFUNC))
 	    type_class |= 8;
 	}