From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qv1-xf2d.google.com (mail-qv1-xf2d.google.com [IPv6:2607:f8b0:4864:20::f2d]) by sourceware.org (Postfix) with ESMTPS id 35EA2385741A for ; Tue, 24 May 2022 06:42:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 35EA2385741A Received: by mail-qv1-xf2d.google.com with SMTP id y11so181501qvr.9 for ; Mon, 23 May 2022 23:42:12 -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=1rIFmroFYqBiScNjUi1acqiRn1piEzmU4ksELYXzHiM=; b=Qgr8lhp2Wq8Jt6JTBYEhkTjIZPwjCtCf63bcFLi5HrMYtZ84K+6K8lYNzfk8YbJidb J5bVlB1m8WA8ZFlPSjXT/pvlHLOwLxY/M91bh+hi0xfvbwV3MDnJ30tsp/hojnbOEJBW BaxixrBst3gu5syzctMFwKKQ1fX3IWeGiH9jPBZwYkIhmBirfzfb1QHZlWUkp+OimE2/ FB0fEL0XYRjboMZFA+lcaO6PmnlMaQJmY+IHQoWR5iNJXkpLAyL0yYxH6slpSvfXJndF Fmv7kQgFAd51DvF2dADuJUm/8DqkwXPO7Em9xiB6IRvBiJwq9+Bm7Ugs8J68uAAt67Lb +tag== X-Gm-Message-State: AOAM531OrYTxiDX0NcDPbeg1SoSEqLLNrLro/d0a0AtkUUJBNnfPQOej dCregXXKhIjBqLamqIw4c5WMxVRkLujWU7sP/5w= X-Google-Smtp-Source: ABdhPJxWhPmXm26bfuU+oLDlpoGbt7bnmDmHLREMgKnhU33DmKLHfQj/M0xXt010yyATt0ZwHOTE4c3v3WgaYS9BVGY= X-Received: by 2002:ad4:5ba6:0:b0:462:305c:83a5 with SMTP id 6-20020ad45ba6000000b00462305c83a5mr8597254qvq.34.1653374531501; Mon, 23 May 2022 23:42:11 -0700 (PDT) MIME-Version: 1.0 References: <20220521030120.1977551-1-hjl.tools@gmail.com> In-Reply-To: From: Richard Biener Date: Tue, 24 May 2022 08:42:00 +0200 Message-ID: Subject: Re: [PATCH v2] DSE: Use the constant store source if possible To: "H.J. Lu" Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-7.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE 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: Tue, 24 May 2022 06:42:14 -0000 On Mon, May 23, 2022 at 8:34 PM H.J. Lu wrote: > > On Mon, May 23, 2022 at 12:38:06PM +0200, Richard Biener wrote: > > On Sat, May 21, 2022 at 5:02 AM H.J. Lu via Gcc-patches > > wrote: > > > > > > When recording store for RTL dead store elimination, check if the source > > > register is set only once to a constant. If yes, record the constant > > > as the store source. It eliminates unrolled zero stores after memset 0 > > > in a loop where a vector register is used as the zero store source. > > > > > > gcc/ > > > > > > PR rtl-optimization/105638 > > > * dse.cc (record_store): Use the constant source if the source > > > register is set only once. > > > > > > gcc/testsuite/ > > > > > > PR rtl-optimization/105638 > > > * g++.target/i386/pr105638.C: New test. > > > --- > > > gcc/dse.cc | 19 ++++++++++ > > > gcc/testsuite/g++.target/i386/pr105638.C | 44 ++++++++++++++++++++++++ > > > 2 files changed, 63 insertions(+) > > > create mode 100644 gcc/testsuite/g++.target/i386/pr105638.C > > > > > > diff --git a/gcc/dse.cc b/gcc/dse.cc > > > index 30c11cee034..0433dd3d846 100644 > > > --- a/gcc/dse.cc > > > +++ b/gcc/dse.cc > > > @@ -1508,6 +1508,25 @@ record_store (rtx body, bb_info_t bb_info) > > > > > > if (tem && CONSTANT_P (tem)) > > > const_rhs = tem; > > > + else > > > + { > > > + /* If RHS is set only once to a constant, set CONST_RHS > > > + to the constant. */ > > > + df_ref def = DF_REG_DEF_CHAIN (REGNO (rhs)); > > > + if (def != nullptr > > > + && !DF_REF_IS_ARTIFICIAL (def) > > > + && !DF_REF_NEXT_REG (def)) > > > + { > > > + rtx_insn *def_insn = DF_REF_INSN (def); > > > + rtx def_body = PATTERN (def_insn); > > > + if (GET_CODE (def_body) == SET) > > > + { > > > + rtx def_src = SET_SRC (def_body); > > > + if (CONSTANT_P (def_src)) > > > + const_rhs = def_src; > > > > doesn't DSE have its own tracking of stored values? Shouldn't we > > It tracks stored values only within the basic block. When RTL loop > invariant motion hoists a constant initialization out of the loop into > a separate basic block, the constant store value becomes unknown > within the original basic block. > > > improve _that_ if it is not enough? I also wonder if you need to > > My patch extends DSE stored value tracking to include the constant which > is set only once in another basic block. > > > verify the SET isn't partial? > > > > Here is the v2 patch to check that the constant is set by a non-partial > unconditional load. > > OK for master? > > Thanks. > > H.J. > --- > RTL DSE tracks redundant constant stores within a basic block. When RTL > loop invariant motion hoists a constant initialization out of the loop > into a separate basic block, the constant store value becomes unknown > within the original basic block. When recording store for RTL DSE, check > if the source register is set only once to a constant by a non-partial > unconditional load. If yes, record the constant as the constant store > source. It eliminates unrolled zero stores after memset 0 in a loop > where a vector register is used as the zero store source. > > gcc/ > > PR rtl-optimization/105638 > * dse.cc (record_store): Use the constant source if the source > register is set only once. > > gcc/testsuite/ > > PR rtl-optimization/105638 > * g++.target/i386/pr105638.C: New test. > --- > gcc/dse.cc | 22 ++++++++++++ > gcc/testsuite/g++.target/i386/pr105638.C | 44 ++++++++++++++++++++++++ > 2 files changed, 66 insertions(+) > create mode 100644 gcc/testsuite/g++.target/i386/pr105638.C > > diff --git a/gcc/dse.cc b/gcc/dse.cc > index 30c11cee034..af8e88dac32 100644 > --- a/gcc/dse.cc > +++ b/gcc/dse.cc > @@ -1508,6 +1508,28 @@ record_store (rtx body, bb_info_t bb_info) > > if (tem && CONSTANT_P (tem)) > const_rhs = tem; > + else > + { > + /* If RHS is set only once to a constant, set CONST_RHS > + to the constant. */ > + df_ref def = DF_REG_DEF_CHAIN (REGNO (rhs)); > + if (def != nullptr > + && !DF_REF_IS_ARTIFICIAL (def) > + && !(DF_REF_FLAGS (def) > + & (DF_REF_PARTIAL | DF_REF_CONDITIONAL)) > + && !DF_REF_NEXT_REG (def)) Can we really use df-chain here and rely that a single definition is the only one? If rhs is a hardreg does df-chain include implicit sets of function argument registers for example? Don't we need RD here or at least verify the single df-chain definition dominates the use here (if we can rely on the reg otherwise be uninitialized and thus the use invoking undefined behavior we could use the constant even in non-dominating context, but WRT the function args & hardregs I'm not sure we can tell). > + { > + rtx_insn *def_insn = DF_REF_INSN (def); > + rtx def_body = PATTERN (def_insn); > + if (GET_CODE (def_body) == SET) I think you should be able to use rtx def_body = single_set (def_insn); if (def_body) here to get at some more cases. > + { > + rtx def_src = SET_SRC (def_body); > + if (CONSTANT_P (def_src) > + && GET_MODE (def_src) == GET_MODE (rhs)) possibly verify SET_DEST (def_body) == rhs to rule out subregs and mode punning instead? Then def_src should be automatically OK. I wonder whether it's worth tracking (or if DSE even can) loads from the constant pool with constant REG_EQUAL/EQUIV notes? > + const_rhs = def_src; > + } > + } > + } > } > } > > diff --git a/gcc/testsuite/g++.target/i386/pr105638.C b/gcc/testsuite/g++.target/i386/pr105638.C > new file mode 100644 > index 00000000000..ff40a459de1 > --- /dev/null > +++ b/gcc/testsuite/g++.target/i386/pr105638.C > @@ -0,0 +1,44 @@ > +/* { dg-do compile { target { ! ia32 } } } */ > +/* { dg-options "-std=gnu++20 -O2 -march=skylake" } */ > +/* { dg-final { scan-assembler-not "vpxor" } } */ > + > +#include > +#include > +#include > + > +class FastBoard { > +public: > + typedef std::pair movescore_t; > + typedef std::tr1::array scoredlist_t; > + > +protected: > + std::vector m_critical; > + > + int m_boardsize; > +}; > + > +class FastState { > +public: > + FastBoard board; > + > + int movenum; > +protected: > + FastBoard::scoredlist_t scoredmoves; > +}; > + > +class KoState : public FastState { > +private: > + std::vector ko_hash_history; > + std::vector hash_history; > +}; > + > +class GameState : public KoState { > +public: > + void foo (); > +private: > + std::vector game_history; > +}; > + > +void GameState::foo() { > + game_history.resize(movenum); > +} > -- > 2.36.1 >