[dpdk-dev] eal/common: better likely() and unlikely()

  A warning is issued when using an argument to likely() or unlikely()
builtins which is evaluated to a pointer value, as __builtin_expect()
expects a 'long int' type for its first argument. With this fix
a pointer value is converted to an integer with the value of 0 or 1.

Signed-off-by: Aleksey Baulin <Aleksey.Baulin@gmail.com>
---
 lib/librte_eal/common/include/rte_branch_prediction.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

> On Nov 19, 2017, at 4:16 PM, Aleksey Baulin <aleksey.baulin@gmail.com> wrote:
> 
> A warning is issued when using an argument to likely() or unlikely()
> builtins which is evaluated to a pointer value, as __builtin_expect()
> expects a 'long int' type for its first argument. With this fix
> a pointer value is converted to an integer with the value of 0 or 1.
> 
> Signed-off-by: Aleksey Baulin <Aleksey.Baulin@gmail.com>
> ---
> lib/librte_eal/common/include/rte_branch_prediction.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_branch_prediction.h b/lib/librte_eal/common/include/rte_branch_prediction.h
> index a6a56d1..2e7dc69 100644
> --- a/lib/librte_eal/common/include/rte_branch_prediction.h
> +++ b/lib/librte_eal/common/include/rte_branch_prediction.h
> @@ -50,7 +50,7 @@
>  *
>  */
> #ifndef likely
> -#define likely(x)  __builtin_expect((x),1)
> +#define likely(x)	__builtin_expect(!!(x), 1)
> #endif /* likely */
> 
> /**
> @@ -64,7 +64,7 @@
>  *
>  */
> #ifndef unlikely
> -#define unlikely(x)  __builtin_expect((x),0)
> +#define unlikely(x)	__builtin_expect(!!(x), 0)

I have not looked at the generated code, but does this add some extra instruction now to do the !!(x) ?

> #endif /* unlikely */
> 
> #endif /* _RTE_BRANCH_PREDICTION_H_ */
> -- 
> 2.7.4
> 

Regards,
Keith
  

> On Nov 20, 2017, at 7:36 AM, Wiles, Keith <keith.wiles@intel.com> wrote:
> 
>> On Nov 19, 2017, at 4:16 PM, Aleksey Baulin <aleksey.baulin@gmail.com> wrote:
>> 
>> #ifndef unlikely
>> -#define unlikely(x)  __builtin_expect((x),0)
>> +#define unlikely(x)	__builtin_expect(!!(x), 0)
> 
> I have not looked at the generated code, but does this add some extra instruction now to do the !!(x) ?
> 
>> #endif /* unlikely */
>> 
>> #endif /* _RTE_BRANCH_PREDICTION_H_ */
>> -- 
>> 2.7.4
>> 
> 
> Regards,
> Keith
> 

With no ‘-O’, you get an extra cmpl instruction with the double negated unlikely() .vs the one without the ‘!!’.
The same assembly is generated with ‘-O’ as a compiler switch.

Tested on:
[jim@blackbox-1 ~]$ uname -a
Linux blackbox-1.netgate.com 3.10.0-514.26.2.el7.x86_64 #1 SMP Tue Jul 4 15:04:05 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
[jim@blackbox-1 ~]$ gcc --version
gcc (GCC) 4.8.5 20150623 (Red Hat 4.8.5-11)
Copyright (C) 2015 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Jim
  

Sorry for late response. Jim had given the correct answer already.
You won't get an extra instruction with compiler optimization turned on.

Aleksey.

On Mon, Nov 20, 2017 at 4:36 PM, Wiles, Keith <keith.wiles@intel.com> wrote:

