Inlining policy (was: emacs rendering comparisson between emacs23 and emacs26.3)

[Andrea Corallo <akrl@sdf.org>]

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

Paul Eggert <eggert@cs.ucla.edu> writes:

> I looked into this, and although it's no doubt fundamentally due to a
> slow algorithm, the slowness is exacerbated if you use -Og (which you
> appear to be using). Stefan's recent message hinted at this. I
> installed the attached into master to try to fix the -Og issue; please
> give it a try.

Hi Paul,

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?

Something like the attached patch.

Going further I think we could also use this to take control of the
subset of functions we really want to have always inlined also when
optimizing.

Don't know maybe this solution of the always_inline attribute has been
already attempted, in case apologies for the noise.

  Andrea

--
akrl@sdf.org

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Introduce-KEY_OP_INLINE-for-key-operations-inlining.patch --]
[-- Type: text/x-diff, Size: 10707 bytes --]

From 902a46835d8c37089de407d847ce1259c477dc00 Mon Sep 17 00:00:00 2001
From: Andrea Corallo <akrl@sdf.org>
Date: Sun, 5 Apr 2020 10:09:43 +0100
Subject: [PATCH] Introduce KEY_OP_INLINE for key operations inlining.

2020-04-05  Andrea Corallo  <akrl@sdf.org>

	* src/lisp.h:
	(XLI, XIL, XLP, XPL, CHECK_FIXNUM, CHECK_SYMBOL, CHECK_TYPE)
	(CONSP, EQ, FLOATP, FIXNUMP, NILP, SET_SYMBOL_VAL)
	(SYMBOL_CONSTANT_P, SYMBOL_TRAPPED_WRITE_P, SYMBOL_VAL, SYMBOLP)
	(TAGGEDP, VECTORLIKEP, XCAR, XCDR, XCONS, XHASH, make_fixnum)
	(XFIXNUM_RAW, XTYPE): Do not define these as macro plus use
	KEY_OP_INLINE in the function definition.
	* src/conf_post.h (ATTRIBUTE_ALWAYS_INLINE): New macro.
	(KEY_OP_INLINE): New macro.
	* src/Makefile.in (DEFINE_KEY_OPS_AS_MACROS): Rename into
	FORCE_INLINE_KEY_OPS.
---
 src/Makefile.in |   2 +-
 src/conf_post.h |  14 +++++++
 src/lisp.h      | 107 ++++++++++++++----------------------------------
 3 files changed, 45 insertions(+), 78 deletions(-)

diff --git a/src/Makefile.in b/src/Makefile.in
index dfd322553b..51c200dc9e 100644
--- a/src/Makefile.in
+++ b/src/Makefile.in
@@ -378,7 +378,7 @@ pdmp :=
 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)
+KEY_OPS_CFLAGS = $(if $(filter -Og,$(CFLAGS)),-DFORCE_INLINE_KEY_OPS)
 
 # -Demacs makes some files produce the correct version for use in Emacs.
 # MYCPPFLAGS is for by-hand Emacs-specific overrides, e.g.,
diff --git a/src/conf_post.h b/src/conf_post.h
index eb8fb18c00..441e19d829 100644
--- a/src/conf_post.h
+++ b/src/conf_post.h
@@ -240,6 +240,8 @@ #define NO_INLINE __attribute__((noinline))
 #define NO_INLINE
 #endif
 
+#define ATTRIBUTE_ALWAYS_INLINE __attribute__((always_inline))
+
 #if __has_attribute (externally_visible)
 #define EXTERNALLY_VISIBLE __attribute__((externally_visible))
 #else
@@ -446,6 +448,18 @@ #define ATTRIBUTE_MALLOC_SIZE(args) ATTRIBUTE_MALLOC ATTRIBUTE_ALLOC_SIZE (args)
 
 #endif
 
+/* Have key operations inlined at -O0 and -Og not to excessively kill
+   performance.  */
+#ifndef KEY_OP_INLINE
+# if (! defined __OPTIMIZE__ \
+      || (defined FORCE_INLINE_KEY_OPS && ! defined __NO_INLINE__ \
+	  && ! defined __OPTIMIZE_SIZE__))
+#  define KEY_OP_INLINE static inline ATTRIBUTE_UNUSED ATTRIBUTE_ALWAYS_INLINE
+# else
+#  define KEY_OP_INLINE INLINE
+# endif
+#endif
+
 /* 'int x UNINIT;' is equivalent to 'int x;', except it cajoles GCC
    into not warning incorrectly about use of an uninitialized variable.  */
 #if defined GCC_LINT || defined lint
