[Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

[PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Markus Elfring]

> +(
> +* !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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Markus Elfring]

> +(
> +* !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

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Markus Elfring]

> +(
> +* !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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Markus Elfring]

> +(
> +* !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

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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
> 

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Joe Perches]

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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Joe Perches]

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/@) {

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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/@) {
> 
> 

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Julia Lawall]

> 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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Julia Lawall]

> 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/@) {
>> 
>> 

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Rasmus Villemoes]

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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Rasmus Villemoes]

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

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Joe Perches]

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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Joe Perches]

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.

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Rasmus Villemoes]

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))"

Rasmus

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Julia Lawall]

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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Julia Lawall]

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

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

> 
> 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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

> 
> 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

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

> 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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

> 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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Pavel Machek]

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

Re: [Cocci] [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [PATCH] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

[PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Julia Lawall]

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

Re: [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Julia Lawall]

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

Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Rasmus Villemoes]

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

Re: [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Rasmus Villemoes]

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

Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Markus Elfring]

> +@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

Re: [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Markus Elfring]

> +@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

Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Markus Elfring]

> +/// 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

Re: [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Markus Elfring]

> +/// 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

Re: [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Markus Elfring]

> +/// 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

Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Julia Lawall]

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

Re: [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Julia Lawall]

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
>
>

Re: [Cocci] [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [PATCH v2] scripts: coccinelle: check for !(un)?likely usage

[Denis Efremov]

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

Re: [Cocci] [v2] scripts: coccinelle: check for !(un)?likely usage

[Markus Elfring]

> 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

Re: [v2] scripts: coccinelle: check for !(un)?likely usage

[Markus Elfring]

> 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