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

[Richard Sandiford <richard.sandiford@arm.com>]

"H.J. Lu via Gcc-patches" <gcc-patches@gcc.gnu.org> writes:
> 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 introduce a helper for this?  There are already similar tests
in ira and loop-iv, and it seems a bit too complex to have to open-code
each time.

Thanks,
Richard

> +		{
> +		  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)
> +			  && GET_MODE (def_src) == GET_MODE (rhs))
> +			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);
> +}