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

[Will Newton <will dot newton at linaro dot org>]

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.

-- 
Will Newton
Toolchain Working Group, Linaro