>
>
> > On Nov 19, 2017, at 4:16 PM, Aleksey Baulin <aleksey.baulin@gmail.com>
> wrote:
> >
> > A warning is issued when using an argument to likely() or unlikely()
> > builtins which is evaluated to a pointer value, as __builtin_expect()
> > expects a 'long int' type for its first argument. With this fix
> > a pointer value is converted to an integer with the value of 0 or 1.
> >
> > Signed-off-by: Aleksey Baulin <Aleksey.Baulin@gmail.com>
> > ---
> > lib/librte_eal/common/include/rte_branch_prediction.h | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/common/include/rte_branch_prediction.h
> b/lib/librte_eal/common/include/rte_branch_prediction.h
> > index a6a56d1..2e7dc69 100644
> > --- a/lib/librte_eal/common/include/rte_branch_prediction.h
> > +++ b/lib/librte_eal/common/include/rte_branch_prediction.h
> > @@ -50,7 +50,7 @@
> >  *
> >  */
> > #ifndef likely
> > -#define likely(x)  __builtin_expect((x),1)
> > +#define likely(x)    __builtin_expect(!!(x), 1)
> > #endif /* likely */
> >
> > /**
> > @@ -64,7 +64,7 @@
> >  *
> >  */
> > #ifndef unlikely
> > -#define unlikely(x)  __builtin_expect((x),0)
> > +#define unlikely(x)  __builtin_expect(!!(x), 0)
>
> I have not looked at the generated code, but does this add some extra
> instruction now to do the !!(x) ?
>
> > #endif /* unlikely */
> >
> > #endif /* _RTE_BRANCH_PREDICTION_H_ */
> > --
> > 2.7.4
> >
>
> Regards,
> Keith
>
>
  

21/11/2017 08:05, Aleksey Baulin:
> On Mon, Nov 20, 2017 at 4:36 PM, Wiles, Keith <keith.wiles@intel.com> wrote:
> > > On Nov 19, 2017, at 4:16 PM, Aleksey Baulin <aleksey.baulin@gmail.com>
> > wrote:
> > > -#define unlikely(x)  __builtin_expect((x),0)
> > > +#define unlikely(x)  __builtin_expect(!!(x), 0)
> >
> > I have not looked at the generated code, but does this add some extra
> > instruction now to do the !!(x) ?
> 
> Sorry for late response. Jim had given the correct answer already.
> You won't get an extra instruction with compiler optimization turned on.

So this patch is adding an instruction in not optimized binary.
I don't understand the benefit.
Is it just to avoid to make pointer comparison explicit?
likely(pointer != NULL) looks better than likely(pointer).
  

This is an interesting question. Perhaps, even a philosophical one. :-)

'likely(pointer)' is a perfectly legal statement in C language, as well as
a concise one as
compared to a more explicit (and redundant/superfluous) 'likely(pointer !=
NULL)'. If you
_require_ this kind of explicitness in cases like this in the code style,
then I have no
argument here. However, I don't see that anywhere.

There're other cases of explicitness, with the most widespread being a
series of logical and
compare operations in one statement. For instance, 'if (a > b && a < c)'.
Explicitness would
require writing it like this: 'if ((a > b) && (a < c))'. I've seen cases on
this list where that was
frowned upon as it's totally unnecessary due to C operator precedence
rules, even though
those statements, I think, looked better to their authors (actually, they
do to me). Granted,
it didn't lead to compiler errors, which is the case with the current
implementation of 'likely()'.

So, my answer to the question is yes, it's to avoid making pointer
comparison explicit. I would
add though, that it is to avoid making a perfectly legal C statement an
illegal one, as with the
way the current macro is constructed, compiler emits an error when DPDK is
built. I write in C
for many years with the most time spent in kernels, Linux and not, and I
find it unnatural to
always add a redundant '!= NULL' just to satisfy the current macro
implementation. I would
have to accept that though if it's a requirement clearly stated somewhere
like a code style.

As for an extra instruction, I believe that everything in DPDK distribution
is compiled with
optimization. So the execution speed in not a concern here. Perhaps there
are cases where
it's compiled without optimization, like debugging, but then I believe it's
a non-issue.

Hope my explanations shed some more light on this patch. :-)


On Fri, Jan 12, 2018 at 6:35 PM, Thomas Monjalon <thomas@monjalon.net>
wrote:

> 21/11/2017 08:05, Aleksey Baulin:
> > On Mon, Nov 20, 2017 at 4:36 PM, Wiles, Keith <keith.wiles@intel.com>
> wrote:
> > > > On Nov 19, 2017, at 4:16 PM, Aleksey Baulin <
> aleksey.baulin@gmail.com>
> > > wrote:
> > > > -#define unlikely(x)  __builtin_expect((x),0)
> > > > +#define unlikely(x)  __builtin_expect(!!(x), 0)
> > >
> > > I have not looked at the generated code, but does this add some extra
> > > instruction now to do the !!(x) ?
> >
> > Sorry for late response. Jim had given the correct answer already.
> > You won't get an extra instruction with compiler optimization turned on.
>
> So this patch is adding an instruction in not optimized binary.
> I don't understand the benefit.
> Is it just to avoid to make pointer comparison explicit?
> likely(pointer != NULL) looks better than likely(pointer).
>
  

