From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com [IPv6:2a00:1450:4864:20::12f]) by sourceware.org (Postfix) with ESMTPS id 86D0638983BC for ; Thu, 6 Oct 2022 14:28:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 86D0638983BC Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=woven-planet.global Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=woven-planet.global Received: by mail-lf1-x12f.google.com with SMTP id o7so2972692lfk.7 for ; Thu, 06 Oct 2022 07:28:55 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=woven-planet.global; s=google; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=y2lZSt10LP6bkdgijFtUr7SiwOKDAflt9CRVGaH7EcI=; b=FwGusmg5/zutN+gTIBDsC+zDiboPkkgwDLhm+TWYpZRi2tBUp0Q/l/QYSwHKqt22T4 tNbgZ9f2F7/TEIDA1kjkUd9eVQQKMFtCdgkAMoYUE5pcdnYjiKnEzOs2zKurVxrst1T3 DQBkqQ4lnLnBwSsaLzaKzpqDEB1AMqkPT2rcw= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=y2lZSt10LP6bkdgijFtUr7SiwOKDAflt9CRVGaH7EcI=; b=4UzpcRXXbLRYNIVxy+HpnKSMG5TvEQ0lj1gFKaTpnvJv98DUoCIJb7Y1oAIX4V0euB v0mxirFZ7b3S+mqoHxRBxiKnRyeel6/STCcuo579zc2ObjTvZRqfI+9z6BgYNCIdI2u6 dZ78pjLpsdPF1xCDKHK+gqQIWDiwu+EJfGwKWcyA2kuNe9ewDMZSJ2nzcY4kbprnnVCZ Cg7kHjQfRGXXVb1fTNtKLmSNgABA+aa1duLTwf7jwejlmzP0Kza4yRCP2o3WAVqp32IH yfZ1tRd9gl8AVQ/7UdM/pZM0yGH1Rn4FOZn9xzNRH3hxeuTWQexkmeK8zWAV9xqxXF7I aQNg== X-Gm-Message-State: ACrzQf2nQLd7tUJr+T6hWnUIGJpYRvGjsvRmo/yhjIyJyN3FunRuyBjg XovuSl6z0X8mow0Db7wHhue9xg== X-Google-Smtp-Source: AMsMyM6J69xYD/0um0LUeJa/UzMOGn1/0AcA4ZBFlnG+aA2vzyW/8AuNDSsbyvvXhfKkIzCiNS1HDw== X-Received: by 2002:a05:6512:ba4:b0:4a2:6e4a:6ffc with SMTP id b36-20020a0565120ba400b004a26e4a6ffcmr79961lfv.11.1665066533600; Thu, 06 Oct 2022 07:28:53 -0700 (PDT) Received: from [192.168.68.118] ([178.232.205.76]) by smtp.gmail.com with ESMTPSA id bi18-20020a0565120e9200b00494a2a0f6cfsm2713073lfb.183.2022.10.06.07.28.52 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 06 Oct 2022 07:28:53 -0700 (PDT) Message-ID: Date: Thu, 6 Oct 2022 16:28:50 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.13.1 Subject: Re: [PATCH 2/2] Split edge when edge locus and dest don't match Content-Language: en-US To: Richard Biener , =?UTF-8?Q?Martin_Li=c5=a1ka?= Cc: gcc-patches@gcc.gnu.org References: <20221005120403.68935-1-jorgen.kvalsvik@woven-planet.global> <20221005120403.68935-3-jorgen.kvalsvik@woven-planet.global> <97cd4512-9515-1860-266d-a0bfc809e85f@suse.cz> From: =?UTF-8?Q?J=c3=b8rgen_Kvalsvik?= In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,NICE_REPLY_A,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 List-Id: On 06/10/2022 10:12, Richard Biener wrote: > On Wed, Oct 5, 2022 at 2:49 PM Martin Liška 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); >>