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 <efremov@linux.com> --- 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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> +( > +* !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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> +( > +* !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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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/@) { _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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/@) { > > _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> 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? 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/@) { >> >> _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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 > _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> 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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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). Rasmus _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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 <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). 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. _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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. 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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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 <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). Thanks. Making the change seems like a good idea. julia _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> > 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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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 <efremov@linux.com> Cc: Julia Lawall <Julia.Lawall@lip6.fr> Cc: Gilles Muller <Gilles.Muller@lip6.fr> Cc: Nicolas Palix <nicolas.palix@imag.fr> Cc: Michal Marek <michal.lkml@markovi.net> Cc: Markus Elfring <Markus.Elfring@web.de> Cc: Joe Perches <joe@perches.com> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- 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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> +@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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> +/// 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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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 <efremov@linux.com> > Cc: Julia Lawall <Julia.Lawall@lip6.fr> > Cc: Gilles Muller <Gilles.Muller@lip6.fr> > Cc: Nicolas Palix <nicolas.palix@imag.fr> > Cc: Michal Marek <michal.lkml@markovi.net> > Cc: Markus Elfring <Markus.Elfring@web.de> > Cc: Joe Perches <joe@perches.com> > Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> Acked-by: Julia Lawall <julia.lawall@lip6.fr> 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 > > _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
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 <efremov@linux.com> >> Cc: Julia Lawall <Julia.Lawall@lip6.fr> >> Cc: Gilles Muller <Gilles.Muller@lip6.fr> >> Cc: Nicolas Palix <nicolas.palix@imag.fr> >> Cc: Michal Marek <michal.lkml@markovi.net> >> Cc: Markus Elfring <Markus.Elfring@web.de> >> Cc: Joe Perches <joe@perches.com> >> Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk> > > Acked-by: Julia Lawall <julia.lawall@lip6.fr> > > 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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
> 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 _______________________________________________ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci