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

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

[Pip Cet via Emacs development discussions.]

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?

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.

IIUC, the current master branch won't even compile with a compiler that doesn't handle (or ignore) __builtin_expect.

Pip

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

[Eli Zaretskii]

> Date: Thu, 28 Nov 2024 13:53:40 +0000
> Cc: Emacs Devel <emacs-devel@gnu.org>
> From:  Pip Cet via "Emacs development discussions." <emacs-devel@gnu.org>
> 
> 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?
> 
> 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.
> 
> IIUC, the current master branch won't even compile with a compiler that doesn't handle (or ignore) __builtin_expect.

I think we should us __builtin_expect only when building with GCC,
indeed.

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

[Andrea Corallo]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Thu, 28 Nov 2024 13:53:40 +0000
>> Cc: Emacs Devel <emacs-devel@gnu.org>
>> From:  Pip Cet via "Emacs development discussions." <emacs-devel@gnu.org>
>> 
>> 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?
>> 
>> 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.
>> 
>> IIUC, the current master branch won't even compile with a compiler that doesn't handle (or ignore) __builtin_expect.
>
> I think we should us __builtin_expect only when building with GCC,
> indeed.

AFAICS config.h defines __builtin_expect to a nop if the used compiler
does not support it, am I wrong?

  Andrea

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

[Eli Zaretskii]

> From: Andrea Corallo <acorallo@gnu.org>
> Cc: Pip Cet <pipcet@protonmail.com>,  emacs-devel@gnu.org
> Date: Thu, 28 Nov 2024 09:55:34 -0500
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I think we should us __builtin_expect only when building with GCC,
> > indeed.
> 
> AFAICS config.h defines __builtin_expect to a nop if the used compiler
> does not support it, am I wrong?

Ah, you are right, sorry.

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

[Andrea Corallo]

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.

> 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.

> IIUC, the current master branch won't even compile with a compiler that doesn't handle (or ignore) __builtin_expect.

See my first point.

  Andrea

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

[Mattias Engdegård]

28 nov. 2024 kl. 14.53 skrev Pip Cet via Emacs development discussions. <emacs-devel@gnu.org>:

> 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?

Actually __builtin_expect can definitely provide a measurable performance improvement, mainly for BB ordering and cold-path moving as suggested by Andrea's commit note. I've been thinking about using it in other cases.

We should probably define some variant of likely/unlikely because those cover most needs of __builtin_expect, rather than using it directly. Then portability wouldn't be a problem.

> 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.

On this point I agree -- this particular change may have been made in haste, although I applaud the spirit.

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

[Pip Cet via Emacs development discussions.]

On Thursday, November 28th, 2024 at 17:53, Mattias Engdegård <mattias.engdegard@gmail.com> wrote:
> 28 nov. 2024 kl. 14.53 skrev Pip Cet via Emacs development discussions. emacs-devel@gnu.org:
> 
> > 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?
> 
> Actually __builtin_expect can definitely provide a measurable performance improvement,

If we want that improvement, let's use a profile-guided build. No need to reduce the legibility of our C code further by using a GCC extension which is largely obsoleted by PGO and __builtin_expect_with_probability.

> mainly for BB ordering and cold-path moving as suggested by Andrea's commit note. I've been thinking about using it in other cases.

I think we should heed the warning in the GCC manual: it is very hard to get a good grasp of whether __builtin_expect provides any benefit.

> We should probably define some variant of likely/unlikely because those cover most needs of __builtin_expect, rather than using it directly. Then portability wouldn't be a problem.

That's what the Android code already does, IIUC. I don't think that solves any problems, because the compiler uses a 90% probability for __builtin_expect, and "likely" is simply misleading for that.

> > 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.
> 
> On this point I agree -- this particular change may have been made in haste, although I applaud the spirit.

So let's discuss this if and when a real usecase for __builtin_expect arises, and we conclude that use case couldn't be addressed by PGO?

Pip

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

