Re: [PATCH] Fix ICE in store-merging (PR tree-optimization/82929)

[Richard Biener <rguenther at suse dot de>]

On Fri, 10 Nov 2017, Jakub Jelinek wrote:

> Hi!
> 
> Because BIT_{AND,IOR,XOR}_EXPR are commutative, we std::swap the
> store_operand_info ops if that means we could append the store into
> a group rather than having to start a new group.  Unfortunately
> for count_multiple_uses we need to walk the stmts computing the stored value
> in order to check the has_single_use stuff and if we've swapped the
> arguments, that confuses the function.
> 
> This patch just records that we've swapped them and then the function
> can take that into account easily.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Thanks,
Richard.

> 2017-11-10  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/82929
> 	* gimple-ssa-store-merging.c (struct store_immediate_info): Add
> 	ops_swapped_p non-static data member.
> 	(store_immediate_info::store_immediate_info): Clear it.
> 	(imm_store_chain_info::coalesce_immediate_stores): If swapping
> 	ops set ops_swapped_p.
> 	(count_multiple_uses): Handle ops_swapped_p.
> 
> 	* gcc.dg/pr82929.c: New test.
> 	* g++.dg/opt/pr82929.C: New test.
> 
> --- gcc/gimple-ssa-store-merging.c.jj	2017-11-09 20:24:34.000000000 +0100
> +++ gcc/gimple-ssa-store-merging.c	2017-11-10 08:04:49.192744048 +0100
> @@ -209,7 +209,11 @@ struct store_immediate_info
>    /* INTEGER_CST for constant stores, MEM_REF for memory copy or
>       BIT_*_EXPR for logical bitwise operation.  */
>    enum tree_code rhs_code;
> +  /* True if BIT_{AND,IOR,XOR}_EXPR result is inverted before storing.  */
>    bool bit_not_p;
> +  /* True if ops have been swapped and thus ops[1] represents
> +     rhs1 of BIT_{AND,IOR,XOR}_EXPR and ops[0] represents rhs2.  */
> +  bool ops_swapped_p;
>    /* Operands.  For BIT_*_EXPR rhs_code both operands are used, otherwise
>       just the first one.  */
>    store_operand_info ops[2];
> @@ -231,7 +235,8 @@ store_immediate_info::store_immediate_in
>  					    const store_operand_info &op0r,
>  					    const store_operand_info &op1r)
>    : bitsize (bs), bitpos (bp), bitregion_start (brs), bitregion_end (bre),
> -    stmt (st), order (ord), rhs_code (rhscode), bit_not_p (bitnotp)
> +    stmt (st), order (ord), rhs_code (rhscode), bit_not_p (bitnotp),
> +    ops_swapped_p (false)
>  #if __cplusplus >= 201103L
>      , ops { op0r, op1r }
>  {
> @@ -1186,7 +1191,10 @@ imm_store_chain_info::coalesce_immediate
>  		  == info->bitpos - infof->bitpos)
>  	      && operand_equal_p (info->ops[1].base_addr,
>  				  infof->ops[0].base_addr, 0))
> -	    std::swap (info->ops[0], info->ops[1]);
> +	    {
> +	      std::swap (info->ops[0], info->ops[1]);
> +	      info->ops_swapped_p = true;
> +	    }
>  	  if ((!infof->ops[0].base_addr
>  	       || compatible_load_p (merged_store, info, base_addr, 0))
>  	      && (!infof->ops[1].base_addr
> @@ -1410,18 +1418,21 @@ count_multiple_uses (store_immediate_inf
>        stmt = SSA_NAME_DEF_STMT (gimple_assign_rhs1 (stmt));
>        /* stmt is now the BIT_*_EXPR.  */
>        if (!has_single_use (gimple_assign_rhs1 (stmt)))
> -	ret += 1 + info->ops[0].bit_not_p;
> -      else if (info->ops[0].bit_not_p)
> +	ret += 1 + info->ops[info->ops_swapped_p].bit_not_p;
> +      else if (info->ops[info->ops_swapped_p].bit_not_p)
>  	{
>  	  gimple *stmt2 = SSA_NAME_DEF_STMT (gimple_assign_rhs1 (stmt));
>  	  if (!has_single_use (gimple_assign_rhs1 (stmt2)))
>  	    ++ret;
>  	}
>        if (info->ops[1].base_addr == NULL_TREE)
> -	return ret;
> +	{
> +	  gcc_checking_assert (!info->ops_swapped_p);
> +	  return ret;
> +	}
>        if (!has_single_use (gimple_assign_rhs2 (stmt)))
> -	ret += 1 + info->ops[1].bit_not_p;
> -      else if (info->ops[1].bit_not_p)
> +	ret += 1 + info->ops[1 - info->ops_swapped_p].bit_not_p;
> +      else if (info->ops[1 - info->ops_swapped_p].bit_not_p)
>  	{
>  	  gimple *stmt2 = SSA_NAME_DEF_STMT (gimple_assign_rhs2 (stmt));
>  	  if (!has_single_use (gimple_assign_rhs1 (stmt2)))
> --- gcc/testsuite/gcc.dg/pr82929.c.jj	2017-11-10 08:10:14.399799845 +0100
> +++ gcc/testsuite/gcc.dg/pr82929.c	2017-11-10 08:18:24.131904561 +0100
> @@ -0,0 +1,18 @@
> +/* PR tree-optimization/82929 */
> +/* { dg-do compile { target store_merge } } */
> +/* { dg-options "-O2 -fdump-tree-store-merging" } */
> +
> +void
> +foo (short *p, short *q, short *r)
> +{
> +  short a = q[0];
> +  short b = q[1];
> +  short c = ~a;
> +  short d = r[0];
> +  short e = r[1];
> +  short f = ~b;
> +  p[0] = c & d;
> +  p[1] = e & f;
> +}
> +
> +/* { dg-final { scan-tree-dump-times "Merging successful" 1 "store-merging" } } */
> --- gcc/testsuite/g++.dg/opt/pr82929.C.jj	2017-11-10 08:17:35.843479732 +0100
> +++ gcc/testsuite/g++.dg/opt/pr82929.C	2017-11-10 08:16:16.000000000 +0100
> @@ -0,0 +1,30 @@
> +// PR tree-optimization/82929
> +// { dg-do compile }
> +// { dg-options "-O2" }
> +
> +template <int _Nw> struct A {
> +  long _M_w[_Nw];
> +  void m_fn1(A p1) {
> +    for (int a = 0;; a++)
> +      _M_w[a] &= p1._M_w[a];
> +  }
> +  void m_fn2() {
> +    for (int b = 0; b < _Nw; b++)
> +      _M_w[b] = ~_M_w[b];
> +  }
> +};
> +template <int _Nb> struct C : A<_Nb / (8 * 8)> {
> +  void operator&=(C p1) { this->m_fn1(p1); }
> +  C m_fn3() {
> +    this->m_fn2();
> +    return *this;
> +  }
> +  C operator~() { return C(*this).m_fn3(); }
> +};
> +struct B {
> +  C<192> Value;
> +};
> +void fn1(C<192> &p1) {
> +  B c;
> +  p1 &= ~c.Value;
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)