From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 279E73947C24 for ; Fri, 28 Jan 2022 10:11:47 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 279E73947C24 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id E5C87210FE; Fri, 28 Jan 2022 10:11:45 +0000 (UTC) Received: from murzim.suse.de (murzim.suse.de [10.160.4.192]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id E0837A3B83; Fri, 28 Jan 2022 10:11:45 +0000 (UTC) Date: Fri, 28 Jan 2022 11:11:45 +0100 (CET) From: Richard Biener To: Jakub Jelinek cc: Jeff Law , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] cfgrtl: Fix up locus comparison in unique_locus_on_edge_between_p [PR104237] In-Reply-To: <20220128091805.GM2646553@tucnak> Message-ID: <7r6qr68r-2qo4-nr31-7851-q521773438p9@fhfr.qr> References: <20220128091805.GM2646553@tucnak> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 28 Jan 2022 10:11:48 -0000 On Fri, 28 Jan 2022, Jakub Jelinek wrote: > Hi! > > The testcase in the PR (not included for the testsuite because we don't > have an (easy) way to -fcompare-debug LTO, we'd need 2 compilations/linking, > one with -g and one with -g0 and -fdump-rtl-final= at the end of lto1 > and compare that) has different code generation for -g vs. -g0. > > The difference appears during expansion, where we have a goto_locus > that is at -O0 compared to the INSN_LOCATION of the previous and next insn > across an edge. With -g0 the locations are equal and so no nop is added. > With -g the locations aren't equal and so a nop is added holding that > location. > > The reason for the different location is in the way how we stream in > locations by lto1. > We have lto_location_cache::apply_location_cache that is called with some > set of expanded locations, qsorts them, creates location_t's for those > and remembers the last expanded location. > lto_location_cache::input_location_and_block when read in expanded_location > is equal to the last expanded location just reuses the last location_t > (or adds/changes/removes LOCATION_BLOCK in it), when it is not queues > it for next apply_location_cache. Now, when streaming in -g input, we can > see extra locations that don't appear with -g0, and if we are unlucky > enough, those can be sorted last during apply_location_cache and affect > what locations are used from the single entry cache next. > In particular, second apply_location_cache with non-empty loc_cache in > the testcase has 14 locations with -g0 and 16 with -g and those 2 extra > ones sort both last (they are the same). The last one from -g0 then > appears to be input_location_and_block sourced again, for -g0 triggers > the single entry cache, while for -g it doesn't and so apply_location_cache > will create for it another location_t with the same content. > > The following patch fixes it by comparing everything we care about the > location instead (well, better in addition) to a simple location_t == > location_t check. I think we don't care about the sysp flag for debug > info... > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? OK. Thanks, Richard. > 2022-01-28 Jakub Jelinek > > PR lto/104237 > * cfgrtl.cc (loc_equal): New function. > (unique_locus_on_edge_between_p): Use it. > > --- gcc/cfgrtl.cc.jj 2022-01-18 11:58:58.947991128 +0100 > +++ gcc/cfgrtl.cc 2022-01-27 19:32:13.949937750 +0100 > @@ -778,6 +778,29 @@ rtl_split_block (basic_block bb, void *i > return new_bb; > } > > +/* Return true if LOC1 and LOC2 are equivalent for > + unique_locus_on_edge_between_p purposes. */ > + > +static bool > +loc_equal (location_t loc1, location_t loc2) > +{ > + if (loc1 == loc2) > + return true; > + > + expanded_location loce1 = expand_location (loc1); > + expanded_location loce2 = expand_location (loc2); > + > + if (loce1.line != loce2.line > + || loce1.column != loce2.column > + || loce1.data != loce2.data) > + return false; > + if (loce1.file == loce2.file) > + return true; > + return (loce1.file != NULL > + && loce2.file != NULL > + && filename_cmp (loce1.file, loce2.file) == 0); > +} > + > /* Return true if the single edge between blocks A and B is the only place > in RTL which holds some unique locus. */ > > @@ -796,7 +819,7 @@ unique_locus_on_edge_between_p (basic_bl > while (insn != end && (!NONDEBUG_INSN_P (insn) || !INSN_HAS_LOCATION (insn))) > insn = PREV_INSN (insn); > > - if (insn != end && INSN_LOCATION (insn) == goto_locus) > + if (insn != end && loc_equal (INSN_LOCATION (insn), goto_locus)) > return false; > > /* Then scan block B forward. */ > @@ -808,7 +831,7 @@ unique_locus_on_edge_between_p (basic_bl > insn = NEXT_INSN (insn); > > if (insn != end && INSN_HAS_LOCATION (insn) > - && INSN_LOCATION (insn) == goto_locus) > + && loc_equal (INSN_LOCATION (insn), goto_locus)) > return false; > } > > > Jakub > > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)