Hi,

I moved your top-post below and did some comments inline.
More opinions are welcome.

13/01/2018 23:05, Aleksey Baulin:
> On Fri, Jan 12, 2018 at 6:35 PM, Thomas Monjalon <thomas@monjalon.net>
> wrote:
> > 21/11/2017 08:05, Aleksey Baulin:
> > > On Mon, Nov 20, 2017 at 4:36 PM, Wiles, Keith <keith.wiles@intel.com>
> > wrote:
> > > > > On Nov 19, 2017, at 4:16 PM, Aleksey Baulin <
> > aleksey.baulin@gmail.com>
> > > > wrote:
> > > > > -#define unlikely(x)  __builtin_expect((x),0)
> > > > > +#define unlikely(x)  __builtin_expect(!!(x), 0)
> > > >
> > > > I have not looked at the generated code, but does this add some extra
> > > > instruction now to do the !!(x) ?
> > >
> > > Sorry for late response. Jim had given the correct answer already.
> > > You won't get an extra instruction with compiler optimization turned on.
> >
> > So this patch is adding an instruction in not optimized binary.
> > I don't understand the benefit.
> > Is it just to avoid to make pointer comparison explicit?
> > likely(pointer != NULL) looks better than likely(pointer).
> 
> This is an interesting question. Perhaps, even a philosophical one. :-)
> 
> 'likely(pointer)' is a perfectly legal statement in C language, as well as
> a concise one as
> compared to a more explicit (and redundant/superfluous) 'likely(pointer !=
> NULL)'. If you
> _require_ this kind of explicitness in cases like this in the code style,
> then I have no
> argument here. However, I don't see that anywhere.

It is stated here:
	http://dpdk.org/doc/guides/contributing/coding_style.html#null-pointers

> There're other cases of explicitness, with the most widespread being a
> series of logical and
> compare operations in one statement. For instance, 'if (a > b && a < c)'.
> Explicitness would
> require writing it like this: 'if ((a > b) && (a < c))'. I've seen cases on
> this list where that was
> frowned upon as it's totally unnecessary due to C operator precedence
> rules, even though
> those statements, I think, looked better to their authors (actually, they
> do to me). Granted,
> it didn't lead to compiler errors, which is the case with the current
> implementation of 'likely()'.
> 
> So, my answer to the question is yes, it's to avoid making pointer
> comparison explicit. I would
> add though, that it is to avoid making a perfectly legal C statement an
> illegal one, as with the
> way the current macro is constructed, compiler emits an error when DPDK is
> built. I write in C
> for many years with the most time spent in kernels, Linux and not, and I
> find it unnatural to
> always add a redundant '!= NULL' just to satisfy the current macro
> implementation. I would
> have to accept that though if it's a requirement clearly stated somewhere
> like a code style.
> 
> As for an extra instruction, I believe that everything in DPDK distribution
> is compiled with
> optimization. So the execution speed in not a concern here. Perhaps there
> are cases where
> it's compiled without optimization, like debugging, but then I believe it's
> a non-issue.

Yes you're right about optimization.
But can we be 100% sure that it is always well optimized?

> Hope my explanations shed some more light on this patch. :-)

If we can be sure that there is no cost on optimized code,
I am not against this patch.
It may be especially useful when not complying to the DPDK
coding rules, like in applications.
  

Please see my comments inline.

On Sun, Jan 14, 2018 at 1:24 AM, Thomas Monjalon <thomas@monjalon.net>
wrote:

