On Sun, 14 Jan 2018 01:45:42 +0300 Aleksey Baulin <aleksey.bau...@gmail.com> wrote:
> Please see my comments inline. > > On Sun, Jan 14, 2018 at 1:24 AM, Thomas Monjalon <tho...@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 <tho...@monjalon.net> > > > wrote: > > > > 21/11/2017 08:05, Aleksey Baulin: > > > > > On Mon, Nov 20, 2017 at 4:36 PM, Wiles, Keith <keith.wi...@intel.com > > > > > > > wrote: > > > > > > > On Nov 19, 2017, at 4:16 PM, Aleksey Baulin < > > > > aleksey.bau...@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.
