From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rock.gnat.com (rock.gnat.com [IPv6:2620:20:4000:0:a9e:1ff:fe9b:1d1]) by sourceware.org (Postfix) with ESMTPS id 5D6843858C60 for ; Thu, 2 Feb 2023 11:16:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5D6843858C60 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=adacore.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=adacore.com Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id 4505611653B; Thu, 2 Feb 2023 06:16:43 -0500 (EST) X-Virus-Scanned: Debian amavisd-new at gnat.com Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id 3mvEMAIUzaVL; Thu, 2 Feb 2023 06:16:43 -0500 (EST) Received: from free.home (tron.gnat.com [IPv6:2620:20:4000:0:46a8:42ff:fe0e:e294]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by rock.gnat.com (Postfix) with ESMTPS id DC7C71164AA; Thu, 2 Feb 2023 06:16:42 -0500 (EST) Received: from livre (livre.home [172.31.160.2]) by free.home (8.15.2/8.15.2) with ESMTPS id 312BGKob780800 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 2 Feb 2023 08:16:22 -0300 From: Alexandre Oliva To: Jakub Jelinek Cc: Richard Biener , Jeff Law , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] sched-deps, cselib: Fix up some -fcompare-debug issues and regressions [PR108463] Organization: Free thinker, does not speak for AdaCore References: Errors-To: aoliva@lxoliva.fsfla.org Date: Thu, 02 Feb 2023 08:16:20 -0300 In-Reply-To: (Jakub Jelinek's message of "Sat, 28 Jan 2023 00:18:49 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Scanned-By: MIMEDefang 2.84 X-Spam-Status: No, score=-6.2 required=5.0 tests=BAYES_00,KAM_DMARC_STATUS,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 Jan 27, 2023, Jakub Jelinek wrote: > Now, 1) is precondition of 2), we can only subst the VALUEs if we > have actually looked the address up, but as can be seen on that testcase, > we are relying on at least the 1) to be done because we subst the values > later on even on DEBUG_INSNs and actually use those when needed. Ugh. That definitely rings a bell, now that you mention it. I wish I had recalled that when I saw the "obvious" opportunity for optimization :-/ > So, I (as done in the patch below) reinstalled the 1) and not 2) for > DEBUG_INSNs. Thanks! > I've spent a day debugging that and found the problem is that as documented > in a large comment in cselib.cc above n_useless_values variable definition, > we spend quite a few effort on making sure that VALUEs created on > DEBUG_INSNs don't affect the cselib decisions for non-DEBUG_INSNs such as > pruning of useless values etc., but if a VALUE created that way is then > looked up/needed from non-DEBUG_INSNs, we promote it to non-debug. *nod* > The reason for -fcompare-debug failure is that there is one large DEBUG_INSN > with 16 MEMs in it mostly with addresses that so far didn't appear in the IL > otherwise. Later on, we see an instruction storing into MEM destination > and invalidate that MEM. Aha! > Unfortunately, n_useless_values which in my understanding should be always > the same between -g and -g0 compilations diverges, has 3 more useless values > for -g. Yeah, that's not good. > Now, these were initially VALUEs created for DEBUG_INSN lookups. As I said, > cselib.cc has code to promote such VALUEs (well, their location elements) to > non-debug if they are looked up from non-DEBUG_INSNs. The problem is that > when looking some completely unrelated MEM from a non-DEBUG_INSN we run into > a hash collision and so call cselib_hasher::equal to check if the unrelated > MEM is equal to the one from DEBUG_INSN only element. The equal static > member function calls rtx_equal_for_cselib_1 and if that returns true, > promotes the location to non-DEBUG, otherwise returns false. So far so > good. But rtx_equal_for_cselib_1 internally performs various other cselib > lookups, all done with the non-DEBUG_INSN cselib_current_insn, so they > all promote to non-debug. Good catch! > So, I think we need to pretend > that such lookup which only happens with -g and not -g0 actually comes > from some DEBUG_INSN (note, the lookups rtx_equal_for_cselib_1 does > are always with create = 0). > The cselib.cc part of the patch does that. Agreed, that makes sense to me, thanks! > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? FWIW, I'd approve it if I had the authority to do so :-) > I'd think we would need to differentiate between num_debug_mems and > num_mems depending on if setting_insn is non-NULL DEBUG_INSN or not. *nod*, I concur. Thanks! -- Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/ Free Software Activist GNU Toolchain Engineer Disinformation flourishes because many people care deeply about injustice but very few check the facts. Ask me about