[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Chicken-hackers] Add branch prediction for C_demand checks [was: Re
From: |
Jim Ursetto |
Subject: |
Re: [Chicken-hackers] Add branch prediction for C_demand checks [was: Re: [PATCH] Statically determine if argvector can be reused] |
Date: |
Fri, 30 Dec 2016 10:28:18 -0600 |
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