__builtin_expect (was: Re: master b0ba0d42b0f: * src/lisp.h (EQ): Improve generated code.)

[Pip Cet via "Emacs development discussions." <emacs-devel@gnu.org>]

On Thursday, November 28th, 2024 at 15:02, Andrea Corallo <acorallo@gnu.org> wrote:
> Pip Cet pipcet@protonmail.com writes:
> > On Thursday, November 28th, 2024 at 10:35, Andrea Corallo acorallo@gnu.org wrote:
> > 
> > > branch: master
> > > commit b0ba0d42b0fdf70a20cd7a070128db8abe4a0826
> > > Author: Andrea Corallo acorallo@gnu.org
> > > 
> > > Commit: Andrea Corallo acorallo@gnu.org
> > > 
> > > * src/lisp.h (EQ): Improve generated code.
> > > 
> > > Outside compilation 'symbols_with_pos_enabled' is always false, so ask
> > > the compiler to organize the most likely execution path in a sequential
> > > fashion in order to favor run-time performance.
> > 
> > Are we officially using __builtin_expect now?
> 
> config.h AFAIU defines it to a nop if the compiler does not support it.

It does, thanks!

I'm not sure that isn't a mere accident, though: currently, gnulib pulls in the builtin-expect module because it's used by gnulib internally, not because we explicitly requested it. If we want to use __builtin_expect in general, not just for special configurations (Android), we need to tell gnulib to pull in the `builtin-expect' module.

IOW, config.h isn't part of the Emacs sources, and whether it includes a section from builtin-expect.m4 depends on gnulib internals that may change without notice. We're talking about a compiler "feature" that the GCC manual advises us not to use, so I think that's a possibility.

But even if we can rely on the existence of the macro, "happens to be available" is not the same as "officially something we use". I still think it's a major decision, and needs to be discussed.

> > I think that's a major change to the way Emacs C code is written, and a decision which might benefit from further discussion.
> > 
> > To quote the GCC manual:
> > In general, you should prefer to use actual profile feedback for this (-fprofile-arcs), as programmers are notoriously bad at predicting how their programs actually perform.
> > 
> > Maybe we should use __builtin_expect_with_probability instead, in
> > those rare cases when we are certain we're making a correct
> > prediction? Or, my preference, avoid using __builtin_expect entirely,
> > so our scarce resources can be spent on more important issues?
> > 
> > I also don't think the assumption you're telling GCC to make in this specific case (more than 90% of calls to EQ happen while syms_with_pos_enabled == false) is obviously correct.
> 
> I think it is correct when we are not compiling, and as mentioned in the
> commit msg that's the case the patch is optimizing for.

The code isn't specific to "when we are not compiling", and the compiler won't read your commit message.

As it stands, we're telling GCC it's okay to make the "slow" case slower by a factor of 5.5 if it believes the "fast" case will be twice as fast.

Pip