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 <ja...@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 <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
