This patch adds coccinelle script for detecting !likely and !unlikely
usage. It's better to use unlikely instead of !likely and vice versa.
Signed-off-by: Denis Efremov <[email protected]>
---
scripts/coccinelle/misc/unlikely.cocci | 70 ++++++++++++++++++++++++++
1 file changed, 70 insertions(+)
create mode 100644 scripts/coccinelle/misc/unlikely.cocci
diff --git a/scripts/coccinelle/misc/unlikely.cocci b/scripts/coccinelle/misc/unlikely.cocci
new file mode 100644
index 000000000000..88278295d76a
--- /dev/null
+++ b/scripts/coccinelle/misc/unlikely.cocci
@@ -0,0 +1,70 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/// Use unlikely instead of !likely and likely instead of !unlikely.
+///
+// Confidence: High
+// Copyright: (C) 2019 Denis Efremov, ISPRAS.
+// Options: --include-headers
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+//----------------------------------------------------------
+// For context mode
+//----------------------------------------------------------
+
+@depends on context disable unlikely@
+expression E;
+@@
+
+(
+* !likely(E)
+|
+* !unlikely(E)
+)
+
+//----------------------------------------------------------
+// For patch mode
+//----------------------------------------------------------
+
+@depends on patch disable unlikely@
+expression E;
+@@
+
+(
+-!likely(E)
++unlikely(E)
+|
+-!unlikely(E)
++likely(E)
+)
+
+//----------------------------------------------------------
+// For org and report mode
+//----------------------------------------------------------
+
+@r depends on (org || report) disable unlikely@
+expression E;
+position p;
+@@
+
+(
+ !likely@p(E)
+|
+ !unlikely@p(E)
+)
+
+@script:python depends on org@
+p << r.p;
+@@
+
+coccilib.org.print_todo(p[0], "WARNING use unlikely instead of !likely")
+
+@script:python depends on report@
+p << r.p;
+@@
+
+msg="WARNING: Use unlikely instead of !likely"
+coccilib.report.print_report(p[0], msg)
+
--
2.21.0
> +(
> +* !likely(E)
> +|
> +* !unlikely(E)
> +)
Can the following code variant be nicer?
+*! \( likely \| unlikely \) (E)
> +(
> +-!likely(E)
> ++unlikely(E)
> +|
> +-!unlikely(E)
> ++likely(E)
> +)
I would find the following SmPL change specification more succinct.
+(
+-!likely
++unlikely
+|
+-!unlikely
++likely
+)(E)
> +coccilib.org.print_todo(p[0], "WARNING use unlikely instead of !likely")
…
> +msg="WARNING: Use unlikely instead of !likely"
> +coccilib.report.print_report(p[0], msg)
1. I find such a message construction nicer without the extra variable “msg”.
2. I recommend to make the provided information unique.
* How do you think about to split the SmPL disjunction in the rule “r”
for this purpose?
* Should the transformation become clearer?
Regards,
Markus
> +(
> +* !likely(E)
> +|
> +* !unlikely(E)
> +)
Can the following code variant be nicer?
+*! \( likely \| unlikely \) (E)
> +(
> +-!likely(E)
> ++unlikely(E)
> +|
> +-!unlikely(E)
> ++likely(E)
> +)
I would find the following SmPL change specification more succinct.
+(
+-!likely
++unlikely
+|
+-!unlikely
++likely
+)(E)
> +coccilib.org.print_todo(p[0], "WARNING use unlikely instead of !likely")
…
> +msg="WARNING: Use unlikely instead of !likely"
> +coccilib.report.print_report(p[0], msg)
1. I find such a message construction nicer without the extra variable “msg”.
2. I recommend to make the provided information unique.
* How do you think about to split the SmPL disjunction in the rule “r”
for this purpose?
* Should the transformation become clearer?
Regards,
Markus
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.
btw: there are relatively few uses like this in the kernel.
$ git grep -P '!\s*(?:un)?likely\s*\(' | wc -l
40
afaict: It may save 2 bytes of x86/64 object code.
For instance:
$ diff -urN kernel/tsacct.lst.old kernel/tsacct.lst.new|less
--- kernel/tsacct.lst.old 2019-08-25 09:21:39.936570183 -0700
+++ kernel/tsacct.lst.new 2019-08-25 09:22:20.774324886 -0700
@@ -24,158 +24,153 @@
15: 48 89 fb mov %rdi,%rbx
u64 time, delta;
- if (!likely(tsk->mm))
+ if (unlikely(tsk->mm))
18: 4c 8d ab 28 02 00 00 lea 0x228(%rbx),%r13
1f: e8 00 00 00 00 callq 24 <__acct_update_integrals+0x24>
20: R_X86_64_PLT32 __sanitizer_cov_trace_pc-0x4
24: 4c 89 ef mov %r13,%rdi
27: e8 00 00 00 00 callq 2c <__acct_update_integrals+0x2c>
28: R_X86_64_PLT32 __asan_load8_noabort-0x4
- 2c: 4c 8b bb 28 02 00 00 mov 0x228(%rbx),%r15
- 33: 4d 85 ff test %r15,%r15
- 36: 74 34 je 6c <__acct_update_integrals+0x6c>
+ 2c: 48 83 bb 28 02 00 00 cmpq $0x0,0x228(%rbx)
+ 33: 00
+ 34: 75 34 jne 6a <__acct_update_integrals+0x6a>
return;
And here's a possible equivalent checkpatch test.
---
scripts/checkpatch.pl | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 287fe73688f0..364603ad1a47 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -6529,6 +6529,24 @@ sub process {
"Using $1 should generally have parentheses around the comparison\n" . $herecurr);
}
+# !(likely|unlikely)(condition) use should be (unlikely|likely)(condition)
+ if ($perl_version_ok &&
+ $line =~ /(\!\s*((?:un)?likely))\s*$balanced_parens/) {
+ my $match = $1;
+ my $type = $2;
+ my $reverse;
+ if ($type eq "likely") {
+ $reverse = "unlikely";
+ } else {
+ $reverse = "likely";
+ }
+ if (WARN("LIKELY_MISUSE",
+ "Prefer $reverse over $match\n" . $herecurr) &&
+ $fix) {
+ $fixed[$fixlinenr] =~ s/\Q$match\E\s*\(/$reverse(/;
+ }
+ }
+
# whine mightly about in_atomic
if ($line =~ /\bin_atomic\s*\(/) {
if ($realfile =~ m@^drivers/@) {
> On 26 Aug 2019, at 02:59, Denis Efremov <[email protected]> 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?
Julia
> and the initial branch-prediction intent
> could be "falsified". I would say that either you need to move the
> negation under the bracket "!unlikely(cond) -> unlikely(!cond)" or
> you need to use likely instead "!unlikely(cond) -> likely(cond)".
> However, I'm not a compiler expert to state that this is a general
> rule. But, we've got 2 special macro for branch predicting, not one.
> There is also ftrace in-between. I will try to do some simple
> benchmarking.
>
>> btw: there are relatively few uses like this in the kernel.
>>
>> $ git grep -P '!\s*(?:un)?likely\s*\(' | wc -l
>> 40
>>
>> afaict: It may save 2 bytes of x86/64 object code.
>>
>> For instance:
>>
>> $ diff -urN kernel/tsacct.lst.old kernel/tsacct.lst.new|less
>> --- kernel/tsacct.lst.old 2019-08-25 09:21:39.936570183 -0700
>> +++ kernel/tsacct.lst.new 2019-08-25 09:22:20.774324886 -0700
>> @@ -24,158 +24,153 @@
>> 15: 48 89 fb mov %rdi,%rbx
>> u64 time, delta;
>>
>> - if (!likely(tsk->mm))
>> + if (unlikely(tsk->mm))
>> 18: 4c 8d ab 28 02 00 00 lea 0x228(%rbx),%r13
>> 1f: e8 00 00 00 00 callq 24 <__acct_update_integrals+0x24>
>> 20: R_X86_64_PLT32 __sanitizer_cov_trace_pc-0x4
>> 24: 4c 89 ef mov %r13,%rdi
>> 27: e8 00 00 00 00 callq 2c <__acct_update_integrals+0x2c>
>> 28: R_X86_64_PLT32 __asan_load8_noabort-0x4
>> - 2c: 4c 8b bb 28 02 00 00 mov 0x228(%rbx),%r15
>> - 33: 4d 85 ff test %r15,%r15
>> - 36: 74 34 je 6c <__acct_update_integrals+0x6c>
>> + 2c: 48 83 bb 28 02 00 00 cmpq $0x0,0x228(%rbx)
>> + 33: 00
>> + 34: 75 34 jne 6a <__acct_update_integrals+0x6a>
>> return;
>
> I think it's incorrect to say so in general. For example, on x86/64:
>
> $ make mrproper
> $ make allyesconfig
> $ make && mv vmlinux vmlinux-000
> $ make coccicheck MODE=patch COCCI=scripts/coccinelle/misc/unlikely.cocci | patch -p1
> $ make
> $ ./scripts/bloat-o-meter ./vmlinux-000 ./vmlinux
> add/remove: 0/0 grow/shrink: 3/4 up/down: 41/-35 (6)
> Function old new delta
> dpaa2_io_service_rearm 357 382 +25
> intel_pmu_hw_config 1277 1285 +8
> get_sigframe.isra.constprop 1657 1665 +8
> csum_partial_copy_from_user 605 603 -2
> wait_consider_task 3807 3797 -10
> __acct_update_integrals 384 373 -11
> pipe_to_sendpage 459 447 -12
> Total: Before=312759461, After=312759467, chg +0.00%
>
> It definitely influence the way the compiler optimizes the code.
>
>>
>> And here's a possible equivalent checkpatch test.
>> ---
>> scripts/checkpatch.pl | 18 ++++++++++++++++++
>> 1 file changed, 18 insertions(+)
>>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 287fe73688f0..364603ad1a47 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -6529,6 +6529,24 @@ sub process {
>> "Using $1 should generally have parentheses around the comparison\n" . $herecurr);
>> }
>>
>> +# !(likely|unlikely)(condition) use should be (unlikely|likely)(condition)
>> + if ($perl_version_ok &&
>> + $line =~ /(\!\s*((?:un)?likely))\s*$balanced_parens/) {
>> + my $match = $1;
>> + my $type = $2;
>> + my $reverse;
>> + if ($type eq "likely") {
>> + $reverse = "unlikely";
>> + } else {
>> + $reverse = "likely";
>> + }
>> + if (WARN("LIKELY_MISUSE",
>> + "Prefer $reverse over $match\n" . $herecurr) &&
>> + $fix) {
>> + $fixed[$fixlinenr] =~ s/\Q$match\E\s*\(/$reverse(/;
>> + }
>> + }
>> +
>> # whine mightly about in_atomic
>> if ($line =~ /\bin_atomic\s*\(/) {
>> if ($realfile =~ m@^drivers/@) {
>>
>>
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 and the initial branch-prediction intent
could be "falsified". I would say that either you need to move the
negation under the bracket "!unlikely(cond) -> unlikely(!cond)" or
you need to use likely instead "!unlikely(cond) -> likely(cond)".
However, I'm not a compiler expert to state that this is a general
rule. But, we've got 2 special macro for branch predicting, not one.
There is also ftrace in-between. I will try to do some simple
benchmarking.
> btw: there are relatively few uses like this in the kernel.
>
> $ git grep -P '!\s*(?:un)?likely\s*\(' | wc -l
> 40
>
> afaict: It may save 2 bytes of x86/64 object code.
>
> For instance:
>
> $ diff -urN kernel/tsacct.lst.old kernel/tsacct.lst.new|less
> --- kernel/tsacct.lst.old 2019-08-25 09:21:39.936570183 -0700
> +++ kernel/tsacct.lst.new 2019-08-25 09:22:20.774324886 -0700
> @@ -24,158 +24,153 @@
> 15: 48 89 fb mov %rdi,%rbx
> u64 time, delta;
>
> - if (!likely(tsk->mm))
> + if (unlikely(tsk->mm))
> 18: 4c 8d ab 28 02 00 00 lea 0x228(%rbx),%r13
> 1f: e8 00 00 00 00 callq 24 <__acct_update_integrals+0x24>
> 20: R_X86_64_PLT32 __sanitizer_cov_trace_pc-0x4
> 24: 4c 89 ef mov %r13,%rdi
> 27: e8 00 00 00 00 callq 2c <__acct_update_integrals+0x2c>
> 28: R_X86_64_PLT32 __asan_load8_noabort-0x4
> - 2c: 4c 8b bb 28 02 00 00 mov 0x228(%rbx),%r15
> - 33: 4d 85 ff test %r15,%r15
> - 36: 74 34 je 6c <__acct_update_integrals+0x6c>
> + 2c: 48 83 bb 28 02 00 00 cmpq $0x0,0x228(%rbx)
> + 33: 00
> + 34: 75 34 jne 6a <__acct_update_integrals+0x6a>
> return;
I think it's incorrect to say so in general. For example, on x86/64:
$ make mrproper
$ make allyesconfig
$ make && mv vmlinux vmlinux-000
$ make coccicheck MODE=patch COCCI=scripts/coccinelle/misc/unlikely.cocci | patch -p1
$ make
$ ./scripts/bloat-o-meter ./vmlinux-000 ./vmlinux
add/remove: 0/0 grow/shrink: 3/4 up/down: 41/-35 (6)
Function old new delta
dpaa2_io_service_rearm 357 382 +25
intel_pmu_hw_config 1277 1285 +8
get_sigframe.isra.constprop 1657 1665 +8
csum_partial_copy_from_user 605 603 -2
wait_consider_task 3807 3797 -10
__acct_update_integrals 384 373 -11
pipe_to_sendpage 459 447 -12
Total: Before=312759461, After=312759467, chg +0.00%
It definitely influence the way the compiler optimizes the code.
>
> And here's a possible equivalent checkpatch test.
> ---
> scripts/checkpatch.pl | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 287fe73688f0..364603ad1a47 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6529,6 +6529,24 @@ sub process {
> "Using $1 should generally have parentheses around the comparison\n" . $herecurr);
> }
>
> +# !(likely|unlikely)(condition) use should be (unlikely|likely)(condition)
> + if ($perl_version_ok &&
> + $line =~ /(\!\s*((?:un)?likely))\s*$balanced_parens/) {
> + my $match = $1;
> + my $type = $2;
> + my $reverse;
> + if ($type eq "likely") {
> + $reverse = "unlikely";
> + } else {
> + $reverse = "likely";
> + }
> + if (WARN("LIKELY_MISUSE",
> + "Prefer $reverse over $match\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ s/\Q$match\E\s*\(/$reverse(/;
> + }
> + }
> +
> # whine mightly about in_atomic
> if ($line =~ /\bin_atomic\s*\(/) {
> if ($realfile =~ m@^drivers/@) {
>
>
On 25.08.2019 18:30, Markus Elfring wrote:
>> +(
>> +* !likely(E)
>> +|
>> +* !unlikely(E)
>> +)
>
> Can the following code variant be nicer?
>
> +*! \( likely \| unlikely \) (E)
>
>
>> +(
>> +-!likely(E)
>> ++unlikely(E)
>> +|
>> +-!unlikely(E)
>> ++likely(E)
>> +)
>
> I would find the following SmPL change specification more succinct.
>
> +(
> +-!likely
> ++unlikely
> +|
> +-!unlikely
> ++likely
> +)(E)
>
>
>> +coccilib.org.print_todo(p[0], "WARNING use unlikely instead of !likely")
> …
>> +msg="WARNING: Use unlikely instead of !likely"
>> +coccilib.report.print_report(p[0], msg)
>
> 1. I find such a message construction nicer without the extra variable “msg”.
>
> 2. I recommend to make the provided information unique.
> * How do you think about to split the SmPL disjunction in the rule “r”
> for this purpose?
>
> * Should the transformation become clearer?
Thank you for the review, I will prepare v2.
>
> Regards,
> Markus
>
> I think it's incorrect to say so in general. For example, on x86/64:
>
> $ make mrproper
> $ make allyesconfig
> $ make && mv vmlinux vmlinux-000
> $ make coccicheck MODE=patch COCCI=scripts/coccinelle/misc/unlikely.cocci | patch -p1
> $ make
> $ ./scripts/bloat-o-meter ./vmlinux-000 ./vmlinux
> add/remove: 0/0 grow/shrink: 3/4 up/down: 41/-35 (6)
> Function old new delta
> dpaa2_io_service_rearm 357 382 +25
> intel_pmu_hw_config 1277 1285 +8
> get_sigframe.isra.constprop 1657 1665 +8
> csum_partial_copy_from_user 605 603 -2
> wait_consider_task 3807 3797 -10
> __acct_update_integrals 384 373 -11
> pipe_to_sendpage 459 447 -12
> Total: Before=312759461, After=312759467, chg +0.00%
>
> It definitely influence the way the compiler optimizes the code.
Small addition:
Results with allyesconfig and KCOV, KASAN, KUBSAN, FTRACE, TRACE_BRANCH_PROFILING,
PROFILE_ALL_BRANCHES disabled:
./scripts/bloat-o-meter ./vmlinux-000 ./vmlinux
add/remove: 0/0 grow/shrink: 2/3 up/down: 22/-22 (0)
Function old new delta
i40e_xmit_xdp_ring 457 477 +20
__acct_update_integrals 127 129 +2
csum_partial_copy_from_user 208 207 -1
dpaa2_io_service_rearm 180 177 -3
wait_consider_task 1338 1320 -18
For defconfig:
./scripts/bloat-o-meter ./vmlinux-000 ./vmlinux
add/remove: 0/0 grow/shrink: 3/1 up/down: 16/-5 (11)
Function old new delta
do_signal 1513 1521 +8
wait_consider_task 2151 2157 +6
__acct_update_integrals 127 129 +2
csum_partial_copy_from_user 223 218 -5
Denis
On 25/08/2019 21.19, Julia Lawall wrote:
>
>
>> On 26 Aug 2019, at 02:59, Denis Efremov <[email protected]> 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).
Rasmus
On Wed, 2019-08-28 at 13:33 +0200, Rasmus Villemoes wrote:
> On 25/08/2019 21.19, Julia Lawall wrote:
> > > On 26 Aug 2019, at 02:59, Denis Efremov <[email protected]> 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).
likely and unlikely use __builtin_expect
https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005fexpect
https://stackoverflow.com/questions/7346929/what-is-the-advantage-of-gccs-builtin-expect-in-if-else-statements
It's probable that of the more than 20K uses of
likely and unlikely in the kernel, most have no
real performance effect.
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 <[email protected]> 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.
if (!likely(cond))
if (!__builtin_expect(!!(cond), 1))
if (!((!!(cond)) == 1))
if ((!!(cond)) != 1) and since !! could result in 0 or 1
if ((!!(cond)) == 0)
if (unlikely(cond))
if (__builtin_expect(!!(cond), 0))
if ((!!(cond)) == 0))
Thanks,
Denis
On Wed, 28 Aug 2019, Rasmus Villemoes wrote:
> On 25/08/2019 21.19, Julia Lawall wrote:
> >
> >
> >> On 26 Aug 2019, at 02:59, Denis Efremov <[email protected]> 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).
Thanks. Making the change seems like a good idea.
julia
>
> As a human I am confused. Is !likely(x) equivalent to x or !x?
>
> Julia
>
As far as I could understand it:
# define likely(x) __builtin_expect(!!(x), 1)
!likely(x)
!__builtin_expect(!!(x), 1)
!((!!(x)) == 1)
(!!(x)) != 1, since !! could result in 0 or 1
(!!(x)) == 0
!(!!(x))
!!!(x)
!(x)
Thanks,
Denis
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 <[email protected]> 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))"
Rasmus
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 <[email protected]> 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
On 8/28/19 3:41 PM, Denis Efremov wrote:
>
>>
>> As a human I am confused. Is !likely(x) equivalent to x or !x?
>>
>> Julia
>>
>
> As far as I could understand it:
>
> # define likely(x) __builtin_expect(!!(x), 1)
> !likely(x)
> !__builtin_expect(!!(x), 1)
> !((!!(x)) == 1)
> (!!(x)) != 1, since !! could result in 0 or 1
> (!!(x)) == 0
> !(!!(x))
> !!!(x)
> !(x)
>
Thanks Rasmus for the explanation, this should be:
!likely(x)
!__builtin_expect(!!(x), 1)
!(!!(x))
!!!(x)
!(x)
Denis
This patch adds coccinelle script for detecting !likely and
!unlikely usage. These notations are confusing. It's better
to replace !likely(x) with unlikely(!x) and !unlikely(x) with
likely(!x) for readability.
The rule transforms !likely(x) to unlikely(!x) based on this logic:
!likely(x) iff
!__builtin_expect(!!(x), 1) iff
__builtin_expect(!!!(x), 0) iff
unlikely(!x)
For !unlikely(x) to likely(!x):
!unlikely(x) iff
!__builtin_expect(!!(x), 0) iff
__builtin_expect(!!!(x), 1) iff
likely(!x)
Signed-off-by: Denis Efremov <[email protected]>
Cc: Julia Lawall <[email protected]>
Cc: Gilles Muller <[email protected]>
Cc: Nicolas Palix <[email protected]>
Cc: Michal Marek <[email protected]>
Cc: Markus Elfring <[email protected]>
Cc: Joe Perches <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
---
scripts/coccinelle/misc/neg_unlikely.cocci | 89 ++++++++++++++++++++++
1 file changed, 89 insertions(+)
create mode 100644 scripts/coccinelle/misc/neg_unlikely.cocci
diff --git a/scripts/coccinelle/misc/neg_unlikely.cocci b/scripts/coccinelle/misc/neg_unlikely.cocci
new file mode 100644
index 000000000000..9aac0a7ff042
--- /dev/null
+++ b/scripts/coccinelle/misc/neg_unlikely.cocci
@@ -0,0 +1,89 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/// Use unlikely(!x) instead of !likely(x) and vice versa.
+/// Notations !unlikely(x) and !likely(x) are confusing.
+//
+// Confidence: High
+// Copyright: (C) 2019 Denis Efremov, ISPRAS.
+// Options: --no-includes --include-headers
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+//----------------------------------------------------------
+// For context mode
+//----------------------------------------------------------
+
+@depends on context disable unlikely@
+expression E;
+@@
+
+*! \( likely \| unlikely \) (E)
+
+//----------------------------------------------------------
+// For patch mode
+//----------------------------------------------------------
+
+@depends on patch disable unlikely@
+expression E;
+@@
+
+(
+-!likely(!E)
++unlikely(E)
+|
+-!likely(E)
++unlikely(!E)
+|
+-!unlikely(!E)
++likely(E)
+|
+-!unlikely(E)
++likely(!E)
+)
+
+//----------------------------------------------------------
+// For org and report mode
+//----------------------------------------------------------
+
+@r1 depends on (org || report) disable unlikely@
+expression E;
+position p;
+@@
+
+!likely@p(E)
+
+@r2 depends on (org || report) disable unlikely@
+expression E;
+position p;
+@@
+
+!unlikely@p(E)
+
+@script:python depends on org && r1@
+p << r1.p;
+@@
+
+coccilib.org.print_todo(p[0], "unlikely(!x) is more readable than !likely(x)")
+
+@script:python depends on org && r2@
+p << r2.p;
+@@
+
+coccilib.org.print_todo(p[0], "likely(!x) is more readable than !unlikely(x)")
+
+@script:python depends on report && r1@
+p << r1.p;
+@@
+
+coccilib.report.print_report(p[0],
+ "unlikely(!x) is more readable than !likely(x)")
+
+@script:python depends on report && r2@
+p << r2.p;
+@@
+
+coccilib.report.print_report(p[0],
+ "likely(!x) is more readable than !unlikely(x)")
+
--
2.21.0
On 8/29/19 8:10 PM, Denis Efremov wrote:
> This patch adds coccinelle script for detecting !likely and
> !unlikely usage. These notations are confusing. It's better
> to replace !likely(x) with unlikely(!x) and !unlikely(x) with
> likely(!x) for readability.
I'm not sure that this rule deserves the acceptance.
Just to want to be sure that "!unlikely(x)" and "!likely(x)"
are hard-readable is not only my perception and that they
become more clear in form "likely(!x)" and "unlikely(!x)" too.
Thanks,
Denis
> +@script:python depends on report && r1@
> +p << r1.p;
> +@@
> +
> +coccilib.report.print_report(p[0],
> + "unlikely(!x) is more readable than !likely(x)")
Can alignment matter for the string literal together with
the first method parameter?
Regards,
Markus
On Thu, 29 Aug 2019, Denis Efremov wrote:
> On 8/29/19 8:10 PM, Denis Efremov wrote:
> > This patch adds coccinelle script for detecting !likely and
> > !unlikely usage. These notations are confusing. It's better
> > to replace !likely(x) with unlikely(!x) and !unlikely(x) with
> > likely(!x) for readability.
>
> I'm not sure that this rule deserves the acceptance.
> Just to want to be sure that "!unlikely(x)" and "!likely(x)"
> are hard-readable is not only my perception and that they
> become more clear in form "likely(!x)" and "unlikely(!x)" too.
Is likely/unlikely even useful for anything once it is a subexpression?
julia
On 30.08.2019 03:42, Julia Lawall wrote:
>
>
> On Thu, 29 Aug 2019, Denis Efremov wrote:
>
>> On 8/29/19 8:10 PM, Denis Efremov wrote:
>>> This patch adds coccinelle script for detecting !likely and
>>> !unlikely usage. These notations are confusing. It's better
>>> to replace !likely(x) with unlikely(!x) and !unlikely(x) with
>>> likely(!x) for readability.
>>
>> I'm not sure that this rule deserves the acceptance.
>> Just to want to be sure that "!unlikely(x)" and "!likely(x)"
>> are hard-readable is not only my perception and that they
>> become more clear in form "likely(!x)" and "unlikely(!x)" too.
>
> Is likely/unlikely even useful for anything once it is a subexpression?
>> julia
>
Well, as far as I understand it,
It's correct since it sets the probability of likely/unlikely subexpression
is true to 90% (see https://gcc.gnu.org/onlinedocs/gcc-9.2.0/gcc/Other-Builtins.html).
The probability of a whole expression is then computed by GCC
in this case. It's kind of assigning individual weights to conjuncts/disjuncts.
I think that it can be useful when you are not sure about the probability
of the whole expression but you know something about subexpressions it consists, e.g.,
likely(E1) && E2. However, I think that "!unlikely(x)" is fully equivalent in this sense
to "likely(!x)". I tested it once again for allyesconfig with branch profiling
disabled and bloat-o-meter shows no diff in binary size.
Denis
> +/// Notations !unlikely(x) and !likely(x) are confusing.
I am curious if more software developers will share their views around
these likeliness annotations.
* How much does the scope matter for expressions?
* Are different coding style preferences involved?
> +//----------------------------------------------------------
> +// For context mode
> +//----------------------------------------------------------
> +
> +@depends on context disable unlikely@
I wonder about the need for such a comment when the specification
of SmPL rule dependencies should be sufficient.
> +@depends on patch disable unlikely@
> +expression E;
> +@@
> +
> +(
> +-!likely(!E)
> ++unlikely(E)
> +|
> +-!likely(E)
> ++unlikely(!E)
> +|
> +-!unlikely(!E)
> ++likely(E)
> +|
> +-!unlikely(E)
> ++likely(!E)
> +)
Will another variant for the change specification with the semantic
patch language influence corresponding readability concerns?
+@replacement depends on patch disable unlikely@
+expression x;
+@@
+-!
+(
+(
+-unlikely
++likely
+|
+-likely
++unlikely
+)
+ (
+- !
+ x
+ )
+|
+(
+-unlikely
++likely
+|
+-likely
++unlikely
+)
+ (
++ !
+ x
+ )
+)
Can the use of nested SmPL disjunctions help here together with
an other SmPL code formatting?
Regards,
Markus
On 30/08/2019 08.56, Denis Efremov wrote:
>
>
> On 30.08.2019 03:42, Julia Lawall wrote:
>>
>>
>> On Thu, 29 Aug 2019, Denis Efremov wrote:
>>
>>> On 8/29/19 8:10 PM, Denis Efremov wrote:
>>>> This patch adds coccinelle script for detecting !likely and
>>>> !unlikely usage. These notations are confusing. It's better
>>>> to replace !likely(x) with unlikely(!x) and !unlikely(x) with
>>>> likely(!x) for readability.
>>>
>>> I'm not sure that this rule deserves the acceptance.
>>> Just to want to be sure that "!unlikely(x)" and "!likely(x)"
>>> are hard-readable is not only my perception and that they
>>> become more clear in form "likely(!x)" and "unlikely(!x)" too.
>>
>> Is likely/unlikely even useful for anything once it is a subexpression?
>>> julia
>>
>
> Well, as far as I understand it,
Yes, and it could in fact make sense in cases like
if (likely(foo->bar) && unlikely(foo->bar->baz)) {
do_stuff_with(foo->bar->baz);
do_more_stuff();
}
which the compiler could then compile as (of course actual code
generation is always much more complicated due to things in the
surrounding code)
load foo->bar;
test bar;
if 0 goto skip;
load bar->baz;
test baz;
if !0 goto far_away;
skip:
....
so in the normal flow, neither branch is taken. If instead one wrote
unlikely(foo->bar && foo->bar->baz), gcc doesn't really know why we
expect the whole conjuntion to turn out false, so it could compile this
as a jump when foo->bar turns out non-zero - i.e., in the normal case,
we'd end up jumping.
But as far as !(un)likely(), I agree that it's easier to read as a human
if the ! operator is moved inside (and the "un" prefix stripped/added).
Whether it deserves a cocci script I don't know.
Rasmus
Hi!
> > 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.
>
> btw: there are relatively few uses like this in the kernel.
>
> $ git grep -P '!\s*(?:un)?likely\s*\(' | wc -l
> 40
>
> afaict: It may save 2 bytes of x86/64 object code.
>
> For instance:
>
> $ diff -urN kernel/tsacct.lst.old kernel/tsacct.lst.new|less
> --- kernel/tsacct.lst.old 2019-08-25 09:21:39.936570183 -0700
> +++ kernel/tsacct.lst.new 2019-08-25 09:22:20.774324886 -0700
> @@ -24,158 +24,153 @@
> 15: 48 89 fb mov %rdi,%rbx
> u64 time, delta;
>
> - if (!likely(tsk->mm))
> + if (unlikely(tsk->mm))
Are you sure this is equivalent?
Pavel
> 18: 4c 8d ab 28 02 00 00 lea 0x228(%rbx),%r13
> 1f: e8 00 00 00 00 callq 24 <__acct_update_integrals+0x24>
> 20: R_X86_64_PLT32 __sanitizer_cov_trace_pc-0x4
> 24: 4c 89 ef mov %r13,%rdi
> 27: e8 00 00 00 00 callq 2c <__acct_update_integrals+0x2c>
> 28: R_X86_64_PLT32 __asan_load8_noabort-0x4
> - 2c: 4c 8b bb 28 02 00 00 mov 0x228(%rbx),%r15
> - 33: 4d 85 ff test %r15,%r15
> - 36: 74 34 je 6c <__acct_update_integrals+0x6c>
> + 2c: 48 83 bb 28 02 00 00 cmpq $0x0,0x228(%rbx)
> + 33: 00
> + 34: 75 34 jne 6a <__acct_update_integrals+0x6a>
> return;
>
> And here's a possible equivalent checkpatch test.
> ---
> scripts/checkpatch.pl | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 287fe73688f0..364603ad1a47 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -6529,6 +6529,24 @@ sub process {
> "Using $1 should generally have parentheses around the comparison\n" . $herecurr);
> }
>
> +# !(likely|unlikely)(condition) use should be (unlikely|likely)(condition)
> + if ($perl_version_ok &&
> + $line =~ /(\!\s*((?:un)?likely))\s*$balanced_parens/) {
> + my $match = $1;
> + my $type = $2;
> + my $reverse;
> + if ($type eq "likely") {
> + $reverse = "unlikely";
> + } else {
> + $reverse = "likely";
> + }
> + if (WARN("LIKELY_MISUSE",
> + "Prefer $reverse over $match\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ s/\Q$match\E\s*\(/$reverse(/;
> + }
> + }
> +
> # whine mightly about in_atomic
> if ($line =~ /\bin_atomic\s*\(/) {
> if ($realfile =~ m@^drivers/@) {
>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 01.09.2019 20:24, Pavel Machek wrote:
> Hi!
>
>>> 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.
>>
>> btw: there are relatively few uses like this in the kernel.
>>
>> $ git grep -P '!\s*(?:un)?likely\s*\(' | wc -l
>> 40
>>
>> afaict: It may save 2 bytes of x86/64 object code.
>>
>> For instance:
>>
>> $ diff -urN kernel/tsacct.lst.old kernel/tsacct.lst.new|less
>> --- kernel/tsacct.lst.old 2019-08-25 09:21:39.936570183 -0700
>> +++ kernel/tsacct.lst.new 2019-08-25 09:22:20.774324886 -0700
>> @@ -24,158 +24,153 @@
>> 15: 48 89 fb mov %rdi,%rbx
>> u64 time, delta;
>>
>> - if (!likely(tsk->mm))
>> + if (unlikely(tsk->mm))
>
> Are you sure this is equivalent?
>
> Pavel
Hi,
No, it's not correct. Thanks Rasmus for clarifying the things here.
This was my mistake. As a human, I failed to parse "likely" and "!"
and made too hasty conclusions :)
The correct transformation is in v2. This will be
!likely(tsk->mm) -> unlikely(!tsk->mm)
Thanks,
Denis
On Thu, 29 Aug 2019, Denis Efremov wrote:
> This patch adds coccinelle script for detecting !likely and
> !unlikely usage. These notations are confusing. It's better
> to replace !likely(x) with unlikely(!x) and !unlikely(x) with
> likely(!x) for readability.
>
> The rule transforms !likely(x) to unlikely(!x) based on this logic:
> !likely(x) iff
> !__builtin_expect(!!(x), 1) iff
> __builtin_expect(!!!(x), 0) iff
> unlikely(!x)
>
> For !unlikely(x) to likely(!x):
> !unlikely(x) iff
> !__builtin_expect(!!(x), 0) iff
> __builtin_expect(!!!(x), 1) iff
> likely(!x)
>
> Signed-off-by: Denis Efremov <[email protected]>
> Cc: Julia Lawall <[email protected]>
> Cc: Gilles Muller <[email protected]>
> Cc: Nicolas Palix <[email protected]>
> Cc: Michal Marek <[email protected]>
> Cc: Markus Elfring <[email protected]>
> Cc: Joe Perches <[email protected]>
> Cc: Rasmus Villemoes <[email protected]>
Acked-by: Julia Lawall <[email protected]>
A small improvement though would be to improve the explicit dependency of
the last four python rules on r1 and r2. Those rules won't execute unless
the inherited metavariable has a value, which makes the same dependency.
julia
> ---
> scripts/coccinelle/misc/neg_unlikely.cocci | 89 ++++++++++++++++++++++
> 1 file changed, 89 insertions(+)
> create mode 100644 scripts/coccinelle/misc/neg_unlikely.cocci
>
> diff --git a/scripts/coccinelle/misc/neg_unlikely.cocci b/scripts/coccinelle/misc/neg_unlikely.cocci
> new file mode 100644
> index 000000000000..9aac0a7ff042
> --- /dev/null
> +++ b/scripts/coccinelle/misc/neg_unlikely.cocci
> @@ -0,0 +1,89 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/// Use unlikely(!x) instead of !likely(x) and vice versa.
> +/// Notations !unlikely(x) and !likely(x) are confusing.
> +//
> +// Confidence: High
> +// Copyright: (C) 2019 Denis Efremov, ISPRAS.
> +// Options: --no-includes --include-headers
> +
> +virtual patch
> +virtual context
> +virtual org
> +virtual report
> +
> +//----------------------------------------------------------
> +// For context mode
> +//----------------------------------------------------------
> +
> +@depends on context disable unlikely@
> +expression E;
> +@@
> +
> +*! \( likely \| unlikely \) (E)
> +
> +//----------------------------------------------------------
> +// For patch mode
> +//----------------------------------------------------------
> +
> +@depends on patch disable unlikely@
> +expression E;
> +@@
> +
> +(
> +-!likely(!E)
> ++unlikely(E)
> +|
> +-!likely(E)
> ++unlikely(!E)
> +|
> +-!unlikely(!E)
> ++likely(E)
> +|
> +-!unlikely(E)
> ++likely(!E)
> +)
> +
> +//----------------------------------------------------------
> +// For org and report mode
> +//----------------------------------------------------------
> +
> +@r1 depends on (org || report) disable unlikely@
> +expression E;
> +position p;
> +@@
> +
> +!likely@p(E)
> +
> +@r2 depends on (org || report) disable unlikely@
> +expression E;
> +position p;
> +@@
> +
> +!unlikely@p(E)
> +
> +@script:python depends on org && r1@
> +p << r1.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "unlikely(!x) is more readable than !likely(x)")
> +
> +@script:python depends on org && r2@
> +p << r2.p;
> +@@
> +
> +coccilib.org.print_todo(p[0], "likely(!x) is more readable than !unlikely(x)")
> +
> +@script:python depends on report && r1@
> +p << r1.p;
> +@@
> +
> +coccilib.report.print_report(p[0],
> + "unlikely(!x) is more readable than !likely(x)")
> +
> +@script:python depends on report && r2@
> +p << r2.p;
> +@@
> +
> +coccilib.report.print_report(p[0],
> + "likely(!x) is more readable than !unlikely(x)")
> +
> --
> 2.21.0
>
>
Hi,
On 06.09.2019 23:19, Julia Lawall wrote:
>
>
> On Thu, 29 Aug 2019, Denis Efremov wrote:
>
>> This patch adds coccinelle script for detecting !likely and
>> !unlikely usage. These notations are confusing. It's better
>> to replace !likely(x) with unlikely(!x) and !unlikely(x) with
>> likely(!x) for readability.
>>
>> The rule transforms !likely(x) to unlikely(!x) based on this logic:
>> !likely(x) iff
>> !__builtin_expect(!!(x), 1) iff
>> __builtin_expect(!!!(x), 0) iff
>> unlikely(!x)
>>
>> For !unlikely(x) to likely(!x):
>> !unlikely(x) iff
>> !__builtin_expect(!!(x), 0) iff
>> __builtin_expect(!!!(x), 1) iff
>> likely(!x)
>>
>> Signed-off-by: Denis Efremov <[email protected]>
>> Cc: Julia Lawall <[email protected]>
>> Cc: Gilles Muller <[email protected]>
>> Cc: Nicolas Palix <[email protected]>
>> Cc: Michal Marek <[email protected]>
>> Cc: Markus Elfring <[email protected]>
>> Cc: Joe Perches <[email protected]>
>> Cc: Rasmus Villemoes <[email protected]>
>
> Acked-by: Julia Lawall <[email protected]>
>
> A small improvement though would be to improve the explicit dependency of
> the last four python rules on r1 and r2. Those rules won't execute unless
> the inherited metavariable has a value, which makes the same dependency.
>
> julia
I think I will resend this patch as a part of patchset with all warnings fixed
in a couple of days. Hope this will help to create a discussion point with other
developers about readability of "!likely" and "!unlikely".
Thanks,
Denis
> I think I will resend this patch as a part of patchset with all warnings fixed
> in a couple of days. Hope this will help to create a discussion point with other
> developers about readability of "!likely" and "!unlikely".
Will the influence of code variations become more interesting also for
the discussed SmPL script?
Regards,
Markus