Re: [PATCH 1/3] Fix Unlikely(x) == y

[Willy Tarreau <w@1wt.eu>]

On Sat, Feb 16, 2008 at 09:42:26AM -0800, Arjan van de Ven wrote:
> On Sat, 16 Feb 2008 18:33:16 +0100
> Willy Tarreau <w@1wt.eu> wrote:
> 
> > On Sat, Feb 16, 2008 at 09:25:52AM -0800, Arjan van de Ven wrote:
> > > On Sat, 16 Feb 2008 17:08:01 +0100
> > > Roel Kluin <12o3l@tiscali.nl> wrote:
> > > 
> > > > The patch below was not yet tested. If it's correct as it is,
> > > > please comment. ---
> > > > Fix Unlikely(x) == y
> > > > 
> > > 
> > > you found a great set of bugs..
> > > but to be honest... I suspect it's just best to remove unlikely
> > > altogether for these cases; unlikely() is almost a
> > > go-faster-stripes thing, and if you don't know how to use it you
> > > shouldn't be using it... so just removing it for all wrong cases is
> > > actually the best thing to do imo.
> > 
> > Well, eventhough the author may not know how to use it, "unlikely" at
> > least indicates the intention of the author, or his knowledge of what
> > should happen here. I'd suggest leaving it where it is because the
> > authot of this code is in best position to know that this branch is
> > unlikely to happen, eventhough he does not correctly use the macro.
> >
> 
> you have more faith in the authors knowledge of how his code actually behaves than I think is warranted  :)
> Or faith in that he knows what "unlikely" means.
> I should write docs about this; but unlikely() means:
> 1) It happens less than 0.01% of the cases.
> 2) The compiler couldn't have figured this out by itself
>    (NULL pointer checks are compiler done already, same for some other conditions)
> 3) It's a hot codepath where shaving 0.5 cycles (less even on x86) matters
>    (and the author is ok with taking a 500 cycles hit if he's wrong)
> 
> If you think unlikely() means something else, we should fix what it maps to towards gcc ;)
> (to.. be empty ;)

eventhough the gcc docs say it's just a hint to help the compiler optimize
the branch it takes by default, I too have noticed that it more often does
bad than good. Code gets completely reordered and even sometimes partially
duplicated (especially when the branch is a return).

Last but not least, gcc 4 tends to emit stupid checks, to the point that I
have replaced unlikely(x) with (x) in my code when gcc >= 4 is detected. What
I observe is that the following code :

    if (unlikely(p == NULL)) ...

often gets coded like this :

    reg1 = (p == NULL)
    if (reg1 != 0) ...

... which clobbers reg1 for nothing and performs a double test.

But yes, I assumed that the author considered its use to be legitimate (I've
not looked at the code). Maybe you're right and it should be removed, but in
this case we would need a large audit of the abuses of unlikely()...

Willy