Messages in this thread | | | Subject | Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage | From | Denis Efremov <> | Date | Wed, 28 Aug 2019 16:14:32 +0300 |
| |
On 8/28/19 4:05 PM, Rasmus Villemoes wrote: > On 28/08/2019 14.33, Denis Efremov wrote: >> On 8/28/19 2:33 PM, Rasmus Villemoes wrote: >>> On 25/08/2019 21.19, Julia Lawall wrote: >>>> >>>> >>>>> On 26 Aug 2019, at 02:59, Denis Efremov <efremov@linux.com> wrote: >>>>> >>>>> >>>>> >>>>>> On 25.08.2019 19:37, Joe Perches wrote: >>>>>>> On Sun, 2019-08-25 at 16:05 +0300, Denis Efremov wrote: >>>>>>> This patch adds coccinelle script for detecting !likely and !unlikely >>>>>>> usage. It's better to use unlikely instead of !likely and vice versa. >>>>>> >>>>>> Please explain _why_ is it better in the changelog. >>>>>> >>>>> >>>>> In my naive understanding the negation (!) before the likely/unlikely >>>>> could confuse the compiler >>>> >>>> As a human I am confused. Is !likely(x) equivalent to x or !x? >>> >>> #undef likely >>> #undef unlikely >>> #define likely(x) (x) >>> #define unlikely(x) (x) >>> >>> should be a semantic no-op. So changing !likely(x) to unlikely(x) is >>> completely wrong. If anything, !likely(x) can be transformed to >>> unlikely(!x). >> >> As far as I could understand it: >> >> # define likely(x) __builtin_expect(!!(x), 1) >> # define unlikely(x) __builtin_expect(!!(x), 0) >> >> From GCC doc: >> __builtin_expect compares the values. The semantics of the built-in are that it is expected that exp == c. > > When I said "semantic" I meant from the C language point of view. Yes, > of course, the whole reason for having these is that we can give hints > to gcc as to which branch is more likely. Replace the dummy defines by > #define likely(x) (!!(x)) if you like - it amounts to the same thing > when it's only ever used in a boolean context. > >> if (!likely(cond)) >> if (!__builtin_expect(!!(cond), 1)) >> if (!((!!(cond)) == 1)) > > You're inventing this comparison to 1. It should be "if (!(!!(cond)))", > but it ends up being equivalent in C. > >> if ((!!(cond)) != 1) and since !! could result in 0 or 1 >> if ((!!(cond)) == 0) > > which in turn is equivalent to !(cond). > >> >> if (unlikely(cond)) >> if (__builtin_expect(!!(cond), 0)) >> if ((!!(cond)) == 0)) > > No, that last transformation is wrong. Yes, the _expectation_ is that > !!(cond) has the value 0, but that does not mean that the whole > condition turns into "does !!(cond) compare equal to 0?" - we _expect_ > that it does, meaning that we expect not to enter the if block. Read the > docs, the value of __builtin_expect(whatever, foobar) is whatever, so a > correct third line above would be > > "if (!!(cond))" > > which is of course not at all the same as > > "if (!!(cond) == 0)" aka "if (!(cond))"
I get it, you are right. Thank you for the explanation.
Denis
| |