Re: Using __builtin_expect (likely/unlikely macros)

[Eli Zaretskii]

> From: Alex Gramiak <address@hidden>
> Cc: address@hidden,  address@hidden
> Date: Mon, 15 Apr 2019 23:33:51 -0600
> 
> Eli Zaretskii <address@hidden> writes:
> 
> > I'm not sure whether you put LIKELY and UNLIKELY somewhat randomly,
> > just for testing, or did you really think each place is
> > likely/unlikely as in the change, but at least some places in xdisp.c
> > are wrong: they use LIKELY where UNLIKELY is more appropriate, abd
> > vice versa.
> 
> Out of curiosity, which places would that be? I only put UNLIKELY calls
> around the checks for emacs_abort (which I presume to be unlikely), and
> the LIKELY calls are of the form:
> 
>   else if (LIKELY (<cond>))
>     {
>       ...
>     }
>   else
>     emacs_abort ();

OK, it's possible that I don't understand the exact semantics of
__builtin_expect in these situations.

The problem is that you used LIKELY like this:

  if (A)
    do_A;
  else if (B)
    do_B;
  else if (C)
    do_C;
  else if (LIKELY (D))
    do_D;
  else
    cant_happen ();

Essentially, the above is a moral equivalent of a 'switch' with the
'default' case aborting because it "cannot happen".  In such code, the
order of the clauses doesn't necessarily tell anything about their
likelihood; up front, they all are equally "likely".  So using LIKELY
only in the last one sends a wrong signal: that last condition is
neither more nor less likely than all the others.  Actually, in some
cases it might be _less_ likely than the preceding ones, because if I
knew that some of these conditions happens much more frequently, I'd
test it first.

Now, it's possible that the effect of __builtin_expect doesn't care
about this issue.  The GCC manual doesn't help to figure out whether
this is the case, because it only talks about a simple case of a
single 'if' clause, and doesn't tell any details about what GCC is
allowed to do when it sees __builtin_expect.  But just by looking at
how the code looks, I immediately raised a brow.