[Eli Zaretskii]

> From: Mattias Engdegård <mattias.engdegard@gmail.com>
> Date: Thu, 28 Nov 2024 18:53:46 +0100
> Cc: Andrea Corallo <acorallo@gnu.org>,
>  Emacs Devel <emacs-devel@gnu.org>
> 
> We should probably define some variant of likely/unlikely because those cover most needs of __builtin_expect, rather than using it directly. Then portability wouldn't be a problem.

Portability is already not a problem, so I see no need for inventing
new macros for this purpose.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Mattias Engdegård <mattias.engdegard@gmail.com>
>> Date: Thu, 28 Nov 2024 18:53:46 +0100
>> Cc: Andrea Corallo <acorallo@gnu.org>,
>>  Emacs Devel <emacs-devel@gnu.org>
>>
>> We should probably define some variant of likely/unlikely because those cover most needs of __builtin_expect, rather than using it directly. Then portability wouldn't be a problem.
>
> Portability is already not a problem, so I see no need for inventing
> new macros for this purpose.

Right, since this is already done for us by Gnulib. But the other
reason to having such macros is readability.

To be clear, the proposed macros would look something like this (copied
here from the Linux kernel):

#define likely(x)      __builtin_expect(!!(x), 1)
#define unlikely(x)    __builtin_expect(!!(x), 0)

Andrea's code would then read like so:

-  return BASE_EQ ((__builtin_expect (symbols_with_pos_enabled, false)
+  return BASE_EQ ((unlikely (symbols_with_pos_enabled)

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

[Björn Lindqvist]

Den tors 28 nov. 2024 kl 18:54 skrev Mattias Engdegård
<mattias.engdegard@gmail.com>:
>
> 28 nov. 2024 kl. 14.53 skrev Pip Cet via Emacs development discussions. <emacs-devel@gnu.org>:
>
> > 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?
>
> Actually __builtin_expect can definitely provide a measurable performance improvement, mainly for BB ordering and cold-path moving as suggested by Andrea's commit note. I've been thinking about using it in other cases.

It can also worsen performance if one is not careful. Micro
optimization is very often very counter-intuitive so relying on
theoretical improvements is often not advisable. Especially not for
code that should run efficiently on dozens of different platforms
years into the future.


-- 
mvh/best regards Björn Lindqvist

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

[Andrea Corallo]

Stefan Kangas <stefankangas@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: Mattias Engdegård <mattias.engdegard@gmail.com>
>>> Date: Thu, 28 Nov 2024 18:53:46 +0100
>>> Cc: Andrea Corallo <acorallo@gnu.org>,
>>>  Emacs Devel <emacs-devel@gnu.org>
>>>
>>> We should probably define some variant of likely/unlikely because
>>> those cover most needs of __builtin_expect, rather than using it
>>> directly. Then portability wouldn't be a problem.
>>
>> Portability is already not a problem, so I see no need for inventing
>> new macros for this purpose.
>
> Right, since this is already done for us by Gnulib. But the other
> reason to having such macros is readability.
>
> To be clear, the proposed macros would look something like this (copied
> here from the Linux kernel):
>
> #define likely(x)      __builtin_expect(!!(x), 1)
> #define unlikely(x)    __builtin_expect(!!(x), 0)
>
> Andrea's code would then read like so:
>
> -  return BASE_EQ ((__builtin_expect (symbols_with_pos_enabled, false)
> +  return BASE_EQ ((unlikely (symbols_with_pos_enabled)

Hi Stefan,

I think I'm with Eli in suggesting we should just use __builtin_expect.
Yes it reads a little longer but:

1- We define a layer of macros less

2- It's to me still very readable

3- In my mind introducing ad-hoc macros might encourage developers using
them, while I think developers should refrain using them unless they
really know what they are doing.  IOW annotating branchs should be the
rare exception rather than the rule, and for these rare cases
__builtin_expect is IMO fine.

  Andrea