Message ID | BYAPR08MB4232749003F118FA852BA0E5B6730@BYAPR08MB4232.namprd08.prod.outlook.com |
---|---|
State | Rejected |
Headers |
Return-Path: <libc-alpha-bounces@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id A95903857C67; Tue, 28 Jul 2020 23:10:10 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org A95903857C67 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1595977810; bh=BC3DMV5NjpbZ3+1lhXjBE1FCEtznyOckXFJdCH8cgxw=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=UEQwHId1Wyb2iviTnm0+wfRWNe5xBd07TfL7KKDc3mbdJEiSQMYR6hWvqJlEIpxPS /k1BRueIO1KNRnWiFQK1P5cabdGZvm0QtMSOYjiB3vW3r3cxkbPbzaDCeaJ3R66AvW DY8jpaLanp7qd5hbQivRz4vl6DB1EUeCEEpzXhdA= X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from NAM10-BN7-obe.outbound.protection.outlook.com (mail-bn7nam10olkn2027.outbound.protection.outlook.com [40.92.40.27]) by sourceware.org (Postfix) with ESMTPS id 2E5613857C67 for <libc-alpha@sourceware.org>; Tue, 28 Jul 2020 23:10:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 2E5613857C67 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=NCcbCaBvqcZZ6OxdfJJbjJguSreqphsVP7w9z5WOGceUeUiBnGxuF+PIBvxF89TXjgKIm/VgJMDuVD4Qv8onaiWPPYJeAUclrR2+1K6DnjlOcMP/kv/Aw3LciDtSfIuwwrF5BQPENoBoDyz8rYcYlODDsZLlOUw+jyj4clx0sOriBtePFpDvUxgIPX2EvhE97Jfi0GZPDKdw9LnVCNgx9fAFrRhROK16MpTpm1YzGClTgLEAx7r2vPtOhqWnpRgThUr2i4wel6QWfEhiopfYmo6Uq1aSaU+CIqw3k8sOk5vhEqK0T5XUzq5JaJKidrp3R6sRPCNDluLkr9BhUnDhww== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=BC3DMV5NjpbZ3+1lhXjBE1FCEtznyOckXFJdCH8cgxw=; b=XDAzyw1tzikt//3GY4sqJ+LwsgB5DU/IYohqdz7Zw2NmsxW6RFkHYmFvdb4473O+0mCjPb+dDXgLP5WlO9xyXGdXKbvbByckXwy8dI2Q455ld5t3zc4kncQ2IDuXlFkhhMWNe1FymimHONHGkJuJ5dWO33YUgR9Qr21NHaC248vDlqT6VlWcW5mDJQ1PhTXPCulkY8AacGo+FbH9UTH3YGhvBeNtYahTXriNCiYoA/8vPr1j3G56pkAgRR9/7o6cGH94aeD2isrB8C9yeRTLRcRUEKNPzmh1+w7U9Fxj7ktmy6ACvBAMFUgJhIw2PGC0BpHyrAN7QOmRD7W47LctJQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none Received: from BN7NAM10FT011.eop-nam10.prod.protection.outlook.com (2a01:111:e400:7e8f::50) by BN7NAM10HT010.eop-nam10.prod.protection.outlook.com (2a01:111:e400:7e8f::394) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3216.10; Tue, 28 Jul 2020 23:10:06 +0000 Received: from BYAPR08MB4232.namprd08.prod.outlook.com (2a01:111:e400:7e8f::4d) by BN7NAM10FT011.mail.protection.outlook.com (2a01:111:e400:7e8f::250) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3216.10 via Frontend Transport; Tue, 28 Jul 2020 23:10:06 +0000 Received: from BYAPR08MB4232.namprd08.prod.outlook.com ([fe80::b4c5:300c:aad4:1811]) by BYAPR08MB4232.namprd08.prod.outlook.com ([fe80::b4c5:300c:aad4:1811%7]) with mapi id 15.20.3216.034; Tue, 28 Jul 2020 23:10:06 +0000 To: "libc-alpha@sourceware.org" <libc-alpha@sourceware.org>, Sebastian Pop <sebpop@gmail.com> Subject: Add builtin likely to assertions Thread-Topic: Add builtin likely to assertions Thread-Index: AQHWZTPkR5u7CLnUpEquWnP/n4D7JQ== Date: Tue, 28 Jul 2020 23:10:06 +0000 Message-ID: <BYAPR08MB4232749003F118FA852BA0E5B6730@BYAPR08MB4232.namprd08.prod.outlook.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-incomingtopheadermarker: OriginalChecksum:475F1C9AC06A25B4D6782E391CE52F4B659182C2E576DD97EEA9B28C7E8EE206; UpperCasedChecksum:1E7D2CA1D87A0727E7708F1E96528ABE533AE21CAF5EF5CE689FC9C8CFF9A509; SizeAsReceived:6818; Count:41 x-tmn: [HjGA41bcuqGATwVMWL/7gxOvo35vQCIC4Q9DYARUO8JBaoQW9CSddXlPpqsSdm5d] x-ms-publictraffictype: Email x-incomingheadercount: 41 x-eopattributedmessage: 0 x-ms-office365-filtering-correlation-id: 2b7c24b4-d51d-4e9b-f134-08d8334b5dc7 x-ms-traffictypediagnostic: BN7NAM10HT010: x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: CjrlXznCvNpjHj7s1FGT1zjF0xl1+/9vBecMRM+gPFFXSoJQEKK2PIeQrDJAt7qWGddUbPv3cpt9QmVT0SNPSGKz/BjgGxEWXgF/umxNJIS4lsgAPSQ2RdNxi32caFSExVqwKRA8pmKnkrwVn/N/7N/BFoaB7ZWnnz7zqS9rbB0omFzbm4DidcchrGs9agi0AnLYRO2pEza8qg5LGbg3XA== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:0; SRV:; IPV:NLI; SFV:NSPM; H:BYAPR08MB4232.namprd08.prod.outlook.com; PTR:; CAT:NONE; SFTY:; SFS:; DIR:OUT; SFP:1901; x-ms-exchange-antispam-messagedata: GG/6+TvR7YrxkJMJUEw6pDcwYX0ihxd9tj5ghNptBQ8zOsOORXAi9gcytkqnZ1rgMcC0ZiakZ4VZSrhUxa1FsvPSk9D4TRpTZLJarRoElpn8pDh+WY7HdsziW8CMI0hvYrWiQSnQN5WMNggdhEGQPPfsivOkHt6Rf0pPFy15bca6nIO9YYN2LEuxksUe3dclBUu0p/ix1Xor8TvZbJUZNw== x-ms-exchange-transport-forked: True Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-AuthSource: BN7NAM10FT011.eop-nam10.prod.protection.outlook.com X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-CrossTenant-Network-Message-Id: 2b7c24b4-d51d-4e9b-f134-08d8334b5dc7 X-MS-Exchange-CrossTenant-rms-persistedconsumerorg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Jul 2020 23:10:06.1123 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Internet X-MS-Exchange-CrossTenant-id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN7NAM10HT010 X-Spam-Status: No, score=-10.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_PASS, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list <libc-alpha.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> From: Aditya K via Libc-alpha <libc-alpha@sourceware.org> Reply-To: Aditya K <hiraditya@msn.com> Errors-To: libc-alpha-bounces@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces@sourceware.org> |
Series |
Add builtin likely to assertions
|
|
Commit Message
Aditya K
July 28, 2020, 11:10 p.m. UTC
The true part of assertions are more likely to get executed so adding likely attribute to the true part will help with better basic block layout. ``` commit fe153e42a57550f9db667b130e3d00a178c27199 (HEAD -> master) Author: AK <1894981+hiraditya@users.noreply.github.com> Date: Tue Jul 28 15:59:08 2020 -0700 Add builtin likely to assertions ``` Thanks -Aditya
Comments
On Tue, Jul 28, 2020 at 7:10 PM Aditya K via Libc-alpha <libc-alpha@sourceware.org> wrote: > The true part of assertions are more likely to get executed so adding likely attribute > to the true part will help with better basic block layout. Do you actually observe code layout changes with this patch, and if so, with which compiler? This should already be handled by the heuristic that any code path leading to a noreturn function is unlikely. zw
On Tue, Jul 28, 2020 at 4:10 PM Aditya K via Libc-alpha <libc-alpha@sourceware.org> wrote: > > The true part of assertions are more likely to get executed so adding likely attribute > to the true part will help with better basic block layout. This is not needed for GCC to do the correct thing as __assert_fail is marked as noreturn and GCC heuristics for branch prediction already predicts it correctly and has done since at least 4.4 (or even beforehand). Can you explain more on which compiler this helps with? Thanks, Andrew Pinski > > ``` > commit fe153e42a57550f9db667b130e3d00a178c27199 (HEAD -> master) > Author: AK <1894981+hiraditya@users.noreply.github.com> > Date: Tue Jul 28 15:59:08 2020 -0700 > > Add builtin likely to assertions > > diff --git a/assert/assert.h b/assert/assert.h > index 266ee92e..2933ff60 100644 > --- a/assert/assert.h > +++ b/assert/assert.h > @@ -87,7 +87,7 @@ __END_DECLS > suppress warnings we'd expect to be detected by gcc's -Wparentheses. */ > # if defined __cplusplus > # define assert(expr) \ > - (static_cast <bool> (expr) \ > + (__builtin_expect(static_cast <bool> (expr), 1) \ > ? void (0) \ > : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION)) > # elif !defined __GNUC__ || defined __STRICT_ANSI__ > @@ -103,7 +103,7 @@ __END_DECLS > suppress the evaluation of variable length arrays. */ > # define assert(expr) \ > ((void) sizeof ((expr) ? 1 : 0), __extension__ ({ \ > - if (expr) \ > + if (__builtin_expect(expr, 1)) \ > ; /* empty */ \ > else \ > __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION); \ > > ``` > > Thanks > -Aditya
> This is not needed for GCC to do the correct thing as __assert_fail is > marked as noreturn and GCC heuristics for branch prediction already > predicts it correctly and has done since at least 4.4 (or even > beforehand). > Can you explain more on which compiler this helps with? I saw this pattern in 'MacOSX.sdk/usr/include/assert.h', so added here. I don't have any test case to show a different layout with this patch. -Aditya
On 7/29/20 2:05 AM, Andrew Pinski via Libc-alpha wrote: > This is not needed for GCC to do the correct thing as __assert_fail is > marked as noreturn and GCC heuristics for branch prediction already > predicts it correctly and has done since at least 4.4 (or even > beforehand). Exactly, one can verify that by looking at guessed branch probabilities: $ cat /tmp/tc.c int main(int argc) { if (argc == 123) __builtin_abort (); else return 0; } $ gcc /tmp/tc.c -fdump-tree-optimized=/dev/stdout -O2 ... if (argc_1(D) == 123) goto <bb 3>; [0.00%] else goto <bb 4>; [100.00%] One can see more details in -fdump-tree-profile_estimate-details dump file. Martin
On Tue, Jul 28, 2020 at 10:19 PM Aditya K via Libc-alpha <libc-alpha@sourceware.org> wrote: > > > This is not needed for GCC to do the correct thing as __assert_fail is > > marked as noreturn and GCC heuristics for branch prediction already > > predicts it correctly and has done since at least 4.4 (or even > > beforehand). > > Can you explain more on which compiler this helps with? > > I saw this pattern in 'MacOSX.sdk/usr/include/assert.h', so added here. I don't have any test case to show a different layout with this patch. That suggests that some versions of clang might not have the "noreturn paths are cold" heuristic. Can you test whether anything changes with your patch on Linux with clang? Try several different versions if possible. I want to be clear that we're _not_ rejecting your patch out of hand. It's just that we don't want to add annotations that don't make any difference for code generation. If there is a compiler that's commonly used with glibc, that either doesn't have the "noreturn paths are cold" heuristic or doesn't understand that __assert_fail etc never return, then we would take changes to assert.h to improve code generation with that compiler. zw
On Wed, 29 Jul 2020, Zack Weinberg wrote: > I want to be clear that we're _not_ rejecting your patch out of hand. > It's just that we don't want to add annotations that don't make any > difference for code generation. If there is a compiler that's > commonly used with glibc, that either doesn't have the "noreturn paths > are cold" heuristic or doesn't understand that __assert_fail etc never > return, then we would take changes to assert.h to improve code > generation with that compiler. Note that in any case where annotations turn out to be useful, we should use __glibc_likely / __glibc_unlikely (for any true / false conditional) rather than using __builtin_expect expect directly.
So I tried with fairly latest clang (trunk June) and removing (noreturn) from assert_fail does change the basic block layout at -O2/-O3. So it seems this change may not be needed. I tried compiling 'https://github.com/llvm/llvm-project/blob/master/clang/test/Analysis/null-deref-ps.c' which is self-contained with a definition of the assert macro. A reduced test example is here: https://godbolt.org/z/WeTcMj -Aditya From: Joseph Myers <joseph@codesourcery.com> Sent: Wednesday, July 29, 2020 10:00 AM To: Zack Weinberg <zackw@panix.com> Cc: Aditya K <hiraditya@msn.com>; libc-alpha@sourceware.org <libc-alpha@sourceware.org>; Sebastian Pop <sebpop@gmail.com> Subject: Re: Add builtin likely to assertions On Wed, 29 Jul 2020, Zack Weinberg wrote: > I want to be clear that we're _not_ rejecting your patch out of hand. > It's just that we don't want to add annotations that don't make any > difference for code generation. If there is a compiler that's > commonly used with glibc, that either doesn't have the "noreturn paths > are cold" heuristic or doesn't understand that __assert_fail etc never > return, then we would take changes to assert.h to improve code > generation with that compiler. Note that in any case where annotations turn out to be useful, we should use __glibc_likely / __glibc_unlikely (for any true / false conditional) rather than using __builtin_expect expect directly.
diff --git a/assert/assert.h b/assert/assert.h index 266ee92e..2933ff60 100644 --- a/assert/assert.h +++ b/assert/assert.h @@ -87,7 +87,7 @@ __END_DECLS suppress warnings we'd expect to be detected by gcc's -Wparentheses. */ # if defined __cplusplus # define assert(expr) \ - (static_cast <bool> (expr) \ + (__builtin_expect(static_cast <bool> (expr), 1) \ ? void (0) \ : __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION)) # elif !defined __GNUC__ || defined __STRICT_ANSI__ @@ -103,7 +103,7 @@ __END_DECLS suppress the evaluation of variable length arrays. */ # define assert(expr) \ ((void) sizeof ((expr) ? 1 : 0), __extension__ ({ \ - if (expr) \ + if (__builtin_expect(expr, 1)) \ ; /* empty */ \ else \ __assert_fail (#expr, __FILE__, __LINE__, __ASSERT_FUNCTION); \