From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 794193858424 for ; Fri, 29 Jul 2022 09:52:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 794193858424 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id AD61E1063; Fri, 29 Jul 2022 02:52:11 -0700 (PDT) Received: from [10.2.78.65] (e126323.cambridge.arm.com [10.2.78.65]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 7C3E23F73D; Fri, 29 Jul 2022 02:52:10 -0700 (PDT) Message-ID: <21a567be-4898-2e69-178f-a735be8c9742@foss.arm.com> Date: Fri, 29 Jul 2022 10:52:08 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: cselib: add function to check if SET is redundant [PR106187] Content-Language: en-GB To: Richard Biener , Richard Earnshaw Cc: "gcc-patches@gcc.gnu.org" References: From: Richard Earnshaw In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-3490.6 required=5.0 tests=BAYES_00, KAM_DMARC_STATUS, KAM_LAZY_DOMAIN_SECURITY, NICE_REPLY_A, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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, 29 Jul 2022 09:52:13 -0000 On 29/07/2022 08:06, Richard Biener via Gcc-patches wrote: > On Thu, Jul 28, 2022 at 6:46 PM Richard Earnshaw > wrote: >> >> [resend with correct subject line] >> >> A SET operation that writes memory may have the same value as an earlier >> store but if the alias sets of the new and earlier store do not conflict >> then the set is not truly redundant. This can happen, for example, if >> objects of different types share a stack slot. >> >> To fix this we define a new function in cselib that first checks for >> equality and if that is successful then finds the earlier store in the >> value history and checks the alias sets. >> >> The routine is used in two places elsewhere in the compiler. Firstly >> in cfgcleanup and secondly in postreload. > > I can't comment on the stripping on SUBREGs and friends but it seems > to be conservative apart from > > + if (!flag_strict_aliasing || !MEM_P (dest)) > + return true; > > where if dest is not a MEM but were to contain one we'd miss it. > Double-checking > from more RTL literate people appreciated. There are very few things that can wrap a MEM in a SET_DEST. I'm pretty sure that's all of them. It certainly matches the code in cselib_invalidate_rtx which has to deal with this sort of case. > > + /* Lookup the equivalents to the dest. This is more likely to succeed > + than looking up the equivalents to the source (for example, when the > + src is some form of constant). */ > > I think the comment is misleading - we _do_ have to lookup the MEM, > looking up equivalences of a reg or an expression on the RHS isn't > what we are interested in. OK, I'll try to reword it. > > + return alias_sets_conflict_p (MEM_ALIAS_SET (dest), > + MEM_ALIAS_SET (src_equiv)); > > that's not conservative enough - dse.cc has correct boilerplate, we have > to check both MEM_ALIAS_SET and MEM_EXPR here (the latter only > if the former load/store has a MEM_EXPR). Note in particular > using alias_set_subset_of instead of alias_sets_conflict_p. > > /* We can only remove the later store if the earlier aliases > at least all accesses the later one. */ > && ((MEM_ALIAS_SET (mem) == MEM_ALIAS_SET (s_info->mem) > || alias_set_subset_of (MEM_ALIAS_SET (mem), > MEM_ALIAS_SET (s_info->mem))) > && (!MEM_EXPR (s_info->mem) > || refs_same_for_tbaa_p (MEM_EXPR (s_info->mem), > MEM_EXPR (mem))))) > OK, that's an easy enough change. > + /* We failed to find a recorded value in the cselib history, so try the > + source of this set. */ > + rtx src = SET_SRC (set); > + while (GET_CODE (src) == SUBREG) > + src = XEXP (src, 0); > + > + if (MEM_P (src) && rtx_equal_for_cselib_1 (dest_addr, XEXP (src, 0), > + GET_MODE (dest), 0)) > + return alias_sets_conflict_p (MEM_ALIAS_SET (dest), > + MEM_ALIAS_SET (src)); > > this looks like an odd case to me - wouldn't that only catch things > like self-assignments, aka *p = *p? So I'd simply drop this fallback. It catches the case of *p = *q when p and q have the same value. It did come up in testing on x86 (when previously I was aborting to make sure I'd caught everything). We could leave it out as the fallback case in this instance is to record a conflict, but it's not a path that's likely to be performance critical and the probability of this being a redundant store is quite high. I'll update the comment to make this clearer. R. > > Otherwise it looks OK to me. > > Thanks, > Richard. > >> gcc/ChangeLog: >> * cselib.h (cselib_redundant_set_p): Declare. >> * cselib.cc: Include alias.h >> (cselib_redundant_set_p): New function. >> * cfgcleanup.cc: (mark_effect): Use cselib_redundant_set_p instead >> of rtx_equal_for_cselib_p. >> * postreload.c (reload_cse_simplify): Use cselib_redundant_set_p. >> (reload_cse_noop_set_p): Delete.