From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ej1-x630.google.com (mail-ej1-x630.google.com [IPv6:2a00:1450:4864:20::630]) by sourceware.org (Postfix) with ESMTPS id D0DA0385C41B for ; Thu, 6 Oct 2022 08:12:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D0DA0385C41B Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-ej1-x630.google.com with SMTP id ot12so2881170ejb.1 for ; Thu, 06 Oct 2022 01:12:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date; bh=aUwGtOhgbV08mXfhgriggkftH745CubJQEu8iHabx4Q=; b=lF1FhFkysXOM8ZAr0VgwAweUNN+b6VEaWwdyE2XebUpSjVx2F65BBTufXcqTYreCcK FVsfwkjUNNMcUSA0AdUE5BbJUMkC4Fe66dVPeeqEW2Hf7DhuUOLYUMege0uT+xUfdz6a 4wZo9NURiSTe2pkGhd2803wyox/KEICA0HeVVdJStN/yalYU6kafDGklHKMUB+95vlwM BTysEQr6Yew0tMANjTMcf8TawLjwzA8N7O5YEw0lSuTD0jP1AniYy+Yc1BOMc876U/DU /FD8dpmG8dGanb1Sww6ncsAOBnzwSxMKkn56ra7MAazhMo4aYPc0s2dxjhNTGtYL5d+I HlrA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date; bh=aUwGtOhgbV08mXfhgriggkftH745CubJQEu8iHabx4Q=; b=dFHRyJxEWT9M0GrzFo+FcQOn25bR6UKo4OTVEuNKSrLQ7BOBeZjekGhE1sjAOh+4EL +UvvLPC6FcOVKgjf/ZJQkk5b9+MC7op8FFh7VsRCUutHOerbtpWNKIAEILZgVSt+kfpn D+85kldozWMekcYK4lLo2DzLXm63bTk4+yc1QprU6xhrAQ7eSLd+DGktT0zFAqayjGUf OsZY54fOEUjHCakMTLNrmKjBib19ifcrQrelvn+A04vB7sYl9SVEch/FT5ZhTODfm/Z3 EyiTGVllAWVc47+uL0s0Ml7ZjMzVZ0ledWj6gXBh9RjqG4PXS1wEJR+djbd9Ssepophz prCw== X-Gm-Message-State: ACrzQf3XhmH9pFkdvOlu6mhicqhTHAELwoH4zChJEp6Kfa+43qNpItVj jsQal2u40HwyDQbYG3qKF+y9WIVptiW9dC8+yELoQ9hE X-Google-Smtp-Source: AMsMyM5e6rJshfjfGbwHAvHu00+NTe72J3EQYyD2PxL1Y4glqg2mSBcndidlwkt7YW8TBDGmgfrweirs5XcSITCl+Y0= X-Received: by 2002:a17:907:980a:b0:78d:282f:1607 with SMTP id ji10-20020a170907980a00b0078d282f1607mr2889380ejc.511.1665043976608; Thu, 06 Oct 2022 01:12:56 -0700 (PDT) MIME-Version: 1.0 References: <20221005120403.68935-1-jorgen.kvalsvik@woven-planet.global> <20221005120403.68935-3-jorgen.kvalsvik@woven-planet.global> <97cd4512-9515-1860-266d-a0bfc809e85f@suse.cz> In-Reply-To: <97cd4512-9515-1860-266d-a0bfc809e85f@suse.cz> From: Richard Biener Date: Thu, 6 Oct 2022 10:12:44 +0200 Message-ID: Subject: Re: [PATCH 2/2] Split edge when edge locus and dest don't match To: =?UTF-8?Q?Martin_Li=C5=A1ka?= Cc: =?UTF-8?Q?J=C3=B8rgen_Kvalsvik?= , gcc-patches@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-8.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Wed, Oct 5, 2022 at 2:49 PM Martin Li=C5=A1ka wrote: > > On 10/5/22 14:04, J=C3=B8rgen 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 =3D 0; > > 3 if (a && b) { > > 4 x =3D a; > > 5 } else { > > 6 a_: > > 7 x =3D (a - b); > > 8 } > > 9 > > 10 return x; > > 11 } > > > > block file line col stmt > > > > src t.c 3 10 if (a_3(D) !=3D 0) > > edge t.c 6 1 > > dest t.c 6 1 a_: > > > > src t.c 3 13 if (b_4(D) !=3D 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 =3D 0; > > 3 if (a && b) { > > 4 x =3D a; > > 5 } else { > > 6 // a_: <- label removed > > 7 x =3D (a - b); > > 8 } > > 9 > > 10 return x; > > 11 > > > > block file line col stmt > > > > src t.c 3 10 if (a_3(D) !=3D 0) > > edge (null) 0 0 > > dest t.c 6 1 a_: > > > > src t.c 3 13 if (b_4(D) !=3D 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 =3D 0; > > 209 > > 210 switch (i) /* branch(80 25) */ > > 211 /* branch(end) */ > > 212 { > > 213 case 1: > > 214 result =3D do_something (2); > > 215 break; > > 216 case 2: > > 217 result =3D do_something (1024); > > 218 break; > > 219 case 3: > > 220 case 4: > > 221 if (j =3D=3D 2) /* branch(67) */ > > 222 /* branch(end) */ > > 223 return do_something (4); > > 224 result =3D do_something (8); > > 225 break; > > 226 default: > > 227 result =3D 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 =3D do_something (2); > > edge 4b.c 215 9 > > dst 4b.c 231 10 _22 =3D result_3; > > > > src 4b.c 217 18 result_16 =3D do_something (1024); > > edge 4b.c 218 9 > > dst 4b.c 231 10 _22 =3D result_3; > > > > src 4b.c 224 18 result_12 =3D do_something (8); > > edge 4b.c 225 9 > > dst 4b.c 231 10 _22 =3D result_3; > > > > Note that the behaviour of comparison is preserved for the (switch) edg= e > > splitting case. The former case now fails the check (even though > > e->goto_locus is no longer a reserved location) because the dest matche= s > > 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 =3D NULL; > > + gimple *dest =3D NULL; > > > > /* It may happen that there are compiler generated statemen= ts > > 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 =3D gsi_last_nondebug_bb (bb); > > + for (gsi =3D 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 =3D gsi_stmt (gsi); > > - if (!RESERVED_LOCATION_P (gimple_location (last))) > > + dest =3D 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) > > - !=3D LOCATION_FILE (gimple_location (last)) > > + !=3D LOCATION_FILE (gimple_location (dest)) > > || (LOCATION_LINE (e->goto_locus) > > - !=3D LOCATION_LINE (gimple_location (last))))) > > + !=3D 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' her= e. I think the patch is wrong. Please revert if you applied it already. Thanks, Richard. > > { > > basic_block new_bb =3D split_edge (e); > > edge ne =3D single_succ_edge (new_bb); >