Message ID | 20221005120403.68935-3-jorgen.kvalsvik@woven-planet.global |
---|---|
State | New, archived |
Headers |
Return-Path: <gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4ac7:0:0:0:0:0 with SMTP id y7csp563906wrs; Wed, 5 Oct 2022 05:06:00 -0700 (PDT) X-Google-Smtp-Source: AMsMyM4bg8KdAlTP/eM5+WSy0AhBHoqz+3Pl4agee3KJU7OTIDyPcsfnNd8YAs1S9e1QLsHhJfPv X-Received: by 2002:a17:907:6d28:b0:782:32ad:7b64 with SMTP id sa40-20020a1709076d2800b0078232ad7b64mr24852937ejc.23.1664971560900; Wed, 05 Oct 2022 05:06:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1664971560; cv=none; d=google.com; s=arc-20160816; b=m2xfeO4r8YZX/ibNcaGkW9UJO0/dhBzR+3q6FPutdJe9tf1QVgbJ5E3x2cjWIL5Tnz mYjMpNEDR4q92UIyH3lpDkU/7mrWUR+YYFsQAt5hUZIPFKTtjXW6isp5wfUi+AEDATRF THSYT/BWssJPj3rCDLrgfvAj9dtkJ4rHa3QljdbsYHPwNKxrgHy/t7fllAD3PP1LiPPu Z1UEdppLek2fPgfTqatzNNGEQmAOU2Yb1yaHj2s0I26Aa6dL4s8HlaXOpN2H7nUP2uk3 c3cScT7qBhhf9TF0jKk2rO0yVN4HU4O7zMVxiATb5pB0gB5TPn+DnQpSgqlTfOs/o/xE 8cag== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=sender:errors-to:reply-to:from:list-subscribe:list-help:list-post :list-archive:list-unsubscribe:list-id:precedence :content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:dmarc-filter:delivered-to:dkim-signature :dkim-filter; bh=W9v/g5RcSN+BGfzyVsexfPSX3I3BolHfVT6yL227E+Q=; b=IN1tp72ph4Vvf/1V02Fkhf1YtsZ/cZSQhfPg5/f4KQmeKPp8nBWiP64MzaTaRgwOIk d8OVEkCH06GQ9Is1SeiKih2td751i3upUZuoprxO8j2lWzmakCS4PVVwTHwecwGR1vSA +zoHxnbNAO01r13puV4TYL/coJ3T+QxGi+nXbABPKPSVyXcheB4xchWZ+auE9m7UTGhq FSUv8JUqdfKJhi1wDdT6zZw7bAaDWES1fVbSltsj2FEBe40VKPzxrQm6/TJTxrEN/cfE 1Z6qIgnkES6tOWxw5ngadY+dr5L1Q2HDBA5ekPXT2IHTrzvGVj50kUkVXbHI30aEPmsd yM6Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=JLz2QxsE; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from sourceware.org (server2.sourceware.org. [2620:52:3:1:0:246e:9693:128c]) by mx.google.com with ESMTPS id jg5-20020a170907970500b007828ae31468si12022171ejc.698.2022.10.05.05.06.00 for <ouuuleilei@gmail.com> (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Oct 2022 05:06:00 -0700 (PDT) Received-SPF: pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) client-ip=2620:52:3:1:0:246e:9693:128c; Authentication-Results: mx.google.com; dkim=pass header.i=@gcc.gnu.org header.s=default header.b=JLz2QxsE; spf=pass (google.com: domain of gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org designates 2620:52:3:1:0:246e:9693:128c as permitted sender) smtp.mailfrom="gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=gnu.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 7FB9D3856246 for <ouuuleilei@gmail.com>; Wed, 5 Oct 2022 12:05:09 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 7FB9D3856246 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1664971509; bh=W9v/g5RcSN+BGfzyVsexfPSX3I3BolHfVT6yL227E+Q=; h=To:Subject:Date:In-Reply-To:References:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To: From; b=JLz2QxsEBBMow5tSKwBtu0CasF300lILHxnzFiBEgN23kiInDCVm7eKT7ecS69JIu 8Q7spcO4iBWuDFEf6im1Xk0aCDELPbWcESuguI+W3Gxo6yYfxnoEvigc6oAZeiZbda IAWh5NCvod7ZsxQLC+aqFvf70ZqvVzBOs5Wmy+8c= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from mail-pf1-x431.google.com (mail-pf1-x431.google.com [IPv6:2607:f8b0:4864:20::431]) by sourceware.org (Postfix) with ESMTPS id AAA49385735C for <gcc-patches@gcc.gnu.org>; Wed, 5 Oct 2022 12:04:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AAA49385735C Received: by mail-pf1-x431.google.com with SMTP id y8so1232078pfp.13 for <gcc-patches@gcc.gnu.org>; Wed, 05 Oct 2022 05:04:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=W9v/g5RcSN+BGfzyVsexfPSX3I3BolHfVT6yL227E+Q=; b=FRk5t2mGLdLmmjPcbMhv1hGxUle6JpX2VM1hrW9QJb8Ja+wkWg21/btPAFadtT+sTi 5jWi197b8tckEKBjoe0KCmAIFTHXuarRMMwvTktAo5f+46qbBP/ht+0ffmTNuVjgitfS GjfaDeTrdr/Hjt6tsNW09eTMIjZMAocnKHnTdTzPmbo1Hz/PcxbbhtnH+4iju0KAqYAY 5BFW6DETUIHsroFjRR4NVxMBVcEW7Ql0tsE84BtSILOVH9XNuZcV66Abz0yhv5pl1HVc yOEenXPJsYKx2yit7IzV7r+qTtfnaD5E06hYweByUL+lkmorKTLV1cBQImcK2o/k+elb j72A== X-Gm-Message-State: ACrzQf2+sTxe6Nwi33QzCmyjN/3O7lt/fndsbWnnkz3zCUwQyAFdr3Mr AtsLFT2N1mcmEDUqL22H/ZwDa05skyLnPv7vMsEF9NJmiK1v+FvcazIVZTUpqWbome+eQeWW0+D hzn8uShXMeOg1PODxmOpOJnR78kvKFgCQmvNCpVoazy+2S9qjyhCZ1JQdtKPmbjYvqfbmAlSik0 IcZx95OHkglO57BUDxDpc= X-Received: by 2002:a62:ce83:0:b0:560:97b5:2d2c with SMTP id y125-20020a62ce83000000b0056097b52d2cmr18241960pfg.61.1664971464308; Wed, 05 Oct 2022 05:04:24 -0700 (PDT) Received: from FA21110339.ts.tri-ad.global ([103.175.111.222]) by smtp.gmail.com with ESMTPSA id rm10-20020a17090b3eca00b0020936894e76sm1054084pjb.10.2022.10.05.05.04.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 05 Oct 2022 05:04:23 -0700 (PDT) To: gcc-patches@gcc.gnu.org Subject: [PATCH 2/2] Split edge when edge locus and dest don't match Date: Wed, 5 Oct 2022 14:04:03 +0200 Message-Id: <20221005120403.68935-3-jorgen.kvalsvik@woven-planet.global> X-Mailer: git-send-email 2.34.0 In-Reply-To: <20221005120403.68935-1-jorgen.kvalsvik@woven-planet.global> References: <20221005120403.68935-1-jorgen.kvalsvik@woven-planet.global> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, TXREP, T_SPF_PERMERROR autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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: =?utf-8?q?J=C3=B8rgen_Kvalsvik_via_Gcc-patches?= <gcc-patches@gcc.gnu.org> Reply-To: =?utf-8?q?J=C3=B8rgen_Kvalsvik?= <jorgen.kvalsvik@woven-planet.global> Errors-To: gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+ouuuleilei=gmail.com@gcc.gnu.org> X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1745849218985192364?= X-GMAIL-MSGID: =?utf-8?q?1745849218985192364?= |
Series |
gcov: Split when edge locus differ from dest bb
|
|
Commit Message
Jørgen Kvalsvik
Oct. 5, 2022, 12:04 p.m. UTC
Edges with locus are candidates for splitting so that the edge with locus is the only edge out of a basic block, except when the locuses match. The test checks the last (non-debug) statement in a basic block, but this causes an unnecessary split when the edge locus go to a block which coincidentally has an unrelated label. Comparing the first statement of the destination block is the same check but does not get tripped up by labels. An example with source/edge/dest locus when an edge is split: 1 int fn (int a, int b, int c) { 2 int x = 0; 3 if (a && b) { 4 x = a; 5 } else { 6 a_: 7 x = (a - b); 8 } 9 10 return x; 11 } block file line col stmt src t.c 3 10 if (a_3(D) != 0) edge t.c 6 1 dest t.c 6 1 a_: src t.c 3 13 if (b_4(D) != 0) edge t.c 6 1 dst t.c 6 1 a_: With label removed: 1 int fn (int a, int b, int c) { 2 int x = 0; 3 if (a && b) { 4 x = a; 5 } else { 6 // a_: <- label removed 7 x = (a - b); 8 } 9 10 return x; 11 block file line col stmt src t.c 3 10 if (a_3(D) != 0) edge (null) 0 0 dest t.c 6 1 a_: src t.c 3 13 if (b_4(D) != 0) edge (null) 0 0 dst t.c 6 1 a_: and this is extract from gcov-4b.c which *should* split: 205 int 206 test_switch (int i, int j) 207 { 208 int result = 0; 209 210 switch (i) /* branch(80 25) */ 211 /* branch(end) */ 212 { 213 case 1: 214 result = do_something (2); 215 break; 216 case 2: 217 result = do_something (1024); 218 break; 219 case 3: 220 case 4: 221 if (j == 2) /* branch(67) */ 222 /* branch(end) */ 223 return do_something (4); 224 result = do_something (8); 225 break; 226 default: 227 result = do_something (32); 228 switch_m++; 229 break; 230 } 231 return result; 231 } block file line col stmt src 4b.c 214 18 result_18 = do_something (2); edge 4b.c 215 9 dst 4b.c 231 10 _22 = result_3; src 4b.c 217 18 result_16 = do_something (1024); edge 4b.c 218 9 dst 4b.c 231 10 _22 = result_3; src 4b.c 224 18 result_12 = do_something (8); edge 4b.c 225 9 dst 4b.c 231 10 _22 = result_3; Note that the behaviour of comparison is preserved for the (switch) edge splitting case. The former case now fails the check (even though e->goto_locus is no longer a reserved location) because the dest matches the e->locus. gcc/ChangeLog: * profile.cc (branch_prob): Compare edge locus to dest, not src. --- gcc/profile.cc | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)
Comments
On 10/5/22 14:04, Jørgen Kvalsvik via Gcc-patches wrote: > Edges with locus are candidates for splitting so that the edge with > locus is the only edge out of a basic block, except when the locuses > match. The test checks the last (non-debug) statement in a basic block, > but this causes an unnecessary split when the edge locus go to a block > which coincidentally has an unrelated label. Comparing the first > statement of the destination block is the same check but does not get > tripped up by labels. > > An example with source/edge/dest locus when an edge is split: > > 1 int fn (int a, int b, int c) { > 2 int x = 0; > 3 if (a && b) { > 4 x = a; > 5 } else { > 6 a_: > 7 x = (a - b); > 8 } > 9 > 10 return x; > 11 } > > block file line col stmt > > src t.c 3 10 if (a_3(D) != 0) > edge t.c 6 1 > dest t.c 6 1 a_: > > src t.c 3 13 if (b_4(D) != 0) > edge t.c 6 1 > dst t.c 6 1 a_: > > With label removed: > > 1 int fn (int a, int b, int c) { > 2 int x = 0; > 3 if (a && b) { > 4 x = a; > 5 } else { > 6 // a_: <- label removed > 7 x = (a - b); > 8 } > 9 > 10 return x; > 11 > > block file line col stmt > > src t.c 3 10 if (a_3(D) != 0) > edge (null) 0 0 > dest t.c 6 1 a_: > > src t.c 3 13 if (b_4(D) != 0) > edge (null) 0 0 > dst t.c 6 1 a_: > > and this is extract from gcov-4b.c which *should* split: > > 205 int > 206 test_switch (int i, int j) > 207 { > 208 int result = 0; > 209 > 210 switch (i) /* branch(80 25) */ > 211 /* branch(end) */ > 212 { > 213 case 1: > 214 result = do_something (2); > 215 break; > 216 case 2: > 217 result = do_something (1024); > 218 break; > 219 case 3: > 220 case 4: > 221 if (j == 2) /* branch(67) */ > 222 /* branch(end) */ > 223 return do_something (4); > 224 result = do_something (8); > 225 break; > 226 default: > 227 result = do_something (32); > 228 switch_m++; > 229 break; > 230 } > 231 return result; > 231 } > > block file line col stmt > > src 4b.c 214 18 result_18 = do_something (2); > edge 4b.c 215 9 > dst 4b.c 231 10 _22 = result_3; > > src 4b.c 217 18 result_16 = do_something (1024); > edge 4b.c 218 9 > dst 4b.c 231 10 _22 = result_3; > > src 4b.c 224 18 result_12 = do_something (8); > edge 4b.c 225 9 > dst 4b.c 231 10 _22 = result_3; > > Note that the behaviour of comparison is preserved for the (switch) edge > splitting case. The former case now fails the check (even though > e->goto_locus is no longer a reserved location) because the dest matches > the e->locus. It's fine, please install it. I verified tramp3d coverage output is the same as before the patch. Martin > > gcc/ChangeLog: > > * profile.cc (branch_prob): Compare edge locus to dest, not src. > --- > gcc/profile.cc | 18 +++++++++--------- > 1 file changed, 9 insertions(+), 9 deletions(-) > > diff --git a/gcc/profile.cc b/gcc/profile.cc > index 96121d60711..c13a79a84c2 100644 > --- a/gcc/profile.cc > +++ b/gcc/profile.cc > @@ -1208,17 +1208,17 @@ branch_prob (bool thunk) > FOR_EACH_EDGE (e, ei, bb->succs) > { > gimple_stmt_iterator gsi; > - gimple *last = NULL; > + gimple *dest = NULL; > > /* It may happen that there are compiler generated statements > without a locus at all. Go through the basic block from the > last to the first statement looking for a locus. */ > - for (gsi = gsi_last_nondebug_bb (bb); > + for (gsi = gsi_start_nondebug_bb (bb); > !gsi_end_p (gsi); > - gsi_prev_nondebug (&gsi)) > + gsi_next_nondebug (&gsi)) > { > - last = gsi_stmt (gsi); > - if (!RESERVED_LOCATION_P (gimple_location (last))) > + dest = gsi_stmt (gsi); > + if (!RESERVED_LOCATION_P (gimple_location (dest))) > break; > } > > @@ -1227,14 +1227,14 @@ branch_prob (bool thunk) > Don't do that when the locuses match, so > if (blah) goto something; > is not computed twice. */ > - if (last > - && gimple_has_location (last) > + if (dest > + && gimple_has_location (dest) > && !RESERVED_LOCATION_P (e->goto_locus) > && !single_succ_p (bb) > && (LOCATION_FILE (e->goto_locus) > - != LOCATION_FILE (gimple_location (last)) > + != LOCATION_FILE (gimple_location (dest)) > || (LOCATION_LINE (e->goto_locus) > - != LOCATION_LINE (gimple_location (last))))) > + != LOCATION_LINE (gimple_location (dest))))) > { > basic_block new_bb = split_edge (e); > edge ne = single_succ_edge (new_bb);
On Wed, Oct 5, 2022 at 2:49 PM Martin Liška <mliska@suse.cz> wrote: > > On 10/5/22 14:04, Jørgen Kvalsvik via Gcc-patches wrote: > > Edges with locus are candidates for splitting so that the edge with > > locus is the only edge out of a basic block, except when the locuses > > match. The test checks the last (non-debug) statement in a basic block, > > but this causes an unnecessary split when the edge locus go to a block > > which coincidentally has an unrelated label. Comparing the first > > statement of the destination block is the same check but does not get > > tripped up by labels. > > > > An example with source/edge/dest locus when an edge is split: > > > > 1 int fn (int a, int b, int c) { > > 2 int x = 0; > > 3 if (a && b) { > > 4 x = a; > > 5 } else { > > 6 a_: > > 7 x = (a - b); > > 8 } > > 9 > > 10 return x; > > 11 } > > > > block file line col stmt > > > > src t.c 3 10 if (a_3(D) != 0) > > edge t.c 6 1 > > dest t.c 6 1 a_: > > > > src t.c 3 13 if (b_4(D) != 0) > > edge t.c 6 1 > > dst t.c 6 1 a_: > > > > With label removed: > > > > 1 int fn (int a, int b, int c) { > > 2 int x = 0; > > 3 if (a && b) { > > 4 x = a; > > 5 } else { > > 6 // a_: <- label removed > > 7 x = (a - b); > > 8 } > > 9 > > 10 return x; > > 11 > > > > block file line col stmt > > > > src t.c 3 10 if (a_3(D) != 0) > > edge (null) 0 0 > > dest t.c 6 1 a_: > > > > src t.c 3 13 if (b_4(D) != 0) > > edge (null) 0 0 > > dst t.c 6 1 a_: > > > > and this is extract from gcov-4b.c which *should* split: > > > > 205 int > > 206 test_switch (int i, int j) > > 207 { > > 208 int result = 0; > > 209 > > 210 switch (i) /* branch(80 25) */ > > 211 /* branch(end) */ > > 212 { > > 213 case 1: > > 214 result = do_something (2); > > 215 break; > > 216 case 2: > > 217 result = do_something (1024); > > 218 break; > > 219 case 3: > > 220 case 4: > > 221 if (j == 2) /* branch(67) */ > > 222 /* branch(end) */ > > 223 return do_something (4); > > 224 result = do_something (8); > > 225 break; > > 226 default: > > 227 result = do_something (32); > > 228 switch_m++; > > 229 break; > > 230 } > > 231 return result; > > 231 } > > > > block file line col stmt > > > > src 4b.c 214 18 result_18 = do_something (2); > > edge 4b.c 215 9 > > dst 4b.c 231 10 _22 = result_3; > > > > src 4b.c 217 18 result_16 = do_something (1024); > > edge 4b.c 218 9 > > dst 4b.c 231 10 _22 = result_3; > > > > src 4b.c 224 18 result_12 = do_something (8); > > edge 4b.c 225 9 > > dst 4b.c 231 10 _22 = result_3; > > > > Note that the behaviour of comparison is preserved for the (switch) edge > > splitting case. The former case now fails the check (even though > > e->goto_locus is no longer a reserved location) because the dest matches > > the e->locus. > > It's fine, please install it. > I verified tramp3d coverage output is the same as before the patch. > > Martin > > > > > gcc/ChangeLog: > > > > * profile.cc (branch_prob): Compare edge locus to dest, not src. > > --- > > gcc/profile.cc | 18 +++++++++--------- > > 1 file changed, 9 insertions(+), 9 deletions(-) > > > > diff --git a/gcc/profile.cc b/gcc/profile.cc > > index 96121d60711..c13a79a84c2 100644 > > --- a/gcc/profile.cc > > +++ b/gcc/profile.cc > > @@ -1208,17 +1208,17 @@ branch_prob (bool thunk) > > FOR_EACH_EDGE (e, ei, bb->succs) > > { > > gimple_stmt_iterator gsi; > > - gimple *last = NULL; > > + gimple *dest = NULL; > > > > /* It may happen that there are compiler generated statements > > without a locus at all. Go through the basic block from the > > last to the first statement looking for a locus. */ The comment no longer matches the code. > > - for (gsi = gsi_last_nondebug_bb (bb); > > + for (gsi = gsi_start_nondebug_bb (bb); ^^^ and you are looking at the branch block stmts (bb), not the destination block stmts (e->dest) > > !gsi_end_p (gsi); > > - gsi_prev_nondebug (&gsi)) > > + gsi_next_nondebug (&gsi)) > > { > > - last = gsi_stmt (gsi); > > - if (!RESERVED_LOCATION_P (gimple_location (last))) > > + dest = gsi_stmt (gsi); > > + if (!RESERVED_LOCATION_P (gimple_location (dest))) > > break; > > } > > > > @@ -1227,14 +1227,14 @@ branch_prob (bool thunk) > > Don't do that when the locuses match, so > > if (blah) goto something; > > is not computed twice. */ > > - if (last > > - && gimple_has_location (last) > > + if (dest > > + && gimple_has_location (dest) > > && !RESERVED_LOCATION_P (e->goto_locus) > > && !single_succ_p (bb) > > && (LOCATION_FILE (e->goto_locus) > > - != LOCATION_FILE (gimple_location (last)) > > + != LOCATION_FILE (gimple_location (dest)) > > || (LOCATION_LINE (e->goto_locus) > > - != LOCATION_LINE (gimple_location (last))))) > > + != LOCATION_LINE (gimple_location (dest))))) this heuristic needs to be in sync with how we insert edge counters which seems to be hidden in the MST compute (and/or edge insertion). I don't see how it's a win to disregard 'last' and only consider 'dest' here. I think the patch is wrong. Please revert if you applied it already. Thanks, Richard. > > { > > basic_block new_bb = split_edge (e); > > edge ne = single_succ_edge (new_bb); >
On 06/10/2022 10:12, Richard Biener wrote: > On Wed, Oct 5, 2022 at 2:49 PM Martin Liška <mliska@suse.cz> wrote: >> >> On 10/5/22 14:04, Jørgen Kvalsvik via Gcc-patches wrote: >>> Edges with locus are candidates for splitting so that the edge with >>> locus is the only edge out of a basic block, except when the locuses >>> match. The test checks the last (non-debug) statement in a basic block, >>> but this causes an unnecessary split when the edge locus go to a block >>> which coincidentally has an unrelated label. Comparing the first >>> statement of the destination block is the same check but does not get >>> tripped up by labels. >>> >>> An example with source/edge/dest locus when an edge is split: >>> >>> 1 int fn (int a, int b, int c) { >>> 2 int x = 0; >>> 3 if (a && b) { >>> 4 x = a; >>> 5 } else { >>> 6 a_: >>> 7 x = (a - b); >>> 8 } >>> 9 >>> 10 return x; >>> 11 } >>> >>> block file line col stmt >>> >>> src t.c 3 10 if (a_3(D) != 0) >>> edge t.c 6 1 >>> dest t.c 6 1 a_: >>> >>> src t.c 3 13 if (b_4(D) != 0) >>> edge t.c 6 1 >>> dst t.c 6 1 a_: >>> >>> With label removed: >>> >>> 1 int fn (int a, int b, int c) { >>> 2 int x = 0; >>> 3 if (a && b) { >>> 4 x = a; >>> 5 } else { >>> 6 // a_: <- label removed >>> 7 x = (a - b); >>> 8 } >>> 9 >>> 10 return x; >>> 11 >>> >>> block file line col stmt >>> >>> src t.c 3 10 if (a_3(D) != 0) >>> edge (null) 0 0 >>> dest t.c 6 1 a_: >>> >>> src t.c 3 13 if (b_4(D) != 0) >>> edge (null) 0 0 >>> dst t.c 6 1 a_: >>> >>> and this is extract from gcov-4b.c which *should* split: >>> >>> 205 int >>> 206 test_switch (int i, int j) >>> 207 { >>> 208 int result = 0; >>> 209 >>> 210 switch (i) /* branch(80 25) */ >>> 211 /* branch(end) */ >>> 212 { >>> 213 case 1: >>> 214 result = do_something (2); >>> 215 break; >>> 216 case 2: >>> 217 result = do_something (1024); >>> 218 break; >>> 219 case 3: >>> 220 case 4: >>> 221 if (j == 2) /* branch(67) */ >>> 222 /* branch(end) */ >>> 223 return do_something (4); >>> 224 result = do_something (8); >>> 225 break; >>> 226 default: >>> 227 result = do_something (32); >>> 228 switch_m++; >>> 229 break; >>> 230 } >>> 231 return result; >>> 231 } >>> >>> block file line col stmt >>> >>> src 4b.c 214 18 result_18 = do_something (2); >>> edge 4b.c 215 9 >>> dst 4b.c 231 10 _22 = result_3; >>> >>> src 4b.c 217 18 result_16 = do_something (1024); >>> edge 4b.c 218 9 >>> dst 4b.c 231 10 _22 = result_3; >>> >>> src 4b.c 224 18 result_12 = do_something (8); >>> edge 4b.c 225 9 >>> dst 4b.c 231 10 _22 = result_3; >>> >>> Note that the behaviour of comparison is preserved for the (switch) edge >>> splitting case. The former case now fails the check (even though >>> e->goto_locus is no longer a reserved location) because the dest matches >>> the e->locus. >> >> It's fine, please install it. >> I verified tramp3d coverage output is the same as before the patch. >> >> Martin >> >>> >>> gcc/ChangeLog: >>> >>> * profile.cc (branch_prob): Compare edge locus to dest, not src. >>> --- >>> gcc/profile.cc | 18 +++++++++--------- >>> 1 file changed, 9 insertions(+), 9 deletions(-) >>> >>> diff --git a/gcc/profile.cc b/gcc/profile.cc >>> index 96121d60711..c13a79a84c2 100644 >>> --- a/gcc/profile.cc >>> +++ b/gcc/profile.cc >>> @@ -1208,17 +1208,17 @@ branch_prob (bool thunk) >>> FOR_EACH_EDGE (e, ei, bb->succs) >>> { >>> gimple_stmt_iterator gsi; >>> - gimple *last = NULL; >>> + gimple *dest = NULL; >>> >>> /* It may happen that there are compiler generated statements >>> without a locus at all. Go through the basic block from the >>> last to the first statement looking for a locus. */ > > The comment no longer matches the code.> >>> - for (gsi = gsi_last_nondebug_bb (bb); >>> + for (gsi = gsi_start_nondebug_bb (bb); > > ^^^ and you are looking at the branch block stmts (bb), not the destination > block stmts (e->dest) > >>> !gsi_end_p (gsi); >>> - gsi_prev_nondebug (&gsi)) >>> + gsi_next_nondebug (&gsi)) >>> { >>> - last = gsi_stmt (gsi); >>> - if (!RESERVED_LOCATION_P (gimple_location (last))) >>> + dest = gsi_stmt (gsi); >>> + if (!RESERVED_LOCATION_P (gimple_location (dest))) >>> break; >>> } >>> >>> @@ -1227,14 +1227,14 @@ branch_prob (bool thunk) >>> Don't do that when the locuses match, so >>> if (blah) goto something; >>> is not computed twice. */ >>> - if (last >>> - && gimple_has_location (last) >>> + if (dest >>> + && gimple_has_location (dest) >>> && !RESERVED_LOCATION_P (e->goto_locus) >>> && !single_succ_p (bb) >>> && (LOCATION_FILE (e->goto_locus) >>> - != LOCATION_FILE (gimple_location (last)) >>> + != LOCATION_FILE (gimple_location (dest)) >>> || (LOCATION_LINE (e->goto_locus) >>> - != LOCATION_LINE (gimple_location (last))))) >>> + != LOCATION_LINE (gimple_location (dest))))) > > this heuristic needs to be in sync with how we insert edge counters > which seems to be hidden in the MST compute (and/or edge insertion). > I don't see how it's a win to disregard 'last' and only consider 'dest' here. > > I think the patch is wrong. Please revert if you applied it already. I haven't applied it yet, so unless someone beat me to it there's fortunately nothing to revert. > I don't see how it's a win to disregard 'last' and only consider 'dest' here. It might not be other than that it unbreaks my condition profiling by not splitting condition edges and apparently doesn't cause a regression (one caught by tests anyway). That being said the heuristic may very well be wrong (as is the implementation since it considers bb and not e->dest, although I'm sure I tested it with e->dest at some point). I guess the most important question is if the if (a && b) {} {label:} edges should be split in the first place. As mentioned in the patch set, the only difference in the test suite happens on break in switches. I'll tinker a bit more to see if I can figure out what's going on or if the heuristic can otherwise be improved. Then, when does a block with a goto_locus edge have multiple successors? From my previous testing it doesn't seem like it's the conditions make a goto_locus, but it's more than just plain gotos right? When would it then have multiple successors? Switches and exception handling? If that's the case then maybe the heuristic can be improved by simply checking the edge type. Thanks, Jørgen > > Thanks, > Richard. > >>> { >>> basic_block new_bb = split_edge (e); >>> edge ne = single_succ_edge (new_bb); >>
On Thu, Oct 6, 2022 at 4:28 PM Jørgen Kvalsvik <jorgen.kvalsvik@woven-planet.global> wrote: > > On 06/10/2022 10:12, Richard Biener wrote: > > On Wed, Oct 5, 2022 at 2:49 PM Martin Liška <mliska@suse.cz> wrote: > >> > >> On 10/5/22 14:04, Jørgen Kvalsvik via Gcc-patches wrote: > >>> Edges with locus are candidates for splitting so that the edge with > >>> locus is the only edge out of a basic block, except when the locuses > >>> match. The test checks the last (non-debug) statement in a basic block, > >>> but this causes an unnecessary split when the edge locus go to a block > >>> which coincidentally has an unrelated label. Comparing the first > >>> statement of the destination block is the same check but does not get > >>> tripped up by labels. > >>> > >>> An example with source/edge/dest locus when an edge is split: > >>> > >>> 1 int fn (int a, int b, int c) { > >>> 2 int x = 0; > >>> 3 if (a && b) { > >>> 4 x = a; > >>> 5 } else { > >>> 6 a_: > >>> 7 x = (a - b); > >>> 8 } > >>> 9 > >>> 10 return x; > >>> 11 } > >>> > >>> block file line col stmt > >>> > >>> src t.c 3 10 if (a_3(D) != 0) > >>> edge t.c 6 1 > >>> dest t.c 6 1 a_: > >>> > >>> src t.c 3 13 if (b_4(D) != 0) > >>> edge t.c 6 1 > >>> dst t.c 6 1 a_: > >>> > >>> With label removed: > >>> > >>> 1 int fn (int a, int b, int c) { > >>> 2 int x = 0; > >>> 3 if (a && b) { > >>> 4 x = a; > >>> 5 } else { > >>> 6 // a_: <- label removed > >>> 7 x = (a - b); > >>> 8 } > >>> 9 > >>> 10 return x; > >>> 11 > >>> > >>> block file line col stmt > >>> > >>> src t.c 3 10 if (a_3(D) != 0) > >>> edge (null) 0 0 > >>> dest t.c 6 1 a_: > >>> > >>> src t.c 3 13 if (b_4(D) != 0) > >>> edge (null) 0 0 > >>> dst t.c 6 1 a_: > >>> > >>> and this is extract from gcov-4b.c which *should* split: > >>> > >>> 205 int > >>> 206 test_switch (int i, int j) > >>> 207 { > >>> 208 int result = 0; > >>> 209 > >>> 210 switch (i) /* branch(80 25) */ > >>> 211 /* branch(end) */ > >>> 212 { > >>> 213 case 1: > >>> 214 result = do_something (2); > >>> 215 break; > >>> 216 case 2: > >>> 217 result = do_something (1024); > >>> 218 break; > >>> 219 case 3: > >>> 220 case 4: > >>> 221 if (j == 2) /* branch(67) */ > >>> 222 /* branch(end) */ > >>> 223 return do_something (4); > >>> 224 result = do_something (8); > >>> 225 break; > >>> 226 default: > >>> 227 result = do_something (32); > >>> 228 switch_m++; > >>> 229 break; > >>> 230 } > >>> 231 return result; > >>> 231 } > >>> > >>> block file line col stmt > >>> > >>> src 4b.c 214 18 result_18 = do_something (2); > >>> edge 4b.c 215 9 > >>> dst 4b.c 231 10 _22 = result_3; > >>> > >>> src 4b.c 217 18 result_16 = do_something (1024); > >>> edge 4b.c 218 9 > >>> dst 4b.c 231 10 _22 = result_3; > >>> > >>> src 4b.c 224 18 result_12 = do_something (8); > >>> edge 4b.c 225 9 > >>> dst 4b.c 231 10 _22 = result_3; > >>> > >>> Note that the behaviour of comparison is preserved for the (switch) edge > >>> splitting case. The former case now fails the check (even though > >>> e->goto_locus is no longer a reserved location) because the dest matches > >>> the e->locus. > >> > >> It's fine, please install it. > >> I verified tramp3d coverage output is the same as before the patch. > >> > >> Martin > >> > >>> > >>> gcc/ChangeLog: > >>> > >>> * profile.cc (branch_prob): Compare edge locus to dest, not src. > >>> --- > >>> gcc/profile.cc | 18 +++++++++--------- > >>> 1 file changed, 9 insertions(+), 9 deletions(-) > >>> > >>> diff --git a/gcc/profile.cc b/gcc/profile.cc > >>> index 96121d60711..c13a79a84c2 100644 > >>> --- a/gcc/profile.cc > >>> +++ b/gcc/profile.cc > >>> @@ -1208,17 +1208,17 @@ branch_prob (bool thunk) > >>> FOR_EACH_EDGE (e, ei, bb->succs) > >>> { > >>> gimple_stmt_iterator gsi; > >>> - gimple *last = NULL; > >>> + gimple *dest = NULL; > >>> > >>> /* It may happen that there are compiler generated statements > >>> without a locus at all. Go through the basic block from the > >>> last to the first statement looking for a locus. */ > > > > The comment no longer matches the code.> > >>> - for (gsi = gsi_last_nondebug_bb (bb); > >>> + for (gsi = gsi_start_nondebug_bb (bb); > > > > ^^^ and you are looking at the branch block stmts (bb), not the destination > > block stmts (e->dest) > > > >>> !gsi_end_p (gsi); > >>> - gsi_prev_nondebug (&gsi)) > >>> + gsi_next_nondebug (&gsi)) > >>> { > >>> - last = gsi_stmt (gsi); > >>> - if (!RESERVED_LOCATION_P (gimple_location (last))) > >>> + dest = gsi_stmt (gsi); > >>> + if (!RESERVED_LOCATION_P (gimple_location (dest))) > >>> break; > >>> } > >>> > >>> @@ -1227,14 +1227,14 @@ branch_prob (bool thunk) > >>> Don't do that when the locuses match, so > >>> if (blah) goto something; > >>> is not computed twice. */ > >>> - if (last > >>> - && gimple_has_location (last) > >>> + if (dest > >>> + && gimple_has_location (dest) > >>> && !RESERVED_LOCATION_P (e->goto_locus) > >>> && !single_succ_p (bb) > >>> && (LOCATION_FILE (e->goto_locus) > >>> - != LOCATION_FILE (gimple_location (last)) > >>> + != LOCATION_FILE (gimple_location (dest)) > >>> || (LOCATION_LINE (e->goto_locus) > >>> - != LOCATION_LINE (gimple_location (last))))) > >>> + != LOCATION_LINE (gimple_location (dest))))) > > > > this heuristic needs to be in sync with how we insert edge counters > > which seems to be hidden in the MST compute (and/or edge insertion). > > I don't see how it's a win to disregard 'last' and only consider 'dest' here. > > > > I think the patch is wrong. Please revert if you applied it already. > > I haven't applied it yet, so unless someone beat me to it there's fortunately > nothing to revert. > > > I don't see how it's a win to disregard 'last' and only consider 'dest' here. > > It might not be other than that it unbreaks my condition profiling by not > splitting condition edges and apparently doesn't cause a regression (one caught > by tests anyway). That being said the heuristic may very well be wrong (as is > the implementation since it considers bb and not e->dest, although I'm sure I > tested it with e->dest at some point). > > I guess the most important question is if the if (a && b) {} {label:} edges > should be split in the first place. As mentioned in the patch set, the only > difference in the test suite happens on break in switches. I'll tinker a bit > more to see if I can figure out what's going on or if the heuristic can > otherwise be improved. > > Then, when does a block with a goto_locus edge have multiple successors? From my > previous testing it doesn't seem like it's the conditions make a goto_locus, but > it's more than just plain gotos right? When would it then have multiple > successors? Switches and exception handling? If that's the case then maybe the > heuristic can be improved by simply checking the edge type. The goto_locus of an edge is usually either the locus of the control stmt or the locus of the stmt the control transfers to. The most important case is for 'goto' stmts themselves since those are elided and become edges (thus 'goto_locus'). My understanding as of why we split edges at all is that we want to instrument different locations with different counters and since we cannot have counters on edges itself but have to either insert the counter on the source or the destination we in some cases have to split the edge to create an insert location to not falsely account. instrument_edges seems to simply use gsi_insert_on_edge which inserts with the gimple_find_edge_insert_loc logic which doesn't look at goto_locus at all. So the existing heuristic must be fragile as well. BUT - as you say, the plain goto shouldn't be subject to edge instrumentation. The interesting case is probably computed goto (which has multiple successors) and from what I can see a branch where we take the goto_locus from the COND_EXPRs then/else goto stmt which in theory could have different locations. I don't fully understand your requirement of not splitting edges - I'll just note that the place you are patching is not the only place where edges are split (but the insert location computation only sees those splits). Richard. > Thanks, > Jørgen > > > > > > Thanks, > > Richard. > > > >>> { > >>> basic_block new_bb = split_edge (e); > >>> edge ne = single_succ_edge (new_bb); > >>
On 07/10/2022 08:53, Richard Biener wrote: > On Thu, Oct 6, 2022 at 4:28 PM Jørgen Kvalsvik > <jorgen.kvalsvik@woven-planet.global> wrote: >> >> On 06/10/2022 10:12, Richard Biener wrote: >>> On Wed, Oct 5, 2022 at 2:49 PM Martin Liška <mliska@suse.cz> wrote: >>>> >>>> On 10/5/22 14:04, Jørgen Kvalsvik via Gcc-patches wrote: >>>>> Edges with locus are candidates for splitting so that the edge with >>>>> locus is the only edge out of a basic block, except when the locuses >>>>> match. The test checks the last (non-debug) statement in a basic block, >>>>> but this causes an unnecessary split when the edge locus go to a block >>>>> which coincidentally has an unrelated label. Comparing the first >>>>> statement of the destination block is the same check but does not get >>>>> tripped up by labels. >>>>> >>>>> An example with source/edge/dest locus when an edge is split: >>>>> >>>>> 1 int fn (int a, int b, int c) { >>>>> 2 int x = 0; >>>>> 3 if (a && b) { >>>>> 4 x = a; >>>>> 5 } else { >>>>> 6 a_: >>>>> 7 x = (a - b); >>>>> 8 } >>>>> 9 >>>>> 10 return x; >>>>> 11 } >>>>> >>>>> block file line col stmt >>>>> >>>>> src t.c 3 10 if (a_3(D) != 0) >>>>> edge t.c 6 1 >>>>> dest t.c 6 1 a_: >>>>> >>>>> src t.c 3 13 if (b_4(D) != 0) >>>>> edge t.c 6 1 >>>>> dst t.c 6 1 a_: >>>>> >>>>> With label removed: >>>>> >>>>> 1 int fn (int a, int b, int c) { >>>>> 2 int x = 0; >>>>> 3 if (a && b) { >>>>> 4 x = a; >>>>> 5 } else { >>>>> 6 // a_: <- label removed >>>>> 7 x = (a - b); >>>>> 8 } >>>>> 9 >>>>> 10 return x; >>>>> 11 >>>>> >>>>> block file line col stmt >>>>> >>>>> src t.c 3 10 if (a_3(D) != 0) >>>>> edge (null) 0 0 >>>>> dest t.c 6 1 a_: >>>>> >>>>> src t.c 3 13 if (b_4(D) != 0) >>>>> edge (null) 0 0 >>>>> dst t.c 6 1 a_: >>>>> >>>>> and this is extract from gcov-4b.c which *should* split: >>>>> >>>>> 205 int >>>>> 206 test_switch (int i, int j) >>>>> 207 { >>>>> 208 int result = 0; >>>>> 209 >>>>> 210 switch (i) /* branch(80 25) */ >>>>> 211 /* branch(end) */ >>>>> 212 { >>>>> 213 case 1: >>>>> 214 result = do_something (2); >>>>> 215 break; >>>>> 216 case 2: >>>>> 217 result = do_something (1024); >>>>> 218 break; >>>>> 219 case 3: >>>>> 220 case 4: >>>>> 221 if (j == 2) /* branch(67) */ >>>>> 222 /* branch(end) */ >>>>> 223 return do_something (4); >>>>> 224 result = do_something (8); >>>>> 225 break; >>>>> 226 default: >>>>> 227 result = do_something (32); >>>>> 228 switch_m++; >>>>> 229 break; >>>>> 230 } >>>>> 231 return result; >>>>> 231 } >>>>> >>>>> block file line col stmt >>>>> >>>>> src 4b.c 214 18 result_18 = do_something (2); >>>>> edge 4b.c 215 9 >>>>> dst 4b.c 231 10 _22 = result_3; >>>>> >>>>> src 4b.c 217 18 result_16 = do_something (1024); >>>>> edge 4b.c 218 9 >>>>> dst 4b.c 231 10 _22 = result_3; >>>>> >>>>> src 4b.c 224 18 result_12 = do_something (8); >>>>> edge 4b.c 225 9 >>>>> dst 4b.c 231 10 _22 = result_3; >>>>> >>>>> Note that the behaviour of comparison is preserved for the (switch) edge >>>>> splitting case. The former case now fails the check (even though >>>>> e->goto_locus is no longer a reserved location) because the dest matches >>>>> the e->locus. >>>> >>>> It's fine, please install it. >>>> I verified tramp3d coverage output is the same as before the patch. >>>> >>>> Martin >>>> >>>>> >>>>> gcc/ChangeLog: >>>>> >>>>> * profile.cc (branch_prob): Compare edge locus to dest, not src. >>>>> --- >>>>> gcc/profile.cc | 18 +++++++++--------- >>>>> 1 file changed, 9 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/gcc/profile.cc b/gcc/profile.cc >>>>> index 96121d60711..c13a79a84c2 100644 >>>>> --- a/gcc/profile.cc >>>>> +++ b/gcc/profile.cc >>>>> @@ -1208,17 +1208,17 @@ branch_prob (bool thunk) >>>>> FOR_EACH_EDGE (e, ei, bb->succs) >>>>> { >>>>> gimple_stmt_iterator gsi; >>>>> - gimple *last = NULL; >>>>> + gimple *dest = NULL; >>>>> >>>>> /* It may happen that there are compiler generated statements >>>>> without a locus at all. Go through the basic block from the >>>>> last to the first statement looking for a locus. */ >>> >>> The comment no longer matches the code.> >>>>> - for (gsi = gsi_last_nondebug_bb (bb); >>>>> + for (gsi = gsi_start_nondebug_bb (bb); >>> >>> ^^^ and you are looking at the branch block stmts (bb), not the destination >>> block stmts (e->dest) >>> >>>>> !gsi_end_p (gsi); >>>>> - gsi_prev_nondebug (&gsi)) >>>>> + gsi_next_nondebug (&gsi)) >>>>> { >>>>> - last = gsi_stmt (gsi); >>>>> - if (!RESERVED_LOCATION_P (gimple_location (last))) >>>>> + dest = gsi_stmt (gsi); >>>>> + if (!RESERVED_LOCATION_P (gimple_location (dest))) >>>>> break; >>>>> } >>>>> >>>>> @@ -1227,14 +1227,14 @@ branch_prob (bool thunk) >>>>> Don't do that when the locuses match, so >>>>> if (blah) goto something; >>>>> is not computed twice. */ >>>>> - if (last >>>>> - && gimple_has_location (last) >>>>> + if (dest >>>>> + && gimple_has_location (dest) >>>>> && !RESERVED_LOCATION_P (e->goto_locus) >>>>> && !single_succ_p (bb) >>>>> && (LOCATION_FILE (e->goto_locus) >>>>> - != LOCATION_FILE (gimple_location (last)) >>>>> + != LOCATION_FILE (gimple_location (dest)) >>>>> || (LOCATION_LINE (e->goto_locus) >>>>> - != LOCATION_LINE (gimple_location (last))))) >>>>> + != LOCATION_LINE (gimple_location (dest))))) >>> >>> this heuristic needs to be in sync with how we insert edge counters >>> which seems to be hidden in the MST compute (and/or edge insertion). >>> I don't see how it's a win to disregard 'last' and only consider 'dest' here. >>> >>> I think the patch is wrong. Please revert if you applied it already. >> >> I haven't applied it yet, so unless someone beat me to it there's fortunately >> nothing to revert. >> >>> I don't see how it's a win to disregard 'last' and only consider 'dest' here. >> >> It might not be other than that it unbreaks my condition profiling by not >> splitting condition edges and apparently doesn't cause a regression (one caught >> by tests anyway). That being said the heuristic may very well be wrong (as is >> the implementation since it considers bb and not e->dest, although I'm sure I >> tested it with e->dest at some point). >> >> I guess the most important question is if the if (a && b) {} {label:} edges >> should be split in the first place. As mentioned in the patch set, the only >> difference in the test suite happens on break in switches. I'll tinker a bit >> more to see if I can figure out what's going on or if the heuristic can >> otherwise be improved. >> >> Then, when does a block with a goto_locus edge have multiple successors? From my >> previous testing it doesn't seem like it's the conditions make a goto_locus, but >> it's more than just plain gotos right? When would it then have multiple >> successors? Switches and exception handling? If that's the case then maybe the >> heuristic can be improved by simply checking the edge type. > > The goto_locus of an edge is usually either the locus of the control stmt or the > locus of the stmt the control transfers to. The most important case is for > 'goto' stmts themselves since those are elided and become edges (thus > 'goto_locus'). > > My understanding as of why we split edges at all is that we want to instrument > different locations with different counters and since we cannot have counters on > edges itself but have to either insert the counter on the source or > the destination > we in some cases have to split the edge to create an insert location > to not falsely > account. instrument_edges seems to simply use gsi_insert_on_edge which > inserts with the gimple_find_edge_insert_loc logic which doesn't look > at goto_locus > at all. So the existing heuristic must be fragile as well. > > BUT - as you say, the plain goto shouldn't be subject to edge instrumentation. > The interesting case is probably computed goto (which has multiple successors) > and from what I can see a branch where we take the goto_locus from the > COND_EXPRs then/else goto stmt which in theory could have different locations. > > I don't fully understand your requirement of not splitting edges - > I'll just note that > the place you are patching is not the only place where edges are split (but > the insert location computation only sees those splits). > > Richard. Ok, I think I understand. To move forward I propose this additional test case, if anything to catch regressions. Naturally, it fails when the split does not happen because the 'first' label gets incremented twice (I'll probably rename it pre application, assuming it gets approved) not once. This also means I need to change strategy for condition coverage - either, I must snapshot these splits and make a mapping table for the "original" identities or maybe run the analysis before this splitting happens. diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c b/gcc/testsuite/gcc.misc-tests/gcov-4.c index 498d299b66b..b1dc29b573a 100644 --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c @@ -110,6 +110,29 @@ lab2: return 8; /* count(1) */ } +int +test_goto3 (int i, int j) +{ + if (j) goto first; /* count(1) */ + +top: + if (i) /* count(1) */ + { + i = do_something (i); + } + else + { +first: /* count(1) */ + j = do_something (j); /* count(2) */ + if (j) /* count(2) */ + { + j = 0; /* count(1) */ + goto top; /* count(1) */ + } + } + return 16; +} + void call_goto () { @@ -117,6 +140,7 @@ call_goto () goto_val += test_goto1 (1); goto_val += test_goto2 (3); goto_val += test_goto2 (30); + goto_val += test_goto3 (0, 1); } /* Check nested if-then-else statements. */ @@ -260,7 +284,7 @@ main() call_unref (); if ((for_val1 != 12) || (for_val2 != 87) - || (goto_val != 15) + || (goto_val != 31) || (ifelse_val1 != 31) || (ifelse_val2 != 23) || (ifelse_val3 != 246) What do you think? Thanks, Jørgen
On 07/10/2022 13:45, Jørgen Kvalsvik wrote: > On 07/10/2022 08:53, Richard Biener wrote: >> On Thu, Oct 6, 2022 at 4:28 PM Jørgen Kvalsvik >> <jorgen.kvalsvik@woven-planet.global> wrote: >>> >>> On 06/10/2022 10:12, Richard Biener wrote: >>>> On Wed, Oct 5, 2022 at 2:49 PM Martin Liška <mliska@suse.cz> wrote: >>>>> >>>>> On 10/5/22 14:04, Jørgen Kvalsvik via Gcc-patches wrote: >>>>>> Edges with locus are candidates for splitting so that the edge with >>>>>> locus is the only edge out of a basic block, except when the locuses >>>>>> match. The test checks the last (non-debug) statement in a basic block, >>>>>> but this causes an unnecessary split when the edge locus go to a block >>>>>> which coincidentally has an unrelated label. Comparing the first >>>>>> statement of the destination block is the same check but does not get >>>>>> tripped up by labels. >>>>>> >>>>>> An example with source/edge/dest locus when an edge is split: >>>>>> >>>>>> 1 int fn (int a, int b, int c) { >>>>>> 2 int x = 0; >>>>>> 3 if (a && b) { >>>>>> 4 x = a; >>>>>> 5 } else { >>>>>> 6 a_: >>>>>> 7 x = (a - b); >>>>>> 8 } >>>>>> 9 >>>>>> 10 return x; >>>>>> 11 } >>>>>> >>>>>> block file line col stmt >>>>>> >>>>>> src t.c 3 10 if (a_3(D) != 0) >>>>>> edge t.c 6 1 >>>>>> dest t.c 6 1 a_: >>>>>> >>>>>> src t.c 3 13 if (b_4(D) != 0) >>>>>> edge t.c 6 1 >>>>>> dst t.c 6 1 a_: >>>>>> >>>>>> With label removed: >>>>>> >>>>>> 1 int fn (int a, int b, int c) { >>>>>> 2 int x = 0; >>>>>> 3 if (a && b) { >>>>>> 4 x = a; >>>>>> 5 } else { >>>>>> 6 // a_: <- label removed >>>>>> 7 x = (a - b); >>>>>> 8 } >>>>>> 9 >>>>>> 10 return x; >>>>>> 11 >>>>>> >>>>>> block file line col stmt >>>>>> >>>>>> src t.c 3 10 if (a_3(D) != 0) >>>>>> edge (null) 0 0 >>>>>> dest t.c 6 1 a_: >>>>>> >>>>>> src t.c 3 13 if (b_4(D) != 0) >>>>>> edge (null) 0 0 >>>>>> dst t.c 6 1 a_: >>>>>> >>>>>> and this is extract from gcov-4b.c which *should* split: >>>>>> >>>>>> 205 int >>>>>> 206 test_switch (int i, int j) >>>>>> 207 { >>>>>> 208 int result = 0; >>>>>> 209 >>>>>> 210 switch (i) /* branch(80 25) */ >>>>>> 211 /* branch(end) */ >>>>>> 212 { >>>>>> 213 case 1: >>>>>> 214 result = do_something (2); >>>>>> 215 break; >>>>>> 216 case 2: >>>>>> 217 result = do_something (1024); >>>>>> 218 break; >>>>>> 219 case 3: >>>>>> 220 case 4: >>>>>> 221 if (j == 2) /* branch(67) */ >>>>>> 222 /* branch(end) */ >>>>>> 223 return do_something (4); >>>>>> 224 result = do_something (8); >>>>>> 225 break; >>>>>> 226 default: >>>>>> 227 result = do_something (32); >>>>>> 228 switch_m++; >>>>>> 229 break; >>>>>> 230 } >>>>>> 231 return result; >>>>>> 231 } >>>>>> >>>>>> block file line col stmt >>>>>> >>>>>> src 4b.c 214 18 result_18 = do_something (2); >>>>>> edge 4b.c 215 9 >>>>>> dst 4b.c 231 10 _22 = result_3; >>>>>> >>>>>> src 4b.c 217 18 result_16 = do_something (1024); >>>>>> edge 4b.c 218 9 >>>>>> dst 4b.c 231 10 _22 = result_3; >>>>>> >>>>>> src 4b.c 224 18 result_12 = do_something (8); >>>>>> edge 4b.c 225 9 >>>>>> dst 4b.c 231 10 _22 = result_3; >>>>>> >>>>>> Note that the behaviour of comparison is preserved for the (switch) edge >>>>>> splitting case. The former case now fails the check (even though >>>>>> e->goto_locus is no longer a reserved location) because the dest matches >>>>>> the e->locus. >>>>> >>>>> It's fine, please install it. >>>>> I verified tramp3d coverage output is the same as before the patch. >>>>> >>>>> Martin >>>>> >>>>>> >>>>>> gcc/ChangeLog: >>>>>> >>>>>> * profile.cc (branch_prob): Compare edge locus to dest, not src. >>>>>> --- >>>>>> gcc/profile.cc | 18 +++++++++--------- >>>>>> 1 file changed, 9 insertions(+), 9 deletions(-) >>>>>> >>>>>> diff --git a/gcc/profile.cc b/gcc/profile.cc >>>>>> index 96121d60711..c13a79a84c2 100644 >>>>>> --- a/gcc/profile.cc >>>>>> +++ b/gcc/profile.cc >>>>>> @@ -1208,17 +1208,17 @@ branch_prob (bool thunk) >>>>>> FOR_EACH_EDGE (e, ei, bb->succs) >>>>>> { >>>>>> gimple_stmt_iterator gsi; >>>>>> - gimple *last = NULL; >>>>>> + gimple *dest = NULL; >>>>>> >>>>>> /* It may happen that there are compiler generated statements >>>>>> without a locus at all. Go through the basic block from the >>>>>> last to the first statement looking for a locus. */ >>>> >>>> The comment no longer matches the code.> >>>>>> - for (gsi = gsi_last_nondebug_bb (bb); >>>>>> + for (gsi = gsi_start_nondebug_bb (bb); >>>> >>>> ^^^ and you are looking at the branch block stmts (bb), not the destination >>>> block stmts (e->dest) >>>> >>>>>> !gsi_end_p (gsi); >>>>>> - gsi_prev_nondebug (&gsi)) >>>>>> + gsi_next_nondebug (&gsi)) >>>>>> { >>>>>> - last = gsi_stmt (gsi); >>>>>> - if (!RESERVED_LOCATION_P (gimple_location (last))) >>>>>> + dest = gsi_stmt (gsi); >>>>>> + if (!RESERVED_LOCATION_P (gimple_location (dest))) >>>>>> break; >>>>>> } >>>>>> >>>>>> @@ -1227,14 +1227,14 @@ branch_prob (bool thunk) >>>>>> Don't do that when the locuses match, so >>>>>> if (blah) goto something; >>>>>> is not computed twice. */ >>>>>> - if (last >>>>>> - && gimple_has_location (last) >>>>>> + if (dest >>>>>> + && gimple_has_location (dest) >>>>>> && !RESERVED_LOCATION_P (e->goto_locus) >>>>>> && !single_succ_p (bb) >>>>>> && (LOCATION_FILE (e->goto_locus) >>>>>> - != LOCATION_FILE (gimple_location (last)) >>>>>> + != LOCATION_FILE (gimple_location (dest)) >>>>>> || (LOCATION_LINE (e->goto_locus) >>>>>> - != LOCATION_LINE (gimple_location (last))))) >>>>>> + != LOCATION_LINE (gimple_location (dest))))) >>>> >>>> this heuristic needs to be in sync with how we insert edge counters >>>> which seems to be hidden in the MST compute (and/or edge insertion). >>>> I don't see how it's a win to disregard 'last' and only consider 'dest' here. >>>> >>>> I think the patch is wrong. Please revert if you applied it already. >>> >>> I haven't applied it yet, so unless someone beat me to it there's fortunately >>> nothing to revert. >>> >>>> I don't see how it's a win to disregard 'last' and only consider 'dest' here. >>> >>> It might not be other than that it unbreaks my condition profiling by not >>> splitting condition edges and apparently doesn't cause a regression (one caught >>> by tests anyway). That being said the heuristic may very well be wrong (as is >>> the implementation since it considers bb and not e->dest, although I'm sure I >>> tested it with e->dest at some point). >>> >>> I guess the most important question is if the if (a && b) {} {label:} edges >>> should be split in the first place. As mentioned in the patch set, the only >>> difference in the test suite happens on break in switches. I'll tinker a bit >>> more to see if I can figure out what's going on or if the heuristic can >>> otherwise be improved. >>> >>> Then, when does a block with a goto_locus edge have multiple successors? From my >>> previous testing it doesn't seem like it's the conditions make a goto_locus, but >>> it's more than just plain gotos right? When would it then have multiple >>> successors? Switches and exception handling? If that's the case then maybe the >>> heuristic can be improved by simply checking the edge type. >> >> The goto_locus of an edge is usually either the locus of the control stmt or the >> locus of the stmt the control transfers to. The most important case is for >> 'goto' stmts themselves since those are elided and become edges (thus >> 'goto_locus'). >> >> My understanding as of why we split edges at all is that we want to instrument >> different locations with different counters and since we cannot have counters on >> edges itself but have to either insert the counter on the source or >> the destination >> we in some cases have to split the edge to create an insert location >> to not falsely >> account. instrument_edges seems to simply use gsi_insert_on_edge which >> inserts with the gimple_find_edge_insert_loc logic which doesn't look >> at goto_locus >> at all. So the existing heuristic must be fragile as well. >> >> BUT - as you say, the plain goto shouldn't be subject to edge instrumentation. >> The interesting case is probably computed goto (which has multiple successors) >> and from what I can see a branch where we take the goto_locus from the >> COND_EXPRs then/else goto stmt which in theory could have different locations. >> >> I don't fully understand your requirement of not splitting edges - >> I'll just note that >> the place you are patching is not the only place where edges are split (but >> the insert location computation only sees those splits). >> >> Richard. > > Ok, I think I understand. To move forward I propose this additional test case, > if anything to catch regressions. Naturally, it fails when the split does not > happen because the 'first' label gets incremented twice (I'll probably rename it > pre application, assuming it gets approved) not once. > > This also means I need to change strategy for condition coverage - either, I > must snapshot these splits and make a mapping table for the "original" > identities or maybe run the analysis before this splitting happens. > > > diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c > b/gcc/testsuite/gcc.misc-tests/gcov-4.c > index 498d299b66b..b1dc29b573a 100644 > --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c > +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c > @@ -110,6 +110,29 @@ lab2: > return 8; /* count(1) */ > } > > +int > +test_goto3 (int i, int j) > +{ > + if (j) goto first; /* count(1) */ > + > +top: > + if (i) /* count(1) */ > + { > + i = do_something (i); > + } > + else > + { > +first: /* count(1) */ > + j = do_something (j); /* count(2) */ > + if (j) /* count(2) */ > + { > + j = 0; /* count(1) */ > + goto top; /* count(1) */ > + } > + } > + return 16; > +} > + > void > call_goto () > { > @@ -117,6 +140,7 @@ call_goto () > goto_val += test_goto1 (1); > goto_val += test_goto2 (3); > goto_val += test_goto2 (30); > + goto_val += test_goto3 (0, 1); > } > > /* Check nested if-then-else statements. */ > @@ -260,7 +284,7 @@ main() > call_unref (); > if ((for_val1 != 12) > || (for_val2 != 87) > - || (goto_val != 15) > + || (goto_val != 31) > || (ifelse_val1 != 31) > || (ifelse_val2 != 23) > || (ifelse_val3 != 246) > > What do you think? > > Thanks, > Jørgen Hello, After tinkering a bit more I figured out a patch I could do to merge these splits in the condition coverage code, rather than relying on the splits not happening. I still think the tests are a good addition, but there's no longer a reason for me to change the splitting heuristics. Should I prepare a separate patch set for the tests? Thanks, Jørgen
On Tue, Oct 11, 2022 at 12:57 PM Jørgen Kvalsvik <jorgen.kvalsvik@woven-planet.global> wrote: > > On 07/10/2022 13:45, Jørgen Kvalsvik wrote: > > On 07/10/2022 08:53, Richard Biener wrote: > >> On Thu, Oct 6, 2022 at 4:28 PM Jørgen Kvalsvik > >> <jorgen.kvalsvik@woven-planet.global> wrote: > >>> > >>> On 06/10/2022 10:12, Richard Biener wrote: > >>>> On Wed, Oct 5, 2022 at 2:49 PM Martin Liška <mliska@suse.cz> wrote: > >>>>> > >>>>> On 10/5/22 14:04, Jørgen Kvalsvik via Gcc-patches wrote: > >>>>>> Edges with locus are candidates for splitting so that the edge with > >>>>>> locus is the only edge out of a basic block, except when the locuses > >>>>>> match. The test checks the last (non-debug) statement in a basic block, > >>>>>> but this causes an unnecessary split when the edge locus go to a block > >>>>>> which coincidentally has an unrelated label. Comparing the first > >>>>>> statement of the destination block is the same check but does not get > >>>>>> tripped up by labels. > >>>>>> > >>>>>> An example with source/edge/dest locus when an edge is split: > >>>>>> > >>>>>> 1 int fn (int a, int b, int c) { > >>>>>> 2 int x = 0; > >>>>>> 3 if (a && b) { > >>>>>> 4 x = a; > >>>>>> 5 } else { > >>>>>> 6 a_: > >>>>>> 7 x = (a - b); > >>>>>> 8 } > >>>>>> 9 > >>>>>> 10 return x; > >>>>>> 11 } > >>>>>> > >>>>>> block file line col stmt > >>>>>> > >>>>>> src t.c 3 10 if (a_3(D) != 0) > >>>>>> edge t.c 6 1 > >>>>>> dest t.c 6 1 a_: > >>>>>> > >>>>>> src t.c 3 13 if (b_4(D) != 0) > >>>>>> edge t.c 6 1 > >>>>>> dst t.c 6 1 a_: > >>>>>> > >>>>>> With label removed: > >>>>>> > >>>>>> 1 int fn (int a, int b, int c) { > >>>>>> 2 int x = 0; > >>>>>> 3 if (a && b) { > >>>>>> 4 x = a; > >>>>>> 5 } else { > >>>>>> 6 // a_: <- label removed > >>>>>> 7 x = (a - b); > >>>>>> 8 } > >>>>>> 9 > >>>>>> 10 return x; > >>>>>> 11 > >>>>>> > >>>>>> block file line col stmt > >>>>>> > >>>>>> src t.c 3 10 if (a_3(D) != 0) > >>>>>> edge (null) 0 0 > >>>>>> dest t.c 6 1 a_: > >>>>>> > >>>>>> src t.c 3 13 if (b_4(D) != 0) > >>>>>> edge (null) 0 0 > >>>>>> dst t.c 6 1 a_: > >>>>>> > >>>>>> and this is extract from gcov-4b.c which *should* split: > >>>>>> > >>>>>> 205 int > >>>>>> 206 test_switch (int i, int j) > >>>>>> 207 { > >>>>>> 208 int result = 0; > >>>>>> 209 > >>>>>> 210 switch (i) /* branch(80 25) */ > >>>>>> 211 /* branch(end) */ > >>>>>> 212 { > >>>>>> 213 case 1: > >>>>>> 214 result = do_something (2); > >>>>>> 215 break; > >>>>>> 216 case 2: > >>>>>> 217 result = do_something (1024); > >>>>>> 218 break; > >>>>>> 219 case 3: > >>>>>> 220 case 4: > >>>>>> 221 if (j == 2) /* branch(67) */ > >>>>>> 222 /* branch(end) */ > >>>>>> 223 return do_something (4); > >>>>>> 224 result = do_something (8); > >>>>>> 225 break; > >>>>>> 226 default: > >>>>>> 227 result = do_something (32); > >>>>>> 228 switch_m++; > >>>>>> 229 break; > >>>>>> 230 } > >>>>>> 231 return result; > >>>>>> 231 } > >>>>>> > >>>>>> block file line col stmt > >>>>>> > >>>>>> src 4b.c 214 18 result_18 = do_something (2); > >>>>>> edge 4b.c 215 9 > >>>>>> dst 4b.c 231 10 _22 = result_3; > >>>>>> > >>>>>> src 4b.c 217 18 result_16 = do_something (1024); > >>>>>> edge 4b.c 218 9 > >>>>>> dst 4b.c 231 10 _22 = result_3; > >>>>>> > >>>>>> src 4b.c 224 18 result_12 = do_something (8); > >>>>>> edge 4b.c 225 9 > >>>>>> dst 4b.c 231 10 _22 = result_3; > >>>>>> > >>>>>> Note that the behaviour of comparison is preserved for the (switch) edge > >>>>>> splitting case. The former case now fails the check (even though > >>>>>> e->goto_locus is no longer a reserved location) because the dest matches > >>>>>> the e->locus. > >>>>> > >>>>> It's fine, please install it. > >>>>> I verified tramp3d coverage output is the same as before the patch. > >>>>> > >>>>> Martin > >>>>> > >>>>>> > >>>>>> gcc/ChangeLog: > >>>>>> > >>>>>> * profile.cc (branch_prob): Compare edge locus to dest, not src. > >>>>>> --- > >>>>>> gcc/profile.cc | 18 +++++++++--------- > >>>>>> 1 file changed, 9 insertions(+), 9 deletions(-) > >>>>>> > >>>>>> diff --git a/gcc/profile.cc b/gcc/profile.cc > >>>>>> index 96121d60711..c13a79a84c2 100644 > >>>>>> --- a/gcc/profile.cc > >>>>>> +++ b/gcc/profile.cc > >>>>>> @@ -1208,17 +1208,17 @@ branch_prob (bool thunk) > >>>>>> FOR_EACH_EDGE (e, ei, bb->succs) > >>>>>> { > >>>>>> gimple_stmt_iterator gsi; > >>>>>> - gimple *last = NULL; > >>>>>> + gimple *dest = NULL; > >>>>>> > >>>>>> /* It may happen that there are compiler generated statements > >>>>>> without a locus at all. Go through the basic block from the > >>>>>> last to the first statement looking for a locus. */ > >>>> > >>>> The comment no longer matches the code.> > >>>>>> - for (gsi = gsi_last_nondebug_bb (bb); > >>>>>> + for (gsi = gsi_start_nondebug_bb (bb); > >>>> > >>>> ^^^ and you are looking at the branch block stmts (bb), not the destination > >>>> block stmts (e->dest) > >>>> > >>>>>> !gsi_end_p (gsi); > >>>>>> - gsi_prev_nondebug (&gsi)) > >>>>>> + gsi_next_nondebug (&gsi)) > >>>>>> { > >>>>>> - last = gsi_stmt (gsi); > >>>>>> - if (!RESERVED_LOCATION_P (gimple_location (last))) > >>>>>> + dest = gsi_stmt (gsi); > >>>>>> + if (!RESERVED_LOCATION_P (gimple_location (dest))) > >>>>>> break; > >>>>>> } > >>>>>> > >>>>>> @@ -1227,14 +1227,14 @@ branch_prob (bool thunk) > >>>>>> Don't do that when the locuses match, so > >>>>>> if (blah) goto something; > >>>>>> is not computed twice. */ > >>>>>> - if (last > >>>>>> - && gimple_has_location (last) > >>>>>> + if (dest > >>>>>> + && gimple_has_location (dest) > >>>>>> && !RESERVED_LOCATION_P (e->goto_locus) > >>>>>> && !single_succ_p (bb) > >>>>>> && (LOCATION_FILE (e->goto_locus) > >>>>>> - != LOCATION_FILE (gimple_location (last)) > >>>>>> + != LOCATION_FILE (gimple_location (dest)) > >>>>>> || (LOCATION_LINE (e->goto_locus) > >>>>>> - != LOCATION_LINE (gimple_location (last))))) > >>>>>> + != LOCATION_LINE (gimple_location (dest))))) > >>>> > >>>> this heuristic needs to be in sync with how we insert edge counters > >>>> which seems to be hidden in the MST compute (and/or edge insertion). > >>>> I don't see how it's a win to disregard 'last' and only consider 'dest' here. > >>>> > >>>> I think the patch is wrong. Please revert if you applied it already. > >>> > >>> I haven't applied it yet, so unless someone beat me to it there's fortunately > >>> nothing to revert. > >>> > >>>> I don't see how it's a win to disregard 'last' and only consider 'dest' here. > >>> > >>> It might not be other than that it unbreaks my condition profiling by not > >>> splitting condition edges and apparently doesn't cause a regression (one caught > >>> by tests anyway). That being said the heuristic may very well be wrong (as is > >>> the implementation since it considers bb and not e->dest, although I'm sure I > >>> tested it with e->dest at some point). > >>> > >>> I guess the most important question is if the if (a && b) {} {label:} edges > >>> should be split in the first place. As mentioned in the patch set, the only > >>> difference in the test suite happens on break in switches. I'll tinker a bit > >>> more to see if I can figure out what's going on or if the heuristic can > >>> otherwise be improved. > >>> > >>> Then, when does a block with a goto_locus edge have multiple successors? From my > >>> previous testing it doesn't seem like it's the conditions make a goto_locus, but > >>> it's more than just plain gotos right? When would it then have multiple > >>> successors? Switches and exception handling? If that's the case then maybe the > >>> heuristic can be improved by simply checking the edge type. > >> > >> The goto_locus of an edge is usually either the locus of the control stmt or the > >> locus of the stmt the control transfers to. The most important case is for > >> 'goto' stmts themselves since those are elided and become edges (thus > >> 'goto_locus'). > >> > >> My understanding as of why we split edges at all is that we want to instrument > >> different locations with different counters and since we cannot have counters on > >> edges itself but have to either insert the counter on the source or > >> the destination > >> we in some cases have to split the edge to create an insert location > >> to not falsely > >> account. instrument_edges seems to simply use gsi_insert_on_edge which > >> inserts with the gimple_find_edge_insert_loc logic which doesn't look > >> at goto_locus > >> at all. So the existing heuristic must be fragile as well. > >> > >> BUT - as you say, the plain goto shouldn't be subject to edge instrumentation. > >> The interesting case is probably computed goto (which has multiple successors) > >> and from what I can see a branch where we take the goto_locus from the > >> COND_EXPRs then/else goto stmt which in theory could have different locations. > >> > >> I don't fully understand your requirement of not splitting edges - > >> I'll just note that > >> the place you are patching is not the only place where edges are split (but > >> the insert location computation only sees those splits). > >> > >> Richard. > > > > Ok, I think I understand. To move forward I propose this additional test case, > > if anything to catch regressions. Naturally, it fails when the split does not > > happen because the 'first' label gets incremented twice (I'll probably rename it > > pre application, assuming it gets approved) not once. > > > > This also means I need to change strategy for condition coverage - either, I > > must snapshot these splits and make a mapping table for the "original" > > identities or maybe run the analysis before this splitting happens. > > > > > > diff --git a/gcc/testsuite/gcc.misc-tests/gcov-4.c > > b/gcc/testsuite/gcc.misc-tests/gcov-4.c > > index 498d299b66b..b1dc29b573a 100644 > > --- a/gcc/testsuite/gcc.misc-tests/gcov-4.c > > +++ b/gcc/testsuite/gcc.misc-tests/gcov-4.c > > @@ -110,6 +110,29 @@ lab2: > > return 8; /* count(1) */ > > } > > > > +int > > +test_goto3 (int i, int j) > > +{ > > + if (j) goto first; /* count(1) */ > > + > > +top: > > + if (i) /* count(1) */ > > + { > > + i = do_something (i); > > + } > > + else > > + { > > +first: /* count(1) */ > > + j = do_something (j); /* count(2) */ > > + if (j) /* count(2) */ > > + { > > + j = 0; /* count(1) */ > > + goto top; /* count(1) */ > > + } > > + } > > + return 16; > > +} > > + > > void > > call_goto () > > { > > @@ -117,6 +140,7 @@ call_goto () > > goto_val += test_goto1 (1); > > goto_val += test_goto2 (3); > > goto_val += test_goto2 (30); > > + goto_val += test_goto3 (0, 1); > > } > > > > /* Check nested if-then-else statements. */ > > @@ -260,7 +284,7 @@ main() > > call_unref (); > > if ((for_val1 != 12) > > || (for_val2 != 87) > > - || (goto_val != 15) > > + || (goto_val != 31) > > || (ifelse_val1 != 31) > > || (ifelse_val2 != 23) > > || (ifelse_val3 != 246) > > > > What do you think? > > > > Thanks, > > Jørgen > > Hello, > > After tinkering a bit more I figured out a patch I could do to merge these > splits in the condition coverage code, rather than relying on the splits not > happening. I still think the tests are a good addition, but there's no longer a > reason for me to change the splitting heuristics. > > Should I prepare a separate patch set for the tests? Yes, enhancing test coverage is always good and should be done separately. Thanks a lot, Richard. > > Thanks, > Jørgen
diff --git a/gcc/profile.cc b/gcc/profile.cc index 96121d60711..c13a79a84c2 100644 --- a/gcc/profile.cc +++ b/gcc/profile.cc @@ -1208,17 +1208,17 @@ branch_prob (bool thunk) FOR_EACH_EDGE (e, ei, bb->succs) { gimple_stmt_iterator gsi; - gimple *last = NULL; + gimple *dest = NULL; /* It may happen that there are compiler generated statements without a locus at all. Go through the basic block from the last to the first statement looking for a locus. */ - for (gsi = gsi_last_nondebug_bb (bb); + for (gsi = gsi_start_nondebug_bb (bb); !gsi_end_p (gsi); - gsi_prev_nondebug (&gsi)) + gsi_next_nondebug (&gsi)) { - last = gsi_stmt (gsi); - if (!RESERVED_LOCATION_P (gimple_location (last))) + dest = gsi_stmt (gsi); + if (!RESERVED_LOCATION_P (gimple_location (dest))) break; } @@ -1227,14 +1227,14 @@ branch_prob (bool thunk) Don't do that when the locuses match, so if (blah) goto something; is not computed twice. */ - if (last - && gimple_has_location (last) + if (dest + && gimple_has_location (dest) && !RESERVED_LOCATION_P (e->goto_locus) && !single_succ_p (bb) && (LOCATION_FILE (e->goto_locus) - != LOCATION_FILE (gimple_location (last)) + != LOCATION_FILE (gimple_location (dest)) || (LOCATION_LINE (e->goto_locus) - != LOCATION_LINE (gimple_location (last))))) + != LOCATION_LINE (gimple_location (dest))))) { basic_block new_bb = split_edge (e); edge ne = single_succ_edge (new_bb);