From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x535.google.com (mail-pg1-x535.google.com [IPv6:2607:f8b0:4864:20::535]) by sourceware.org (Postfix) with ESMTPS id 4B4233857B9B for ; Tue, 24 May 2022 20:11:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 4B4233857B9B Received: by mail-pg1-x535.google.com with SMTP id 137so17206812pgb.5 for ; Tue, 24 May 2022 13:11:02 -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=HJHwVCsKBLiwjVlB+FwOd+vXy+q45Bq/ufGmXPSi2Fw=; b=GBORJGWyS5+suJ06BmLAwZ53tluTrCuhE1BrRTRzyTfaDTWIEHwucnLE0URRy6pK/A Ws42TkfYVVwcyFBNHAF3Nertz+5Eo8hJY8f9j7pRhow5EDEkhzhbQHBlZpeQRujvRHuT /U4Ma8SKkW75tESs8Oa7G8Lbls8r4sZCV6l7xMnM53jtwkx5qX0Km8fK3h5VLcWuCDqV hLYWaWi8NhHkQJfKefptCRMv2kxP3LY4j7Pg/kNkoOtIiG62EAzbT/Oo+ccCpErfEAmA kwGvsmuD+fnTTft83an9E0eLNCuxVJFgoyCtvX1CgyPKLCivjmhqh5vsKP7XRCVZ4bTr e25w== X-Gm-Message-State: AOAM532jCQea5vjPlJ+6lW02qI2LD/Ey2trlqNlsqQ7k3Pg/ZGfpdzMR 5QNUxSHE8RxVNpoXW0YGs8N7OfHBabWTPsfFvn0= X-Google-Smtp-Source: ABdhPJwBxAbic66pf8zoh6Yr7JuSsLMPEFh+tI1ih1gPce5aJTcuDK6nqBBOPdis4CfzBTkrDl18Gb+Ktezh4edcrW8= X-Received: by 2002:a65:63d9:0:b0:374:6b38:c6b3 with SMTP id n25-20020a6563d9000000b003746b38c6b3mr25808484pgv.195.1653423060884; Tue, 24 May 2022 13:11:00 -0700 (PDT) MIME-Version: 1.0 References: <20220521030120.1977551-1-hjl.tools@gmail.com> In-Reply-To: From: "H.J. Lu" Date: Tue, 24 May 2022 13:10:24 -0700 Message-ID: Subject: Re: [PATCH v2] DSE: Use the constant store source if possible To: Richard Biener Cc: GCC Patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3025.4 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 20:11:04 -0000 On Mon, May 23, 2022 at 11:42 PM Richard Biener wrote: > > 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). Does the hard register have a regular DEF? Won't the function args & hardregs have DF_REF_ARTIFICIAL? > > > + { > > + 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. Will change it. > > + { > > + 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 Will change it. > tracking (or if DSE even can) loads from the constant pool with constant.> REG_EQUAL/EQUIV notes? CONSTANT_P returns true for a constant pool. I am not sure if DSE can handle a constant pool. Thanks. > > > + 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 > > -- H.J.