From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com [IPv6:2a00:1450:4864:20::52b]) by sourceware.org (Postfix) with ESMTPS id 8E17D385840A for ; Fri, 29 Jul 2022 07:06:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8E17D385840A Received: by mail-ed1-x52b.google.com with SMTP id c12so4725902ede.3 for ; Fri, 29 Jul 2022 00:06:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mZEnsZ8QvsRKMxGxIqtMeP3gkQVY2Hc+o28omTdqPKw=; b=q33xIZ3hp2+WXgW7TATZUulWU+ppIcbVVizzJ4d1FP1hitS5HpZ+q9HIn/mRupGS+0 n0pGATOnk9uCuKLcus5p0ypHD5u1FQ63reAieNtSLhl6YWnKWmp1+t38jTFlWDbEtyXQ rceC9GS/91dyiPTs1qwGyCoDd+PdRH8kODkpYv7+mp3F4tXTIXR6tfhZ+9aOs9BZmaHL mOdR37ITfW+uAov+DAnZh+6F9zmWCYzWflcLjazRQOlXpQFI72T7fC0QUU/kxrbAcXkd n+TWo66HDVzsnes+yqvGwT4FBKDqpINwEr717jwKv2L55pBNeYj9O+yW8vgtDvRiuRu2 BlQw== X-Gm-Message-State: AJIora9Tbg5SUw7eSdbKC5I6YZBnsaIXU6/oqiTPGaOmPVRFlDHE44jS hNvDM1+SWbY612UROZs/3j+2VAb0pF4Or/HDlY8= X-Google-Smtp-Source: AGRyM1tGOKJ2iw7C34xFTTTFK6jENt7SVNALxUNZwsSL1pM4RASDFiHddoUPWAOVcMUpqMUbQ87TzuFg0aT9tIUvDiY= X-Received: by 2002:aa7:dd4b:0:b0:43a:d508:7cb9 with SMTP id o11-20020aa7dd4b000000b0043ad5087cb9mr2252073edw.218.1659078388827; Fri, 29 Jul 2022 00:06:28 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Richard Biener Date: Fri, 29 Jul 2022 09:06:15 +0200 Message-ID: Subject: Re: cselib: add function to check if SET is redundant [PR106187] To: Richard Earnshaw Cc: "gcc-patches@gcc.gnu.org" Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-2.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, 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 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 07:06:32 -0000 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. + /* 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. + 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))))) + /* 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. 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.