Re: [PATCH v2] DSE: Use the constant store source if possible

[Richard Biener <richard.guenther@gmail.com>]

On Mon, May 23, 2022 at 8:34 PM H.J. Lu <hjl.tools@gmail.com> 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
> > <gcc-patches@gcc.gnu.org> 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 <stdint.h>
> +#include <vector>
> +#include <tr1/array>
> +
> +class FastBoard {
> +public:
> +    typedef std::pair<int, int> movescore_t;
> +    typedef std::tr1::array<movescore_t, 24> scoredlist_t;
> +
> +protected:
> +    std::vector<int> m_critical;
> +
> +    int m_boardsize;
> +};
> +
> +class FastState {
> +public:
> +    FastBoard board;
> +
> +    int movenum;
> +protected:
> +    FastBoard::scoredlist_t scoredmoves;
> +};
> +
> +class KoState : public FastState {
> +private:
> +    std::vector<uint64_t> ko_hash_history;
> +    std::vector<uint64_t> hash_history;
> +};
> +
> +class GameState : public KoState {
> +public:
> +    void foo ();
> +private:
> +    std::vector<KoState> game_history;
> +};
> +
> +void GameState::foo() {
> +    game_history.resize(movenum);
> +}
> --
> 2.36.1
>