Re: [Chicken-hackers] Add branch prediction for C_demand checks [was: Re: [PATCH] Statically determine if argvector can be reused]

[Jim Ursetto]

This seems to have changed somewhere between kernel 2.4.37 and 2.6.12, where it 
used to be:

#define likely(x)       __builtin_expect((x),1)
#define unlikely(x)     __builtin_expect((x),0)
 
but I can’t find the exact reason it was changed.

Speculating, I would guess it was changed for the 1 case, where the double 
negation forces a non-zero (true) boolean value to be 1 (as Dan mentioned), and 
you have a definite value to compare against. 

Then, it may have been changed for the 0 case for one of two reasons:

1. Just to keep things similar.
2. The !! also forces the return value to be int, so for example, !!NULL is 
integer 0. The docs for __builtin_expect mention this, as its arguments are 
long integers:

        Since you are limited to integral expressions for exp, you should use 
constructions such as

                  if (__builtin_expect (ptr != NULL, 1))
                        error ();
     
        when testing pointer or floating-point values.

It may work, but you get a warning about implicit conversion under clang or 
under gcc with -Wint-conversion:

$ gcc -o a.out a.c
a.c:4:38: warning: incompatible pointer to integer conversion passing 'void *' 
to parameter of type 'long' [-Wint-conversion]
    printf("%ld\n", __builtin_expect(NULL, 0));

so it is likely the !! is to ensure int type in the 0 case.

Jim

> On Dec 30, 2016, at 12:08 AM, Dan Leslie <address@hidden> wrote:
> 
> Just a guess, but considering that the !! forces the value to resolve to a
> valid bool, either 0 or 1, they ran into an issue where the input to their
> macros wasn't just the outcome of Boolean operators? IE, perhaps they were
> seeing:
> 
>    if (likely(SOME_FUNKY_MACRO))
> 
> And that SOME_FUNKY_MACRO could be any long value greater than or equal to
> zero; or it could expand to a Boolean operation.
> 
> -Dan
> 
> -----Original Message-----
> From: Chicken-hackers
> [mailto:address@hidden On Behalf Of
> Peter Bex
> Sent: December 27, 2016 9:15 AM
> To: chicken-hackers <address@hidden>
> Subject: [Chicken-hackers] Add branch prediction for C_demand checks [was:
> Re: [PATCH] Statically determine if argvector can be reused]
> 
> On Thu, Dec 15, 2016 at 11:55:01PM +0100, Peter Bex wrote:
>> Hi all,
>> 
>> I've been playing around a little bit with "perf" and Valgrind's 
>> cachegrind tool, and I noticed that the number of branch prediction 
>> misses can be reduced if the argvector reusing check can be hardcoded 
>> for cases where we know the size of the calling function's argvector.
>> 
>> Here's a simple patch to make this happen (works on both the master 
>> and chicken-5 branches).
> 
> And here's another one, that adds C_likely() and C_unlikely() macros, a la
> the Linux kernel's likely() and unlikely().  These are simple wrappers for
> __builtin_expect() which tell the compiler which branches in a conditional
> expression are likely to be taken.
> 
> These are now used in generated code, when we do a C_demand() check to
> allocate memory.  This is noticably faster in some benchmarks, and can bring
> down compilation time a bit as well.  Attached also are benchmark results
> for unpatched CHICKEN 5, the previous patch which adds static argvector
> checks and results for both this patch and the static argvector.
> 
> Question: Should this be:
> 
> # define C_unlikely(x)             __builtin_expect((x), 0)
> 
> or, like Linux:
> 
> # define C_unlikely(x)             __builtin_expect(!!(x), 0)
> 
> The latter seems to me like it would add more instructions due to the double
> negation, and the exact value of the expression in an if() is never
> important anyway, as long as it's zero or nonzero.  Right?
> 
> Cheers,
> Peter
> 
> 
> _______________________________________________
> Chicken-hackers mailing list
> address@hidden
> https://lists.nongnu.org/mailman/listinfo/chicken-hackers