diff --git a/src/lisp.h b/src/lisp.h
index 23ff89a977..168a496d58 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -411,53 +411,6 @@ #define lisp_h_XHASH(a) XUFIXNUM_RAW (a)
 # define lisp_h_XTYPE(a) ((enum Lisp_Type) (XLI (a) & ~VALMASK))
 #endif
 
-/* When DEFINE_KEY_OPS_AS_MACROS, define key operations as macros to
-   cajole the compiler into inlining them; otherwise define them as
-   inline functions as this is cleaner and can be more efficient.
-   The default is true if the compiler is GCC-like and if function
-   inlining is disabled because the compiler is not optimizing or is
-   optimizing for size.  Otherwise the default is false.  */
-#ifndef DEFINE_KEY_OPS_AS_MACROS
-# if (defined __NO_INLINE__ \
-      && ! defined __OPTIMIZE__ && ! defined __OPTIMIZE_SIZE__)
-#  define DEFINE_KEY_OPS_AS_MACROS true
-# else
-#  define DEFINE_KEY_OPS_AS_MACROS false
-# endif
-#endif
-
-#if DEFINE_KEY_OPS_AS_MACROS
-# define XLI(o) lisp_h_XLI (o)
-# define XIL(i) lisp_h_XIL (i)
-# define XLP(o) lisp_h_XLP (o)
-# define XPL(p) lisp_h_XPL (p)
-# define CHECK_FIXNUM(x) lisp_h_CHECK_FIXNUM (x)
-# define CHECK_SYMBOL(x) lisp_h_CHECK_SYMBOL (x)
-# define CHECK_TYPE(ok, predicate, x) lisp_h_CHECK_TYPE (ok, predicate, x)
-# define CONSP(x) lisp_h_CONSP (x)
-# define EQ(x, y) lisp_h_EQ (x, y)
-# define FLOATP(x) lisp_h_FLOATP (x)
-# define FIXNUMP(x) lisp_h_FIXNUMP (x)
-# define NILP(x) lisp_h_NILP (x)
-# define SET_SYMBOL_VAL(sym, v) lisp_h_SET_SYMBOL_VAL (sym, v)
-# define SYMBOL_CONSTANT_P(sym) lisp_h_SYMBOL_CONSTANT_P (sym)
-# define SYMBOL_TRAPPED_WRITE_P(sym) lisp_h_SYMBOL_TRAPPED_WRITE_P (sym)
-# define SYMBOL_VAL(sym) lisp_h_SYMBOL_VAL (sym)
-# define SYMBOLP(x) lisp_h_SYMBOLP (x)
-# define TAGGEDP(a, tag) lisp_h_TAGGEDP (a, tag)
-# define VECTORLIKEP(x) lisp_h_VECTORLIKEP (x)
-# define XCAR(c) lisp_h_XCAR (c)
-# define XCDR(c) lisp_h_XCDR (c)
-# define XCONS(a) lisp_h_XCONS (a)
-# define XHASH(a) lisp_h_XHASH (a)
-# if USE_LSB_TAG
-#  define make_fixnum(n) lisp_h_make_fixnum (n)
-#  define XFIXNUM_RAW(a) lisp_h_XFIXNUM_RAW (a)
-#  define XTYPE(a) lisp_h_XTYPE (a)
-# endif
-#endif
-
-
 /* Define the fundamental Lisp data structures.  */
 
 /* This is the set of Lisp data types.  If you want to define a new
@@ -718,25 +671,25 @@ definitely_will_not_unexec_p (void)
    if pointers differ in width from EMACS_INT; otherwise they are
    no-ops.  */
 
-INLINE EMACS_INT
+KEY_OP_INLINE EMACS_INT
 (XLI) (Lisp_Object o)
 {
   return lisp_h_XLI (o);
 }
 
-INLINE Lisp_Object
+KEY_OP_INLINE Lisp_Object
 (XIL) (EMACS_INT i)
 {
   return lisp_h_XIL (i);
 }
 
-INLINE void *
+KEY_OP_INLINE void *
 (XLP) (Lisp_Object o)
 {
   return lisp_h_XLP (o);
 }
 
