From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com [IPv6:2a00:1450:4864:20::52b]) by sourceware.org (Postfix) with ESMTPS id D5876389EC4B for ; Fri, 7 Oct 2022 06:53:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org D5876389EC4B 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-ed1-x52b.google.com with SMTP id w10so5753932edd.4 for ; Thu, 06 Oct 2022 23:53:15 -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=fuvLJg4u3u9lnu5sln7r5o5v2chVvIKppjhaZQaIdVc=; b=j0dXwdjSBsv9IE5eQjeANj5eah+8L5fl0S8aWC+rnXvgYwAhB/C8F4PG7gtVrJkrPO 03JxNPCveKMrC/eckbVqs7xszOnyhsiw67FuwnzEZl+lVpERDJsawBS1HHmPHiSkxnJS MAmxfYLk1sYaKxPD6opij7ETHsdy+gYrJ+pTzTTp31O/+D58jhECf7s6ztXtN4CYx0r6 47E1uursaKSskKKMyDA1YcFQv2mFlIYjNMnk4fAdY+AJgd6qir1wgMhBXvUNgAg+3NM5 Ec2pIElrTw6HEGBz0xOwYuBdADJHq3OpXAqBHMEURbfydqC5OlNhtAj+HO/CLetUeHqZ qA1w== 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=fuvLJg4u3u9lnu5sln7r5o5v2chVvIKppjhaZQaIdVc=; b=j0Aa4XhbCQscFShOrtWQM2Sk3j7zFdbW7lpcePEIbHLNctjs/OEHHN5MT5/1WBR7lY 2k0uNrgPfn/SAJsgsHnd/NGtlJb8HtUs+guXeHGvDyu9CZn9kV5z9ku/pTEqgpVeRYun ga+wYcmx9+DfbGTCAVFVXPhim/HYKtmsrqiZ7CTClY8P73hBdvdEhYvUpIxK0YMqw4IF WlWAUtOiDCh+kpdRXgVmC/gfu03Y6lSSSDblpukN607RIHpTDCXbdmoogLPMVwU24i5p qQUuFxjC2RNaHDi6eJwKopgJIu77Si6YRxRQNZj5A87KQY2enojCTpNvqkXydbJwPEeD L61A== X-Gm-Message-State: ACrzQf0N6yhS/MhS+OBRG0V74Ll7J4hxFdhZdSc2GCGcwxOFiYsvKDYS aBC1z9ghA7ZNM7dl91eFC6h2jeEAw9Nz6yhxOzQ= X-Google-Smtp-Source: AMsMyM5hzFb/hj24BZX6yteZSPEjbVJT0EFCuMnociGtdAA1HqxELQ09KRnOHxajC4GE6y8+vCDq35r1BrcIcq/L7QQ= X-Received: by 2002:a05:6402:3584:b0:458:d3fa:fb89 with SMTP id y4-20020a056402358400b00458d3fafb89mr3281170edc.218.1665125594524; Thu, 06 Oct 2022 23:53:14 -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: From: Richard Biener Date: Fri, 7 Oct 2022 08:53:02 +0200 Message-ID: Subject: Re: [PATCH 2/2] Split edge when edge locus and dest don't match To: =?UTF-8?Q?J=C3=B8rgen_Kvalsvik?= Cc: =?UTF-8?Q?Martin_Li=C5=A1ka?= , 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 Thu, Oct 6, 2022 at 4:28 PM J=C3=B8rgen Kvalsvik wrote: > > On 06/10/2022 10:12, Richard Biener wrote: > > 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 bloc= k, > >>> but this causes an unnecessary split when the edge locus go to a bloc= k > >>> 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) e= dge > >>> splitting case. The former case now fails the check (even though > >>> e->goto_locus is no longer a reserved location) because the dest matc= hes > >>> 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 s= rc. > >>> --- > >>> 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 statem= ents > >>> without a locus at all. Go through the basic block fr= om 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 destina= tion > > 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'= 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 fortuna= tely > 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 (a= s is > the implementation since it considers bb and not e->dest, although I'm su= re I > tested it with e->dest at some point). > > I guess the most important question is if the if (a && b) {} {label:} edg= es > should be split in the first place. As mentioned in the patch set, the on= ly > 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_loc= us, 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 mayb= e 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 o= r 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 instrum= ent different locations with different counters and since we cannot have counte= rs 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 instrumentati= on. The interesting case is probably computed goto (which has multiple successo= rs) 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 locatio= ns. 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=C3=B8rgen > > > > > > Thanks, > > Richard. > > > >>> { > >>> basic_block new_bb =3D split_edge (e); > >>> edge ne =3D single_succ_edge (new_bb); > >>