> Hi,
>
> I moved your top-post below and did some comments inline.
> More opinions are welcome.
>
> 13/01/2018 23:05, Aleksey Baulin:
> > On Fri, Jan 12, 2018 at 6:35 PM, Thomas Monjalon <thomas@monjalon.net>
> > wrote:
> > > 21/11/2017 08:05, Aleksey Baulin:
> > > > On Mon, Nov 20, 2017 at 4:36 PM, Wiles, Keith <keith.wiles@intel.com
> >
> > > wrote:
> > > > > > On Nov 19, 2017, at 4:16 PM, Aleksey Baulin <
> > > aleksey.baulin@gmail.com>
> > > > > wrote:
> > > > > > -#define unlikely(x)  __builtin_expect((x),0)
> > > > > > +#define unlikely(x)  __builtin_expect(!!(x), 0)
> > > > >
> > > > > I have not looked at the generated code, but does this add some
> extra
> > > > > instruction now to do the !!(x) ?
> > > >
> > > > Sorry for late response. Jim had given the correct answer already.
> > > > You won't get an extra instruction with compiler optimization turned
> on.
> > >
> > > So this patch is adding an instruction in not optimized binary.
> > > I don't understand the benefit.
> > > Is it just to avoid to make pointer comparison explicit?
> > > likely(pointer != NULL) looks better than likely(pointer).
> >
> > This is an interesting question. Perhaps, even a philosophical one. :-)
> >
> > 'likely(pointer)' is a perfectly legal statement in C language, as well
> as
> > a concise one as
> > compared to a more explicit (and redundant/superfluous) 'likely(pointer
> !=
> > NULL)'. If you
> > _require_ this kind of explicitness in cases like this in the code style,
> > then I have no
> > argument here. However, I don't see that anywhere.
>
> It is stated here:
>         http://dpdk.org/doc/guides/contributing/coding_style.
> html#null-pointers


​Oh, thanks for pointing that out! I am sincerely ashamed for missing it.​
I lose that argument as I certainly do submit to the coding style. My only
excuse is that I am actually developing an app and not the DPDK core.


> > There're other cases of explicitness, with the most widespread being a
> > series of logical and
> > compare operations in one statement. For instance, 'if (a > b && a < c)'.
> > Explicitness would
> > require writing it like this: 'if ((a > b) && (a < c))'. I've seen cases
> on
> > this list where that was
> > frowned upon as it's totally unnecessary due to C operator precedence
> > rules, even though
> > those statements, I think, looked better to their authors (actually, they
> > do to me). Granted,
> > it didn't lead to compiler errors, which is the case with the current
> > implementation of 'likely()'.
> >
> > So, my answer to the question is yes, it's to avoid making pointer
> > comparison explicit. I would
> > add though, that it is to avoid making a perfectly legal C statement an
> > illegal one, as with the
> > way the current macro is constructed, compiler emits an error when DPDK
> is
> > built. I write in C
> > for many years with the most time spent in kernels, Linux and not, and I
> > find it unnatural to
> > always add a redundant '!= NULL' just to satisfy the current macro
> > implementation. I would
> > have to accept that though if it's a requirement clearly stated somewhere
> > like a code style.
> >
> > As for an extra instruction, I believe that everything in DPDK
> distribution
> > is compiled with
> > optimization. So the execution speed in not a concern here. Perhaps there
> > are cases where
> > it's compiled without optimization, like debugging, but then I believe
> it's
> > a non-issue.
>
> Yes you're right about optimization.
> But can we be 100% sure that it is always well optimized?
>

​I believe we can. I hope we get other opinions as well.​

> Hope my explanations shed some more light on this patch. :-)
>
> If we can be sure that there is no cost on optimized code,
> I am not against this patch.
> It may be especially useful when not complying to the DPDK
> coding rules, like in applications.
>

​Yes, that's exactly my case. Thanks.​
  

On Sun, 14 Jan 2018 01:45:42 +0300
Aleksey Baulin <Aleksey.Baulin@gmail.com> wrote:

