From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id 6B089385800A for ; Thu, 2 Feb 2023 12:02:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6B089385800A Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 2830A21B7C; Thu, 2 Feb 2023 12:02:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1675339341; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=5WzN4aJqRIk/HfN/ylCi1zyR+mMqum2Fyc9ozbLE+3A=; b=N8qFCnOxo6dCLGS8Kbvu68g/zmTEMVcrwcoyRfRaxwBOKJPGnB3m8USEzLXJebYAjkcDll JAtdIWc6E+Xl3cBC1pZhUQrYa4WKRTr6yxlIUqfYT+1LCD7lFXmyTEzAPjXVQYMF5o3y3Q sQEA3Utq/WLiPl/1e72PUm63a5dOA6E= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1675339341; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=5WzN4aJqRIk/HfN/ylCi1zyR+mMqum2Fyc9ozbLE+3A=; b=VcpwzJplXPYfbIoyrAsdEQjOQ7XxqTYh7rZk+GxueuoR3nm+TTXVLhh0btnQS4O070io/e sxgqRGB9qNO4cEDQ== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (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 017392C141; Thu, 2 Feb 2023 12:02:19 +0000 (UTC) Date: Thu, 2 Feb 2023 12:02:19 +0000 (UTC) From: Richard Biener To: Alexandre Oliva cc: Jakub Jelinek , Jeff Law , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] sched-deps, cselib: Fix up some -fcompare-debug issues and regressions [PR108463] In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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, 2 Feb 2023, Alexandre Oliva wrote: > 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 :-) OK. Thanks, Richard.