Message ID | MW2PR2101MB177075A47AD1E30F66CED4FE91C59@MW2PR2101MB1770.namprd21.prod.outlook.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.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 2DCE83848581 for <patchwork@sourceware.org>; Fri, 6 May 2022 20:32:49 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 2DCE83848581 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1651869169; bh=mqoEwy7i0RlmnVXuQ79BGPFgrugdGlj22ahdyK7yjjs=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Post: List-Help:List-Subscribe:From:Reply-To:From; b=rcsmzRk7zt+Y5IiJoGGnj11b0KdXVdUlEi2qM/e6Z2mshDrMzvRzMAV8pAPIhOJZF JKzlYgQPOuPk/yh49tAnoTmH0KkOa7X8qYsAjK/hjMgxWM1Uuhn68o2Zm7ns2G3rvq lmsuKmIZKdhJeYeCRuhie77H2IkxXvRT9r5YBzqY= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from na01-obe.outbound.protection.outlook.com (mail-centralusazon11021014.outbound.protection.outlook.com [52.101.62.14]) by sourceware.org (Postfix) with ESMTPS id EF9333839428 for <gcc-patches@gcc.gnu.org>; Fri, 6 May 2022 20:31:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org EF9333839428 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=BhK7u5nyuKTNVwAXoCplpk9Fj08iE/vecwns+1FAWf6gPbZFinSf8fES1cGjnNDEmJ96M/rgTVW302qnO8dRXSIXtbSQvuyf29cBqJQdDMuJeBQ9mbDJQgRgm5r4TIar9wNHLbzZ5nhY64ZfrKyFeGHwGzbLr9bYA5uddvqTxLMTGfQL+pTZrXRQGkI9SLfo3t7o76bj0RubotmHeEEz5ME7Oqd+imkbdGbgdSiLMO2pgBHXiug4P3T937yp+0ZYhYExKMIO51tMzGRu1X7E/JdaXd726n/TWZLRyw7btjohShSrIJnl6kObN34M4FTekdpR644ENUE2spyViSbYhw== 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-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=mqoEwy7i0RlmnVXuQ79BGPFgrugdGlj22ahdyK7yjjs=; b=Mmr8bcdgzEaO02ugXzDTW9nF+5EzZh16DGx4Z9mBbnlgTmZkjU4oPg/rR8w3lq6Ngpr1PwGOnxYA+eFtBCslHN8no8Gt/vP+8DRfc1TUP+3FR+h57WHgBX8xbMv8OIaoEznPikiuUYNjkbYBIKlv4Dwf6VSKdlyDNeA0mc6UK8zkiw8zYfq9a1ZtjoF4weyPTlHxEZnTkRBO1NpylsOCOTWGCqJ741ZDCj6A2G2fLnfOQgqb5yy+PWRX4n3ChcF3vxQCoGUgnYeXPobfoFPvZIVsXFDI6/vbsXgGhoFmEPzT8hiA3ysVGHJZ4Wjn2ltOGGchTEXLi6TB3GP0vWId7w== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=microsoft.com; dmarc=pass action=none header.from=microsoft.com; dkim=pass header.d=microsoft.com; arc=none Received: from MW2PR2101MB1770.namprd21.prod.outlook.com (2603:10b6:302:8::14) by DM6PR21MB1739.namprd21.prod.outlook.com (2603:10b6:5:cb::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5250.11; Fri, 6 May 2022 20:31:00 +0000 Received: from MW2PR2101MB1770.namprd21.prod.outlook.com ([fe80::48c6:abb2:67e0:d6dc]) by MW2PR2101MB1770.namprd21.prod.outlook.com ([fe80::48c6:abb2:67e0:d6dc%5]) with mapi id 15.20.5250.011; Fri, 6 May 2022 20:31:00 +0000 To: "gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org> Subject: [PATCH] Guard against applying scale with 0 denominator Thread-Topic: [PATCH] Guard against applying scale with 0 denominator Thread-Index: AdhhiAXile7dmqnsQc+W2Yc8zXpTkA== Date: Fri, 6 May 2022 20:31:00 +0000 Message-ID: <MW2PR2101MB177075A47AD1E30F66CED4FE91C59@MW2PR2101MB1770.namprd21.prod.outlook.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ActionId=a44e47aa-c5eb-46a1-a46a-e3bc32b98817; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=true; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Standard; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Name=Internal; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2022-05-06T20:28:59Z; MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 3d0ae9b7-7312-4163-f8db-08da2f9f5593 x-ms-traffictypediagnostic: DM6PR21MB1739:EE_ x-microsoft-antispam-prvs: <DM6PR21MB17397D51E921CC6CB1BAC62991C59@DM6PR21MB1739.namprd21.prod.outlook.com> x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: C2ZnTe+DqgZd/yEr32MslYG2yQ+N0NBt22M7o59/AlIRnW6ylh6ZL4tm3LhcKlPlSwbqE2oo1XSt38cqo33nRlICJIfLnM9YQf9EWbWE7V1nCokcidYozPMi1j+twoO5s69GZfqCIm7S3CIFjjHD9aheZPo7fDGCFXBfku+wEMVEcJ2/3NB/Fqz/JzsQf5/nIvMcoNlp3i9LuTbx5gP0rV911eRiv55gqL0xkK3vuxa7PNcYzemtf4Mh011yDBgXP6CcuT+72PknTHqxNpyZGmO+uuPChpfWFHmeTSugV905f02z7JkxvS4lshkKZ6MG5+jIKpySMLqJpuV9cXgNAbmNg4ZhlwmDj582YSgfyciXEyYyenfu4+KAA6A5mKI7abuuAB0jdaDdTpF8SJHX5Zn5NXbVnyCHecDOm4g/D7jh3SS892PMcXQNGABo2V84CBy6pJ15i/9xXo7oteWwQmQlIV18jE8bTQARnFyjomPV6MyeMIwH8nbYtN92U7bnwmojih83NalYq7AqbxpcV7lSdEjhhKAx1VNflEvG6J+pc9pK+X8COOwyVXvREvoiswiQ6odxgRtif1iKtZbPHuRRiNEwurQfswPkwPhw1om7jdVHla35eKoq74H+DP+VKFUi9FVyRFMvh08pEoJ+LKOEyDJGz0h1DSRtj16CIyGtJdnH6IsbuEKipe3EkbtA18JjqLXxQzvxvQAu9dpUuiWXhqWYAIySn8xM/67Z9RZ3Ra6dx2YxMlnWciZH74C6 x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:MW2PR2101MB1770.namprd21.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230001)(4636009)(366004)(451199009)(76116006)(66946007)(66556008)(66476007)(64756008)(66446008)(2906002)(82950400001)(8676002)(316002)(186003)(86362001)(9686003)(8990500004)(55016003)(38100700002)(38070700005)(33656002)(7696005)(71200400001)(122000001)(82960400001)(8936002)(52536014)(6506007)(83380400001)(508600001)(6916009)(10290500003)(5660300002); DIR:OUT; SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 2 x-ms-exchange-antispam-messagedata-0: MO4EbzdaiIToHsiL7V95RGd6PFf6xqqQ2tvLbklAnaXo6nYHKv2KE//Hm+1CPsApr3hr6f72BxumZW56wkbwZnqL4LuMvCTMWnAq3IEr9FfVyW9xh3aiRiy6ywncpcFILLRXwG713GdgRjfbMnGmOM+5CCyL/r++W1Z1UOCJZjnekDBDi3fbd5DfBG8tie7K5WksY2Q7wO9WI07uCRWwAkUR/6ZxN7O/slAqGI02uzJItqU9EQ10h17N6EASuj6RaML9SpoVfxy9XUtDIN+2Crm6zq3UvOPlVqngmM8iMgcRg8kbbV9JJhSn3vdhJ5pYSFVV1ECeiMXmd3iOIY1e9O+QJ1CWcEWx1SaP9N+BrDYgUdrz7WsEqTaX+6aFVtUxePrmsAJF4TJ1t+hFPxJOzERZxF9zqaNf4L3LnMPYHh7WiikkhglE7cMhIIPL1/4L449Hk1Mg1pIBeVTWx8An6iVSLYYIDff+KFkOuTaC0lWI9PUKLVbYJhjAHoH4kFDShAKzSsFik9dVCTM0oUgWVDT9HMPVP1qecYDHnWO6qIf7nk+Zs1+1qArd+NS9vm2Qi7DoCvfjN8rzjT+YFdxtGCMAzTFimjsTZAi4En2iN/T0UYrepwLiwojfo7j3n2tsmOcnslVAGfElZpC9rGDoEw3S3bShVVSllpXdG+s0DaGSQr9zljZdN5/cOtOIujUxvpUU0MrIaIEjRCZtMQKtxV2DtT5CUaoYtv8JWqDRtk79HJtIeJQpf/jc4MRYSYbOxB431m4k9t0BJKhzsQxLkzWFvx88zGM+tVlHm9LPtQe4hPbbInG+16f8MGM/hTfK+vvnt180ncRMnpKPePobUpB9joQB40v4EMXGPBLZhy65G/BZPP+B5LOwAcEUOM7It9JGoAnyOCJuRKIo2ASt5llGXgp9SB8FpO0R+nNejr0PnG/RvfJm0yv2Xr9MNBSbhtKydzAlgxxFzk358QgWvYkKzG/cQN5kL2EJSMkp3N3w9sigxF2Ab4DweefpB7Xe8037eZ1P/ROrs6x25IObOqabCUaZ2qG9acF3tXm9pWqtK0vCHvN0cmd6g627pYtvO0PJvV2Yelnk0DcEE+8Q4CqwWW6zJzs5w2lguPfwsf0214FjCaaJvjnpAHLhmr913Jh+JjM4bcLGznQhjTmRpmSYbTLAQVBb/RN32DLgS6gZ/JYpM5dmnoRDL5pyQEuYxuES64ilnlPsTRzZqpH+mlaLWnz+ijRSgNfubOBWDZHWZXocDsFrR35nDuH1G0opMH1alr1ftsccvwV8TFkW6bPhfNq4BR8JusZEHOmUvFWH6fCRpUI7qA4Ig/dbOZ3p9M14IIERSzanY8Diz0hYc5rJxbsq1TjkQovL1BOO6K9Otty7XDZWqU26gKFCMV5S1l97ZT6ERu3kYoA2W5glqnoTlYEK6LiVnny4zRsCrCfUjQUYb3jM1ofHSPkUAoYwyrVYeNkIVi8yPF2uGPcju7xPmacQAzXQSEdgmxWgA4SKV3sSru+5XmwO4NNFCBMXPVebRBBWPuuwYFf8trLTSSl+PPY8blJkiJt958khjtm4kqPhPPzymqrgMNj+3ZxcA42GQ929OlV+a8fRNEvxZ9AdetH/75/UplSqNa4PVAunp9HmyXtI8ksQE9zXTT8eUBQJQ7lNXUGZ8A9sUXCwyYgI13JhoTxbtS4DHAylOZ0OB/G3cqwnywMH5zVjw0lFroMjPBdDLFhIlMEYJdkfa7ZRmENksfkyZNn6WkGMNDbwp1YNerlLI6mNIjWW0LwzBKRY0nIp x-ms-exchange-antispam-messagedata-1: U2Ssymbzhz0ZaQ== Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: MW2PR2101MB1770.namprd21.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 3d0ae9b7-7312-4163-f8db-08da2f9f5593 X-MS-Exchange-CrossTenant-originalarrivaltime: 06 May 2022 20:31:00.8112 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: cRtNCZBaQ0/YKwS+PaIZ2UXgzOzlWIus9AoFcRM2G1UbKdbw/1YX+3O0VOgihyWfjvKAexaFu7ZnPYcxBqSflQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR21MB1739 X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FORGED_SPF_HELO, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_NONE, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> From: Eugene Rozenfeld via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
Guard against applying scale with 0 denominator
|
|
Commit Message
Eugene Rozenfeld
May 6, 2022, 8:31 p.m. UTC
Calling count.apply_scale with a 0 denominator causes an assert. This change guards against that. Tested on x86_64-pc-linux-gnu. gcc/ChangeLog: * tree-loop-vect-manip.cc (vect_do_peeling): Guard against applying scale with 0 denominator. --- gcc/tree-vect-loop-manip.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) -- 2.25.1
Comments
On Fri, May 6, 2022 at 10:32 PM Eugene Rozenfeld via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Calling count.apply_scale with a 0 denominator causes an assert. > This change guards against that. > > Tested on x86_64-pc-linux-gnu. > > gcc/ChangeLog: > * tree-loop-vect-manip.cc (vect_do_peeling): Guard against applying scale with 0 denominator. > --- > gcc/tree-vect-loop-manip.cc | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc > index 1d4337eb261..db54ae69e45 100644 > --- a/gcc/tree-vect-loop-manip.cc > +++ b/gcc/tree-vect-loop-manip.cc > @@ -2989,10 +2989,11 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree niters, tree nitersm1, > get lost if we scale down to 0. */ > basic_block *bbs = get_loop_body (epilog); > for (unsigned int i = 0; i < epilog->num_nodes; i++) > - bbs[i]->count = bbs[i]->count.apply_scale > - (bbs[i]->count, > - bbs[i]->count.apply_probability > - (prob_vector)); > + if (bbs[i]->count.nonzero_p ()) > + bbs[i]->count = bbs[i]->count.apply_scale > + (bbs[i]->count, > + bbs[i]->count.apply_probability > + (prob_vector)); So exactly what the FIXME in the comment above says happens. It might be better to save/restore the old counts if the intent is to get them back. I'm not exactly sure where the other scaling happens though. Richard. > free (bbs); > } > > -- > 2.25.1
In my case this is not exactly what the FIXME in the comment above says. The count is 0 even before the initial scaling happens. I hit this case with some changes I'm working on to enable per-instruction discriminators for AutoFDO. I looked into saving/restoring the old counts but it would be awkward to do. The initial scaling happens here: if (skip_vector) { split_edge (loop_preheader_edge (loop)); /* Due to the order in which we peel prolog and epilog, we first propagate probability to the whole loop. The purpose is to avoid adjusting probabilities of both prolog and vector loops separately. Note in this case, the probability of epilog loop needs to be scaled back later. */ basic_block bb_before_loop = loop_preheader_edge (loop)->src; if (prob_vector.initialized_p ()) { scale_bbs_frequencies (&bb_before_loop, 1, prob_vector); scale_loop_profile (loop, prob_vector, 0); } } The scaling happens before we do the cloning for the epilog so to save/restore the counts we would need to maintain a mapping between the original basic blocks and the cloned basic blocks in the epilog. I'd like to get my simple fix in since it makes things better even if it doesn't address the issue mentioned In the FIXME. -----Original Message----- From: Richard Biener <richard.guenther@gmail.com> Sent: Monday, May 09, 2022 12:42 AM To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com>; Jan Hubicka <hubicka@ucw.cz> Cc: gcc-patches@gcc.gnu.org Subject: [EXTERNAL] Re: [PATCH] Guard against applying scale with 0 denominator On Fri, May 6, 2022 at 10:32 PM Eugene Rozenfeld via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > Calling count.apply_scale with a 0 denominator causes an assert. > This change guards against that. > > Tested on x86_64-pc-linux-gnu. > > gcc/ChangeLog: > * tree-loop-vect-manip.cc (vect_do_peeling): Guard against applying scale with 0 denominator. > --- > gcc/tree-vect-loop-manip.cc | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc > index 1d4337eb261..db54ae69e45 100644 > --- a/gcc/tree-vect-loop-manip.cc > +++ b/gcc/tree-vect-loop-manip.cc > @@ -2989,10 +2989,11 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree niters, tree nitersm1, > get lost if we scale down to 0. */ > basic_block *bbs = get_loop_body (epilog); > for (unsigned int i = 0; i < epilog->num_nodes; i++) > - bbs[i]->count = bbs[i]->count.apply_scale > - (bbs[i]->count, > - bbs[i]->count.apply_probability > - (prob_vector)); > + if (bbs[i]->count.nonzero_p ()) > + bbs[i]->count = bbs[i]->count.apply_scale > + (bbs[i]->count, > + bbs[i]->count.apply_probability > + (prob_vector)); So exactly what the FIXME in the comment above says happens. It might be better to save/restore the old counts if the intent is to get them back. I'm not exactly sure where the other scaling happens though. Richard. > free (bbs); > } > > -- > 2.25.1
On Thu, May 12, 2022 at 3:37 AM Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com> wrote: > > In my case this is not exactly what the FIXME in the comment above says. The count is 0 even before > the initial scaling happens. I hit this case with some changes I'm working on to enable per-instruction discriminators for AutoFDO. > > I looked into saving/restoring the old counts but it would be awkward to do. The initial scaling happens here: > > if (skip_vector) > { > split_edge (loop_preheader_edge (loop)); > > /* Due to the order in which we peel prolog and epilog, we first > propagate probability to the whole loop. The purpose is to > avoid adjusting probabilities of both prolog and vector loops > separately. Note in this case, the probability of epilog loop > needs to be scaled back later. */ > basic_block bb_before_loop = loop_preheader_edge (loop)->src; > if (prob_vector.initialized_p ()) > { > scale_bbs_frequencies (&bb_before_loop, 1, prob_vector); > scale_loop_profile (loop, prob_vector, 0); > } > } > > The scaling happens before we do the cloning for the epilog so to save/restore the counts > we would need to maintain a mapping between the original basic blocks and the cloned basic blocks in the epilog. There is one already - after the epilogue is copied you can use get_bb_original (epilouge_bb) to get at the block it was copied from. It could also be that we can rely on the basic-block order to be preserved between the copies (I _think_ that would work ... but then assert () for this using get_bb_original ()). That means the simplest fix would be to have an auto_vec<count> and initialize that from the BB counts in loop body order (we also have exactly two BBs in all peeled loops ... But note we only scaled the scalar if-converted loop but eventually used the not if-coverted copy for the epilogue (if not vectorizing the epilogue), so indiscriminate scaling back looks wrong in some cases (I'd have to double-check the details here). > I'd like to get my simple fix in since it makes things better even if it doesn't address the issue mentioned > In the FIXME. But don't you need to check that bbs[i]->count.apply_probability (prob_vector) is not zero instead of checking that bbs[i].count is not zero? Richard. > -----Original Message----- > From: Richard Biener <richard.guenther@gmail.com> > Sent: Monday, May 09, 2022 12:42 AM > To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com>; Jan Hubicka <hubicka@ucw.cz> > Cc: gcc-patches@gcc.gnu.org > Subject: [EXTERNAL] Re: [PATCH] Guard against applying scale with 0 denominator > > On Fri, May 6, 2022 at 10:32 PM Eugene Rozenfeld via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > Calling count.apply_scale with a 0 denominator causes an assert. > > This change guards against that. > > > > Tested on x86_64-pc-linux-gnu. > > > > gcc/ChangeLog: > > * tree-loop-vect-manip.cc (vect_do_peeling): Guard against applying scale with 0 denominator. > > --- > > gcc/tree-vect-loop-manip.cc | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc > > index 1d4337eb261..db54ae69e45 100644 > > --- a/gcc/tree-vect-loop-manip.cc > > +++ b/gcc/tree-vect-loop-manip.cc > > @@ -2989,10 +2989,11 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree niters, tree nitersm1, > > get lost if we scale down to 0. */ > > basic_block *bbs = get_loop_body (epilog); > > for (unsigned int i = 0; i < epilog->num_nodes; i++) > > - bbs[i]->count = bbs[i]->count.apply_scale > > - (bbs[i]->count, > > - bbs[i]->count.apply_probability > > - (prob_vector)); > > + if (bbs[i]->count.nonzero_p ()) > > + bbs[i]->count = bbs[i]->count.apply_scale > > + (bbs[i]->count, > > + bbs[i]->count.apply_probability > > + (prob_vector)); > > So exactly what the FIXME in the comment above says happens. It > might be better > to save/restore the old counts if the intent is to get them back. I'm not exactly sure where the other scaling happens though. > > Richard. > > > > > free (bbs); > > } > > > > -- > > 2.25.1
Thank you for the feedback Richard. I attached a patch that saves/restores counts if the epilog doesn't use a scalar loop. Eugene -----Original Message----- From: Richard Biener <richard.guenther@gmail.com> Sent: Thursday, May 12, 2022 12:34 AM To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com> Cc: Jan Hubicka <hubicka@ucw.cz>; gcc-patches@gcc.gnu.org Subject: Re: [EXTERNAL] Re: [PATCH] Guard against applying scale with 0 denominator On Thu, May 12, 2022 at 3:37 AM Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com> wrote: > > In my case this is not exactly what the FIXME in the comment above > says. The count is 0 even before the initial scaling happens. I hit this case with some changes I'm working on to enable per-instruction discriminators for AutoFDO. > > I looked into saving/restoring the old counts but it would be awkward to do. The initial scaling happens here: > > if (skip_vector) > { > split_edge (loop_preheader_edge (loop)); > > /* Due to the order in which we peel prolog and epilog, we first > propagate probability to the whole loop. The purpose is to > avoid adjusting probabilities of both prolog and vector loops > separately. Note in this case, the probability of epilog loop > needs to be scaled back later. */ > basic_block bb_before_loop = loop_preheader_edge (loop)->src; > if (prob_vector.initialized_p ()) > { > scale_bbs_frequencies (&bb_before_loop, 1, prob_vector); > scale_loop_profile (loop, prob_vector, 0); > } > } > > The scaling happens before we do the cloning for the epilog so to > save/restore the counts we would need to maintain a mapping between the original basic blocks and the cloned basic blocks in the epilog. There is one already - after the epilogue is copied you can use get_bb_original (epilouge_bb) to get at the block it was copied from. It could also be that we can rely on the basic-block order to be preserved between the copies (I _think_ that would work ... but then assert () for this using get_bb_original ()). That means the simplest fix would be to have an auto_vec<count> and initialize that from the BB counts in loop body order (we also have exactly two BBs in all peeled loops ... But note we only scaled the scalar if-converted loop but eventually used the not if-coverted copy for the epilogue (if not vectorizing the epilogue), so indiscriminate scaling back looks wrong in some cases (I'd have to double-check the details here). > I'd like to get my simple fix in since it makes things better even if > it doesn't address the issue mentioned In the FIXME. But don't you need to check that bbs[i]->count.apply_probability (prob_vector) is not zero instead of checking that bbs[i].count is not zero? Richard. > -----Original Message----- > From: Richard Biener <richard.guenther@gmail.com> > Sent: Monday, May 09, 2022 12:42 AM > To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com>; Jan Hubicka > <hubicka@ucw.cz> > Cc: gcc-patches@gcc.gnu.org > Subject: [EXTERNAL] Re: [PATCH] Guard against applying scale with 0 > denominator > > On Fri, May 6, 2022 at 10:32 PM Eugene Rozenfeld via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > Calling count.apply_scale with a 0 denominator causes an assert. > > This change guards against that. > > > > Tested on x86_64-pc-linux-gnu. > > > > gcc/ChangeLog: > > * tree-loop-vect-manip.cc (vect_do_peeling): Guard against applying scale with 0 denominator. > > --- > > gcc/tree-vect-loop-manip.cc | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/gcc/tree-vect-loop-manip.cc > > b/gcc/tree-vect-loop-manip.cc index 1d4337eb261..db54ae69e45 100644 > > --- a/gcc/tree-vect-loop-manip.cc > > +++ b/gcc/tree-vect-loop-manip.cc > > @@ -2989,10 +2989,11 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree niters, tree nitersm1, > > get lost if we scale down to 0. */ > > basic_block *bbs = get_loop_body (epilog); > > for (unsigned int i = 0; i < epilog->num_nodes; i++) > > - bbs[i]->count = bbs[i]->count.apply_scale > > - (bbs[i]->count, > > - bbs[i]->count.apply_probability > > - (prob_vector)); > > + if (bbs[i]->count.nonzero_p ()) > > + bbs[i]->count = bbs[i]->count.apply_scale > > + (bbs[i]->count, > > + bbs[i]->count.apply_probability > > + (prob_vector)); > > So exactly what the FIXME in the comment above says happens. It > might be better > to save/restore the old counts if the intent is to get them back. I'm not exactly sure where the other scaling happens though. > > Richard. > > > > > free (bbs); > > } > > > > -- > > 2.25.1
On Sat, May 21, 2022 at 12:28 AM Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com> wrote: > > Thank you for the feedback Richard. I attached a patch that saves/restores counts if the epilog doesn't use a scalar loop. OK. Thanks, Richard. > Eugene > > -----Original Message----- > From: Richard Biener <richard.guenther@gmail.com> > Sent: Thursday, May 12, 2022 12:34 AM > To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com> > Cc: Jan Hubicka <hubicka@ucw.cz>; gcc-patches@gcc.gnu.org > Subject: Re: [EXTERNAL] Re: [PATCH] Guard against applying scale with 0 denominator > > On Thu, May 12, 2022 at 3:37 AM Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com> wrote: > > > > In my case this is not exactly what the FIXME in the comment above > > says. The count is 0 even before the initial scaling happens. I hit this case with some changes I'm working on to enable per-instruction discriminators for AutoFDO. > > > > I looked into saving/restoring the old counts but it would be awkward to do. The initial scaling happens here: > > > > if (skip_vector) > > { > > split_edge (loop_preheader_edge (loop)); > > > > /* Due to the order in which we peel prolog and epilog, we first > > propagate probability to the whole loop. The purpose is to > > avoid adjusting probabilities of both prolog and vector loops > > separately. Note in this case, the probability of epilog loop > > needs to be scaled back later. */ > > basic_block bb_before_loop = loop_preheader_edge (loop)->src; > > if (prob_vector.initialized_p ()) > > { > > scale_bbs_frequencies (&bb_before_loop, 1, prob_vector); > > scale_loop_profile (loop, prob_vector, 0); > > } > > } > > > > The scaling happens before we do the cloning for the epilog so to > > save/restore the counts we would need to maintain a mapping between the original basic blocks and the cloned basic blocks in the epilog. > > There is one already - after the epilogue is copied you can use get_bb_original (epilouge_bb) to get at the block it was copied from. > It could also be that we can rely on the basic-block order to be preserved between the copies (I _think_ that would work ... but then assert () for this using get_bb_original ()). That means the simplest fix would be to have an auto_vec<count> and initialize that from the BB counts in loop body order (we also have exactly two BBs in all peeled loops ... > > But note we only scaled the scalar if-converted loop but eventually used the not if-coverted copy for the epilogue (if not vectorizing the epilogue), so indiscriminate scaling back looks wrong in some cases (I'd have to double-check the details here). > > > I'd like to get my simple fix in since it makes things better even if > > it doesn't address the issue mentioned In the FIXME. > > But don't you need to check that > bbs[i]->count.apply_probability (prob_vector) is not zero instead of checking that bbs[i].count is not zero? > > Richard. > > > -----Original Message----- > > From: Richard Biener <richard.guenther@gmail.com> > > Sent: Monday, May 09, 2022 12:42 AM > > To: Eugene Rozenfeld <Eugene.Rozenfeld@microsoft.com>; Jan Hubicka > > <hubicka@ucw.cz> > > Cc: gcc-patches@gcc.gnu.org > > Subject: [EXTERNAL] Re: [PATCH] Guard against applying scale with 0 > > denominator > > > > On Fri, May 6, 2022 at 10:32 PM Eugene Rozenfeld via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > > > > > Calling count.apply_scale with a 0 denominator causes an assert. > > > This change guards against that. > > > > > > Tested on x86_64-pc-linux-gnu. > > > > > > gcc/ChangeLog: > > > * tree-loop-vect-manip.cc (vect_do_peeling): Guard against applying scale with 0 denominator. > > > --- > > > gcc/tree-vect-loop-manip.cc | 9 +++++---- > > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > > > diff --git a/gcc/tree-vect-loop-manip.cc > > > b/gcc/tree-vect-loop-manip.cc index 1d4337eb261..db54ae69e45 100644 > > > --- a/gcc/tree-vect-loop-manip.cc > > > +++ b/gcc/tree-vect-loop-manip.cc > > > @@ -2989,10 +2989,11 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree niters, tree nitersm1, > > > get lost if we scale down to 0. */ > > > basic_block *bbs = get_loop_body (epilog); > > > for (unsigned int i = 0; i < epilog->num_nodes; i++) > > > - bbs[i]->count = bbs[i]->count.apply_scale > > > - (bbs[i]->count, > > > - bbs[i]->count.apply_probability > > > - (prob_vector)); > > > + if (bbs[i]->count.nonzero_p ()) > > > + bbs[i]->count = bbs[i]->count.apply_scale > > > + (bbs[i]->count, > > > + bbs[i]->count.apply_probability > > > + (prob_vector)); > > > > So exactly what the FIXME in the comment above says happens. It > > might be better > > to save/restore the old counts if the intent is to get them back. I'm not exactly sure where the other scaling happens though. > > > > Richard. > > > > > > > > > free (bbs); > > > } > > > > > > -- > > > 2.25.1
diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc index 1d4337eb261..db54ae69e45 100644 --- a/gcc/tree-vect-loop-manip.cc +++ b/gcc/tree-vect-loop-manip.cc @@ -2989,10 +2989,11 @@ vect_do_peeling (loop_vec_info loop_vinfo, tree niters, tree nitersm1, get lost if we scale down to 0. */ basic_block *bbs = get_loop_body (epilog); for (unsigned int i = 0; i < epilog->num_nodes; i++) - bbs[i]->count = bbs[i]->count.apply_scale - (bbs[i]->count, - bbs[i]->count.apply_probability - (prob_vector)); + if (bbs[i]->count.nonzero_p ()) + bbs[i]->count = bbs[i]->count.apply_scale + (bbs[i]->count, + bbs[i]->count.apply_probability + (prob_vector)); free (bbs); }