Re: Emacs build fails [MSYS2/MINGW64]

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

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

On 4/29/19 7:53 AM, Eli Zaretskii wrote:
> see, for example, the comments by Florian
> Weimer in this GCC bug report:
>
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88793

Those comments don't invalidate all uses of __attribute__ ((cold)) in
Emacs. Florian says that in cold functions on some platforms, strlen on
large (5000-byte) strings can be as much as 60 times slower due to
questionable optimizations in GCC that are currently being rethought
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=88809#c5>. However, if the
functions currently marked 'cold' in Emacs are called so rarely (at
least on such large strings) that the performance cost (factor of 60 for
rarely-used code) is outweighed by the performance benefit (small factor
of improvement in commonly-used code), then from a performance viewpoint
it's still preferable to mark these functions 'cold'.


> the GCC manual indicates that marking a function as 'cold'
> causes that function to be optimized for size, and we already know
> that GCC 8.x has at least one bug with -Os used together with -O2,

The GCC bug report <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90020>
says that the bug can also occur with -O2, and can occur without
__attribute__ ((cold)). So it's not as simple as saying "__attribute__
((cold)) is buggy".


> I think we have way too many functions marked with this
> attribute.  Functions like wrong_type_argument or Fthrow or
> Fabort_recursive_edit or Fsignal or error or Ftop_level -- do they
> really fit the below description?

It should be purely a performance issue.

For example, on my platform (x86-64, GCC 8.3.1, -O2, Fedora 29) after
inlining, Emacs has 1066 static calls to wrong_type_argument, and with
__attribute__ ((cold)) all these calling sites can be optimized. In
bytecode.c this causes GCC to move wrong_type_argument calls to a cold
section of the text, which means the bytecode interpreter should run
faster for all the usual branch-prediction and icache reasons.

I measured the effect of __attribute__ ((cold)) (including both costs
and benefits) as improving overall performance of a big benchmark ('make
compile-always') by 1.3% on my platform. This test included all the
roughly 50 functions currently marked as cold in Emacs. Although I can
imagine that removing __attribute__ ((cold)) from some of these
functions would not hurt overall performance significantly, I didn't
think it worth the time to investigate which ones. That is, I didn't
investigate marking a smaller set of functions. If someone wants to do
that work then I expect that we should be able to remove markings from
some of these functions without affecting performance significantly.


> if we leave it in the
> sources, we should disable it for MinGW, it seems.

Yes, that sounds like a good idea. I installed the attached patch. It
would be good to know what the actual bug is (it's possible that it's
not a compiler bug at all, but is a portability bug in Emacs), but
that's less urgent.


[-- Attachment #2: 0001-Disable-__attribute__-cold-on-MinGW.patch --]
[-- Type: text/x-patch, Size: 1374 bytes --]

From aaf33358e79e0210f2e8dab17fd8a33ac97383b9 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 29 Apr 2019 12:27:04 -0700
Subject: [PATCH] Disable __attribute__ ((cold)) on MinGW

* src/conf_post.h (ATTRIBUTE_COLD) [__MINGW32__]:
Define to empty on this platform.
---
 src/conf_post.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/conf_post.h b/src/conf_post.h
index 7699d2c95b..4af1ba9331 100644
--- a/src/conf_post.h
+++ b/src/conf_post.h
@@ -59,7 +59,7 @@ along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
    into the same 1-, 2-, or 4-byte allocation unit in the MinGW
    builds.  It was also needed to port to pre-C99 compilers, although
    we don't care about that any more.  */
-#if NS_IMPL_GNUSTEP || defined(__MINGW32__)
+#if NS_IMPL_GNUSTEP || defined __MINGW32__
 typedef unsigned int bool_bf;
 #else
 typedef bool bool_bf;
@@ -225,7 +225,9 @@ extern void _DebPrint (const char *fmt, ...);
 extern char *emacs_getenv_TZ (void);
 extern int emacs_setenv_TZ (char const *);
 
-#if __has_attribute (cold)
+/* Avoid __attribute__ ((cold)) on MinGW; see thread starting at
+   <https://lists.gnu.org/r/emacs-devel/2019-04/msg01152.html>. */
+#if __has_attribute (cold) && !defined __MINGW32__
 # define ATTRIBUTE_COLD __attribute__ ((cold))
 #else
 # define ATTRIBUTE_COLD
-- 
2.20.1