-INLINE Lisp_Object
+KEY_OP_INLINE Lisp_Object
 (XPL) (void *p)
 {
   return lisp_h_XPL (p);
@@ -744,7 +697,7 @@ definitely_will_not_unexec_p (void)
 
 /* Extract A's type.  */
 
-INLINE enum Lisp_Type
+KEY_OP_INLINE enum Lisp_Type
 (XTYPE) (Lisp_Object a)
 {
 #if USE_LSB_TAG
@@ -758,13 +711,13 @@ definitely_will_not_unexec_p (void)
 /* True if A has type tag TAG.
    Equivalent to XTYPE (a) == TAG, but often faster.  */
 
-INLINE bool
+KEY_OP_INLINE bool
 (TAGGEDP) (Lisp_Object a, enum Lisp_Type tag)
 {
   return lisp_h_TAGGEDP (a, tag);
 }
 
-INLINE void
+KEY_OP_INLINE void
 (CHECK_TYPE) (int ok, Lisp_Object predicate, Lisp_Object x)
 {
   lisp_h_CHECK_TYPE (ok, predicate, x);
@@ -991,13 +944,13 @@ #define ROUNDUP(x, y) (POWER_OF_2 (y)					\
     ptrdiff_t size;
   };
 
-INLINE bool
+KEY_OP_INLINE bool
 (SYMBOLP) (Lisp_Object x)
 {
   return lisp_h_SYMBOLP (x);
 }
 
-INLINE struct Lisp_Symbol * ATTRIBUTE_NO_SANITIZE_UNDEFINED
+KEY_OP_INLINE struct Lisp_Symbol * ATTRIBUTE_NO_SANITIZE_UNDEFINED
 XSYMBOL (Lisp_Object a)
 {
   eassert (SYMBOLP (a));
@@ -1052,7 +1005,7 @@ c_symbol_p (struct Lisp_Symbol *sym)
     }
 }
 
-INLINE void
+KEY_OP_INLINE void
 (CHECK_SYMBOL) (Lisp_Object x)
 {
   lisp_h_CHECK_SYMBOL (x);
@@ -1146,14 +1099,14 @@ #define FIXNUM_OVERFLOW_P(i) \
 
 #if USE_LSB_TAG
 
-INLINE Lisp_Object
+KEY_OP_INLINE Lisp_Object
 (make_fixnum) (EMACS_INT n)
 {
   eassert (!FIXNUM_OVERFLOW_P (n));
   return lisp_h_make_fixnum_wrap (n);
 }
 
-INLINE EMACS_INT
+KEY_OP_INLINE EMACS_INT
 (XFIXNUM_RAW) (Lisp_Object a)
 {
   return lisp_h_XFIXNUM_RAW (a);
@@ -1173,7 +1126,7 @@ make_ufixnum (EMACS_INT n)
    the lisp_h_* macros are eventually removed.  */
 
 /* Make a fixnum representing the value of the low order bits of N.  */
-INLINE Lisp_Object
+KEY_OP_INLINE Lisp_Object
 make_fixnum (EMACS_INT n)
 {
   eassert (! FIXNUM_OVERFLOW_P (n));
@@ -1195,7 +1148,7 @@ make_fixnum (EMACS_INT n)
 /* Extract A's value as a signed integer.  Unlike XFIXNUM, this works
    on any Lisp object, although the resulting integer is useful only
    for things like hashing when A is not a fixnum.  */
-INLINE EMACS_INT
+KEY_OP_INLINE EMACS_INT
 XFIXNUM_RAW (Lisp_Object a)
 {
   EMACS_INT i = XLI (a);
@@ -1225,13 +1178,13 @@ make_ufixnum (EMACS_INT n)
 
 #endif /* ! USE_LSB_TAG */
 
-INLINE bool
+KEY_OP_INLINE bool
 (FIXNUMP) (Lisp_Object x)
 {
   return lisp_h_FIXNUMP (x);
 }
 
-INLINE EMACS_INT
+KEY_OP_INLINE EMACS_INT
 XFIXNUM (Lisp_Object a)
 {
   eassert (FIXNUMP (a));
@@ -1253,7 +1206,7 @@ XUFIXNUM (Lisp_Object a)
 }
 
 /* Return A's hash, which is in the range 0..INTMASK.  */
-INLINE EMACS_INT
+KEY_OP_INLINE EMACS_INT
 (XHASH) (Lisp_Object a)
 {
   return lisp_h_XHASH (a);
@@ -1270,7 +1223,7 @@ make_fixed_natnum (EMACS_INT n)
 
 /* Return true if X and Y are the same object.  */
 
-INLINE bool
+KEY_OP_INLINE bool
 (EQ) (Lisp_Object x, Lisp_Object y)
 {
   return lisp_h_EQ (x, y);
@@ -1399,13 +1352,13 @@ make_pointer_integer (void *p)
 };
 verify (GCALIGNED (struct Lisp_Cons));
 
-INLINE bool
+KEY_OP_INLINE bool
 (NILP) (Lisp_Object x)
 {
   return lisp_h_NILP (x);
 }
 
-INLINE bool
+KEY_OP_INLINE bool
 (CONSP) (Lisp_Object x)
 {
   return lisp_h_CONSP (x);
@@ -1417,7 +1370,7 @@ CHECK_CONS (Lisp_Object x)
   CHECK_TYPE (CONSP (x), Qconsp, x);
 }
 
-INLINE struct Lisp_Cons *
+KEY_OP_INLINE struct Lisp_Cons *
 (XCONS) (Lisp_Object a)
 {
   return lisp_h_XCONS (a);
@@ -1443,13 +1396,13 @@ xcdr_addr (Lisp_Object c)
 
 /* Use these from normal code.  */
 
-INLINE Lisp_Object
+KEY_OP_INLINE Lisp_Object
 (XCAR) (Lisp_Object c)
 {
   return lisp_h_XCAR (c);
 }
 
-INLINE Lisp_Object
+KEY_OP_INLINE Lisp_Object
 (XCDR) (Lisp_Object c)
 {
   return lisp_h_XCDR (c);
@@ -1652,7 +1605,7 @@ STRING_SET_CHARS (Lisp_Object string, ptrdiff_t newsize)
     Lisp_Object contents[FLEXIBLE_ARRAY_MEMBER];
   } GCALIGNED_STRUCT;
 
-INLINE bool
+KEY_OP_INLINE bool
 (VECTORLIKEP) (Lisp_Object x)
 {
   return lisp_h_VECTORLIKEP (x);
@@ -2174,7 +2127,7 @@ CHAR_TABLE_EXTRA_SLOTS (struct Lisp_Char_Table *ct)
 
 /* Value is name of symbol.  */
 
-INLINE Lisp_Object
+KEY_OP_INLINE Lisp_Object
 (SYMBOL_VAL) (struct Lisp_Symbol *sym)
 {
   return lisp_h_SYMBOL_VAL (sym);
@@ -2199,7 +2152,7 @@ SYMBOL_FWD (struct Lisp_Symbol *sym)
   return sym->u.s.val.fwd;
 }
 
-INLINE void
+KEY_OP_INLINE void
 (SET_SYMBOL_VAL) (struct Lisp_Symbol *sym, Lisp_Object v)
 {
   lisp_h_SET_SYMBOL_VAL (sym, v);
@@ -2250,7 +2203,7 @@ SYMBOL_INTERNED_IN_INITIAL_OBARRAY_P (Lisp_Object sym)
    i.e. it's a constant (e.g. nil, t, :keywords), or it has some
    watching functions.  */
 
-INLINE int
+KEY_OP_INLINE int
 (SYMBOL_TRAPPED_WRITE_P) (Lisp_Object sym)
 {
   return lisp_h_SYMBOL_TRAPPED_WRITE_P (sym);
@@ -2261,7 +2214,7 @@ SYMBOL_INTERNED_IN_INITIAL_OBARRAY_P (Lisp_Object sym)
    write to SYM, should also check whether there are any watching
    functions.  */
 
-INLINE int
+KEY_OP_INLINE int
 (SYMBOL_CONSTANT_P) (Lisp_Object sym)
 {
   return lisp_h_SYMBOL_CONSTANT_P (sym);
@@ -2806,7 +2759,7 @@ XBUFFER_OBJFWD (lispfwd a)
     } u;
   } GCALIGNED_STRUCT;
 
-INLINE bool
+KEY_OP_INLINE bool
 (FLOATP) (Lisp_Object x)
 {
   return lisp_h_FLOATP (x);
@@ -2970,7 +2923,7 @@ CHECK_LIST_END (Lisp_Object x, Lisp_Object y)
   CHECK_TYPE (NILP (x), Qlistp, y);
 }
 
-INLINE void
+KEY_OP_INLINE void
 (CHECK_FIXNUM) (Lisp_Object x)
 {
   lisp_h_CHECK_FIXNUM (x);
-- 
2.17.1