> Please see my comments inline.
> 
> On Sun, Jan 14, 2018 at 1:24 AM, Thomas Monjalon <thomas@monjalon.net>
> wrote:
> 
> > Hi,
> >
> > I moved your top-post below and did some comments inline.
> > More opinions are welcome.
> >
> > 13/01/2018 23:05, Aleksey Baulin:  
> > > On Fri, Jan 12, 2018 at 6:35 PM, Thomas Monjalon <thomas@monjalon.net>
> > > wrote:  
> > > > 21/11/2017 08:05, Aleksey Baulin:  
> > > > > On Mon, Nov 20, 2017 at 4:36 PM, Wiles, Keith <keith.wiles@intel.com  
> > >  
> > > > wrote:  
> > > > > > > On Nov 19, 2017, at 4:16 PM, Aleksey Baulin <  
> > > > aleksey.baulin@gmail.com>  
> > > > > > wrote:  
> > > > > > > -#define unlikely(x)  __builtin_expect((x),0)
> > > > > > > +#define unlikely(x)  __builtin_expect(!!(x), 0)  
> > > > > >
> > > > > > I have not looked at the generated code, but does this add some  
> > extra  
> > > > > > instruction now to do the !!(x) ?  
> > > > >
> > > > > Sorry for late response. Jim had given the correct answer already.
> > > > > You won't get an extra instruction with compiler optimization turned  
> > on.  
> > > >
> > > > So this patch is adding an instruction in not optimized binary.
> > > > I don't understand the benefit.
> > > > Is it just to avoid to make pointer comparison explicit?
> > > > likely(pointer != NULL) looks better than likely(pointer).  
> > >
> > > This is an interesting question. Perhaps, even a philosophical one. :-)
> > >
> > > 'likely(pointer)' is a perfectly legal statement in C language, as well  
> > as  
> > > a concise one as
> > > compared to a more explicit (and redundant/superfluous) 'likely(pointer  
> > !=  
> > > NULL)'. If you
> > > _require_ this kind of explicitness in cases like this in the code style,
> > > then I have no
> > > argument here. However, I don't see that anywhere.  
> >
> > It is stated here:
> >         http://dpdk.org/doc/guides/contributing/coding_style.
> > html#null-pointers  
> 
> 
> ​Oh, thanks for pointing that out! I am sincerely ashamed for missing it.​
> I lose that argument as I certainly do submit to the coding style. My only
> excuse is that I am actually developing an app and not the DPDK core.
> 
> 
> > > There're other cases of explicitness, with the most widespread being a
> > > series of logical and
> > > compare operations in one statement. For instance, 'if (a > b && a < c)'.
> > > Explicitness would
> > > require writing it like this: 'if ((a > b) && (a < c))'. I've seen cases  
> > on  
> > > this list where that was
> > > frowned upon as it's totally unnecessary due to C operator precedence
> > > rules, even though
> > > those statements, I think, looked better to their authors (actually, they
> > > do to me). Granted,
> > > it didn't lead to compiler errors, which is the case with the current
> > > implementation of 'likely()'.
> > >
> > > So, my answer to the question is yes, it's to avoid making pointer
> > > comparison explicit. I would
> > > add though, that it is to avoid making a perfectly legal C statement an
> > > illegal one, as with the
> > > way the current macro is constructed, compiler emits an error when DPDK  
> > is  
> > > built. I write in C
> > > for many years with the most time spent in kernels, Linux and not, and I
> > > find it unnatural to
> > > always add a redundant '!= NULL' just to satisfy the current macro
> > > implementation. I would
> > > have to accept that though if it's a requirement clearly stated somewhere
> > > like a code style.
> > >
> > > As for an extra instruction, I believe that everything in DPDK  
> > distribution  
> > > is compiled with
> > > optimization. So the execution speed in not a concern here. Perhaps there
> > > are cases where
> > > it's compiled without optimization, like debugging, but then I believe  
> > it's  
> > > a non-issue.  
> >
> > Yes you're right about optimization.
> > But can we be 100% sure that it is always well optimized?
> >  
> 
> ​I believe we can. I hope we get other opinions as well.​
> 
> > Hope my explanations shed some more light on this patch. :-)
> >
> > If we can be sure that there is no cost on optimized code,
> > I am not against this patch.
> > It may be especially useful when not complying to the DPDK
> > coding rules, like in applications.
> >  
> 
> ​Yes, that's exactly my case. Thanks.​
> 

My opinion is that the DPDK likely() macro must behave exactly the same
as the kernel and other projects. Doing something unique is not a great benefit.
  

19/11/2017 23:16, Aleksey Baulin:
> A warning is issued when using an argument to likely() or unlikely()
> builtins which is evaluated to a pointer value, as __builtin_expect()
> expects a 'long int' type for its first argument. With this fix
> a pointer value is converted to an integer with the value of 0 or 1.
> 
> Signed-off-by: Aleksey Baulin <Aleksey.Baulin@gmail.com>

After philosophical debates,
Applied, thanks :)