Hi Tim, On Fri, Feb 21, 2020 at 01:02:02PM +0100, Tim Duesterhus wrote: > Willy, > > I'm not sure whether there is a deeper purpose behind the old versions, but > I found them hard to read when stumbling upon them in the code. Especially > the ones in net_helper.
I agree. There was a reason for this, which is described in compiler.h. Gcc 4.x (at least 4.0 to 4.2) used to do totally stupid things on this: if (__builtin_expect((x) != 0, 1)) foo(); else bar(); In short, they would compare x to 0, build an integer 0 or 1 based on this and compare the result against 1 before doing the jump! So by doing if (likely(a == NULL)) we were actually doubling the test! It was doing something like this: cmp %rdi, $1 sbb %rax, %rax and %rax, $1 cmp %rax, $1 jne unlikely_branch likely_branch: ... unlikely_branch: ... The 3 instructions from sbb to cmp were only needed to build the value 1 to compare against in __builtin_expect()! The unlikely code used to only do this: test %rdi, %rdi je unlikely_branch likely_branch: ... unlikely_branch: ... Worse, by inflating the code, it was clobbering more registers and preventing some parts from being inlined. However for unlikely() it was not a problem as the comparison to zero doesn't require an intermediate result. I tried to make likely(x)=!unlikely(!x) but that also resulted in emitting extra code. Thus I decided to simply make likely(x) do nothing for such versions since in most cases it's not critical, but there were still some places where I really wanted to optimize the path after seeing the output code. This problem didn't exist for 3.x nor 5.x. This was in 2008... I have retested now with 4.4, 4.7, 4.8, 5.5, 7.4 and 8.2 and the problem seems to be definitely gone. I even think that we could remove the special case in the code, I'll do it. > Please carefully check that I did not break anything and that the intended > optimizations still apply. At first glance, your change looks OK, I'll take it. Thanks! Willy