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
