[webkit-dev] Time to remove LIKELY and UNLIKELY macros?

[Adam Barth]

On Mon, Oct 1, 2012 at 11:29 PM, Maciej Stachowiak <mjs at apple.com> wrote:
> On Oct 1, 2012, at 10:55 PM, Adam Barth <abarth at webkit.org> wrote:
>> According to <http://stackoverflow.com/questions/1851299/is-it-possible-to-tell-the-branch-predictor-how-likely-it-is-to-follow-the-branc>,
>> __builtin_expect (what our LIKELY and UNLIKELY macros expand to [1])
>> doesn't do anything on modern CPUs.  Apparently, these used to be
>> important for PowerPC, but I don't think many folks use WebKit on
>> PowerPC anymore.
>>
>> Should we remove these macros?  I wasted some time today experimenting
>> with them without realizing that they compile to no-ops in clang.
>
> In general, removing code with no effect is good. More details on this case:
>
> I'd guess the most popular target architectures for WebKit are x86, x86_64, and ARMv7. Is it a no-op for all of those in clang? How did you determine that it is a no-op?

I misinterpreted #if COMPILER(GCC) as meaning "the compiler is gcc"
instead of "the compiler is compatibile with gcc."  (I started looking
at this issue because the macro didn't have an effect on the
dom-traverse benchmark I was studying.)

> And is the no-opness intentional or an llvm bug? Seems like not to me. At least some LLVM docs imply that it has an effect on codegen: <http://llvm.org/docs/BranchWeightMetadata.html>. This LLVM bug implies that it was implemented, but at least at one point, mistakenly had no effect on the optimizer: <http://llvm.org/bugs/show_bug.cgi?id=2577>. This LLVM bug implies that it should affect basic block reordering, but that was not turned on until somewhat recently: <http://llvm.org/bugs/show_bug.cgi?id=11096>.
>
> Note, despite the stackoverflow thread cited, I would be highly surprised if static branch predicition had no effect ever, even on modern architectures. While recent intel CPUs have very good dynamic branch prediction, the basic block reordering should still have a significant impact in some cases due to cache effects.

Haraken has an example where it does indeed affect the assembly that
is generated (to the benefit of the code in question).

It seem like we shouldn't remove these macros, but we should be
careful in reviewing patches that contain them to ensure that they're
added because of some measurable effect not not just because they look
fancy.

Adam