On Tue, May 24, 2022 at 10:11 PM H.J. Lu <hjl.to...@gmail.com> wrote:
>
> On Mon, May 23, 2022 at 11:42 PM Richard Biener
> <richard.guent...@gmail.com> wrote:
> >
> > On Mon, May 23, 2022 at 8:34 PM H.J. Lu <hjl.to...@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).
>
> Does the hard register have a regular DEF? Won't the function args &
> hardregs have DF_REF_ARTIFICIAL?
I don't know - do they?
> >
> > > + {
> > > + 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 <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
> > >
>
>
>
> --
> H.J.
