Re: Inlining policy

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

On 4/5/20 3:23 AM, Andrea Corallo wrote:
> what about having a new macro to control the key operations we want to
> always have inlined at -O0 and -Og instead of falling back into defining
> these as macros?

I looked into this some more, and found some problems with using 
__attribute__((always_inline)), such as GCC complaining that it can't inline 
some functions in some cases on some platforms. It was a pain to suss out what 
would improve performance while avoiding the warnings, and while fiddling with 
this a bit I found a simpler change that had even better performance than 
__attribute__((always_inline)) did, at least on one platform for my standard 
benchmark of 'make compile-always'. So I installed the two attached patches 
instead. Together they improve Emacs CPU performance for 'gcc -Og' by about 60% 
on my benchmark.

I kept the DEFINE_KEY_OPS_AS_MACROS stuff because of the -O0 performance issues 
that we discussed earlier. Once we no longer need to worry about -O0 performance 
when debugging, we can get rid of that stuff.

Thanks for suggesting inlining as a way to improve Emacs performance when 
compiled with -Og.

[-- Attachment #2: 0001-Improve-inlining-when-compiling-with-Og.patch --]
[-- Type: text/x-patch, Size: 2538 bytes --]

From b9b32074cecb0e0d20cf527eadacbb0b7f5cced9 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 7 Apr 2020 16:57:04 -0700
Subject: [PATCH 1/2] Improve inlining when compiling with -Og
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* src/conf_post.h (EXTERN_INLINE) [!EMACS_EXTERN_INLINE]:
Make it static inline, not merely static.  This is a worthwhile
performance improvement on my two platforms A and B (see below).
On my platform A this change improves user+system CPU performance of
‘make compile-always’ by 52% on an -Og build, and by 1.4% on the
default -O2 build.  On my platform B this improves the same benchmark
by 41% on an -Og build, and by -0.8% on the default -O2 build.
That "-0.8%" is a small negative for this change, and I recall that
it is why I didn't make this change earlier.  However, Platform B uses
an older GCC so we needn't worry overmuch about this small negative.

With this change the performance advantage of -O2 over -Og has dropped
on platform A; formerly -O2 was 87% faster than -Og, and now it is
only 25% faster.  On platform B the performance advantage of -O2 over
-Og has dropped from being 62% faster to being 14% faster.

Platform A is GCC 9.3.1 20200317 (Red Hat 9.3.1-1) on Fedora 31
x86-64 (AMD Phenom II X4 910e, circa 2010).  Platform B is GCC (Ubuntu
7.5.0-3ubuntu1~18.04) 7.5.0 on Ubuntu 18.04.4 (Intel Xeon E3-1225 V2,
circa 2012).

This patch was inspired by a suggestion by Andrea Corallo in:
https://lists.gnu.org/r/emacs-devel/2020-04/msg00263.html
---
 src/conf_post.h | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/src/conf_post.h b/src/conf_post.h
index eb8fb18c00..79fb4cfe8a 100644
--- a/src/conf_post.h
+++ b/src/conf_post.h
@@ -432,15 +432,13 @@ extern int emacs_setenv_TZ (char const *);
 
 #else
 
-/* Use 'static' instead of 'extern inline' because 'static' typically
-   has better performance for Emacs.  Do not use the 'inline' keyword,
-   as modern compilers inline automatically.  ATTRIBUTE_UNUSED
-   pacifies gcc -Wunused-function.  */
+/* Use 'static inline' instead of 'extern inline' because 'static inline'
+   has much better performance for Emacs when compiled with 'gcc -Og'.  */
 
 # ifndef INLINE
 #  define INLINE EXTERN_INLINE
 # endif
-# define EXTERN_INLINE static ATTRIBUTE_UNUSED
+# define EXTERN_INLINE static inline
 # define INLINE_HEADER_BEGIN
 # define INLINE_HEADER_END
 
-- 
2.17.1


[-- Attachment #3: 0002-Revert-my-KEY_OPS_CFLAGS-change-to-src-Makefile.in.patch --]
[-- Type: text/x-patch, Size: 1777 bytes --]

From c36792bd79f3a97f292ab0612aa5c3ab1b4d9e70 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 7 Apr 2020 20:52:28 -0700
Subject: [PATCH 2/2] Revert my KEY_OPS_CFLAGS change to src/Makefile.in
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Now that -Og inlining has been improved this is no longer helpful.
* src/Makefile.in (KEY_OPS_CFLAGS): Remove.  All uses removed.
This improved CPU performance of ‘make compile-always’ by 5% on my
platform, which was gcc -Og, GCC 9.3.1 20200317 (Red Hat 9.3.1-1),
Fedora 31 x86-64 (AMD Phenom II X4 910e, circa 2010).
---
 src/Makefile.in | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/src/Makefile.in b/src/Makefile.in
index dfd322553b..552dd2e50a 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -377,14 +377,11 @@ pdmp :=
 # Flags that might be in WARN_CFLAGS but are not valid for Objective C.
 NON_OBJC_CFLAGS = -Wignored-attributes -Wignored-qualifiers -Wopenmp-simd
 
-# Cajole GCC into inlining key ops even if it wouldn't normally.
-KEY_OPS_CFLAGS = $(if $(filter -Og,$(CFLAGS)),-DDEFINE_KEY_OPS_AS_MACROS)
-
 # -Demacs makes some files produce the correct version for use in Emacs.
 # MYCPPFLAGS is for by-hand Emacs-specific overrides, e.g.,
 # "make MYCPPFLAGS='-DDBUS_DEBUG'".
-EMACS_CFLAGS = -Demacs $(KEY_OPS_CFLAGS) $(MYCPPFLAGS) \
-  -I. -I$(srcdir) -I$(lib) -I$(top_srcdir)/lib \
+EMACS_CFLAGS=-Demacs $(MYCPPFLAGS) -I. -I$(srcdir) \
+  -I$(lib) -I$(top_srcdir)/lib \
   $(C_SWITCH_MACHINE) $(C_SWITCH_SYSTEM) $(C_SWITCH_X_SITE) \
   $(GNUSTEP_CFLAGS) $(CFLAGS_SOUND) $(RSVG_CFLAGS) $(IMAGEMAGICK_CFLAGS) \
   $(PNG_CFLAGS) $(LIBXML2_CFLAGS) $(DBUS_CFLAGS) \
-- 
2.17.1