From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Andrea Corallo Newsgroups: gmane.emacs.devel Subject: Re: __builtin_expect Date: Thu, 28 Nov 2024 16:32:25 -0500 Message-ID: References: <-RjfswKL3C48Xb5i4UIUxCjnwRceVcJLWbxPqC118LgFluGk1pC9nPnX7uvy2pfLr9_tIT4c6LRO47ggVIB5_jw91LHlu4SrGGkJYOd4j8k=@protonmail.com> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="8511"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: Emacs Devel To: Pip Cet Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Thu Nov 28 22:33:21 2024 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1tGm8W-0001ze-KK for ged-emacs-devel@m.gmane-mx.org; Thu, 28 Nov 2024 22:33:20 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1tGm7g-0006QY-Vm; Thu, 28 Nov 2024 16:32:29 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1tGm7f-0006QP-FF for emacs-devel@gnu.org; Thu, 28 Nov 2024 16:32:27 -0500 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1tGm7e-0006zB-02; Thu, 28 Nov 2024 16:32:26 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=MIME-Version:Date:References:In-Reply-To:Subject:To: From; bh=QOIeaaB9M2t9Xn6/gnz5PkHYdXi+InkRzwEXAWvMyyc=; b=RX8UmzBGcZxevijAZprm eh7R3y9rcaSlxYHsh31IsSe6K3jqIWvOIjM64c9R4bYqFRDtiLINW7eAm7NyIEIOtnwAOj7E2Nnp2 5EpumymCwHd23lD6fMABvLHufPv20TFVQbExomFg29ZuNbAqPti9ffPPyOK6OJQ2c7+UYkT0h9cUd XB05Kgwc9h9XrnyOWIWs7uxwjaBm1XTBwcRkqosS+olt+4nyY4rwE8Wf4M9CjPqhs+Jo/WIMB7hw3 5fbyTf0/xUksTckTAHUFRbZd7ucvMBmPBI6+XC8a5iIBE8qN+p9CANyjJHWTllPWytOojT2DSP/r7 WDrJ13j5fhS3LA==; Original-Received: from acorallo by fencepost.gnu.org with local (Exim 4.90_1) (envelope-from ) id 1tGm7d-0007Vw-KX; Thu, 28 Nov 2024 16:32:25 -0500 In-Reply-To: <-RjfswKL3C48Xb5i4UIUxCjnwRceVcJLWbxPqC118LgFluGk1pC9nPnX7uvy2pfLr9_tIT4c6LRO47ggVIB5_jw91LHlu4SrGGkJYOd4j8k=@protonmail.com> (Pip Cet's message of "Thu, 28 Nov 2024 20:29:53 +0000") X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.devel:325835 Archived-At: Pip Cet writes: > On Thursday, November 28th, 2024 at 16:12, Andrea Corallo wrote: >> Pip Cet pipcet@protonmail.com writes: >> >> > 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. >> >> If you feel this need to be fixed could you please submit a patch? > > To enable __builtin_expect, or to remove it? __builtin_expect it's already enabled, I'm talking about builtin-expect.m4 following your comment on it. > I think for now we should do the latter. Noted >> > 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. >> >> >> It was already in use in the Emacs code-base before my commit, > > You mean the two usages in src/android.c? grep is your friend > We don't support arbitrary > compilers for Android code, only clang (and maybe GCC again if that > has been fixed), so what? > so that's hardly precedent to build on when modifying > lisp.h. > >> and I >> don't see any specific reason why we should not use it where deemed to >> be useful. > > At the very least, we'd need to establish it actually is useful. Going by intuition isn't the right answer here. I didn't use my intuition, it generates better code and IME faster code. >> I'm not a fan at all of the spread use of manually annotated >> branches, but this case is pretty obvious and important at the same >> time. > > It's not obvious to me, sorry. I'm sorry as well. Hope my next point clarifies better. >> > > > 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. >> >> I don't understand sorry, no compiler is reading commit messages, but > > You said that your assumption holds "when we are not compiling", but your code change applies to the other case, too. Indeed, but Emacs is rarely compiling, Emacs is not primary a compiler, and I hope it's clear that we want to optimize for its everyday typical use. Given in that case 'syms_with_pos_enabled' is simply false all the time, this is just the way to express that to the compiler so it can generate the right code. Actually AFAIU/R the presence of 'syms_with_pos_enabled' there *is* an optimization to mitigate the performance impact of this feature on the most common Emacs usecase (the one my patch optimized further).