On Tue, Nov 7, 2023 at 10:34 PM Richard Biener <richard.guent...@gmail.com> wrote: > > On Tue, Nov 7, 2023 at 2:03 PM Hongtao Liu <crazy...@gmail.com> wrote: > > > > On Tue, Nov 7, 2023 at 4:10 PM Richard Biener > > <richard.guent...@gmail.com> wrote: > > > > > > On Tue, Nov 7, 2023 at 7:08 AM liuhongt <hongtao....@intel.com> wrote: > > > > > > > > analyze_and_compute_bitop_with_inv_effect assumes the first operand is > > > > loop invariant which is not the case when it's INTEGER_CST. > > > > > > > > Bootstrapped and regtseted on x86_64-pc-linux-gnu{-m32,}. > > > > Ok for trunk? > > > > > > So this addresses a missed optimization, right? It seems to me that > > > even with two SSA names we are only "lucky" when rhs1 is the invariant > > > one. So instead of swapping this way I'd do > > Yes, it's a miss optimization. > > And I think expr_invariant_in_loop_p (loop, match_op[1]) should be > > enough, if match_op[1] is a loop invariant.it must be false for the > > below conditions(there couldn't be any header_phi from its > > definition). > > Yes, all I said is that when you now care for op1 being INTEGER_CST > it could also be an invariant SSA name and thus only after swapping op0/op1 > we could have a successful match, no? Sorry, the commit message is a little bit misleading. At first, I just wanted to handle the INTEGER_CST case (with TREE_CODE (match_op[1]) == INTEGER_CST), but then I realized that this could probably be extended to the normal SSA_NAME case as well, so I used expr_invariant_in_loop_p, which should theoretically be able to handle the SSA_NAME case as well.
if (expr_invariant_in_loop_p (loop, match_op[1])) is true, w/o swapping it must return NULL_TREE for below conditions. if (expr_invariant_in_loop_p (loop, match_op[1])) is false, w/ swapping it must return NULL_TREE too. So it can cover the both cases you mentioned, no need for a loop to iterate 2 match_ops for all conditions. 3692 if (TREE_CODE (match_op[1]) != SSA_NAME 3693 || !expr_invariant_in_loop_p (loop, match_op[0]) 3694 || !(header_phi = dyn_cast <gphi *> (SSA_NAME_DEF_STMT (match_op[1]))) 3695 || gimple_bb (header_phi) != loop->header 3696 || gimple_phi_num_args (header_phi) != 2) 3697 return NULL_TREE; 3698 3699 if (PHI_ARG_DEF_FROM_EDGE (header_phi, loop_latch_edge (loop)) != phidef) 3700 return NULL_TREE; > > > > > > > unsigned i; > > > for (i = 0; i < 2; ++i) > > > if (TREE_CODE (match_op[i]) == SSA_NAME > > > && ...) > > > break; /* found! */ > > > > > > if (i == 2) > > > return NULL_TREE; > > > if (i == 0) > > > std::swap (match_op[0], match_op[1]); > > > > > > to also handle a "swapped" pair of SSA names? > > > > > > > gcc/ChangeLog: > > > > > > > > PR tree-optimization/105735 > > > > PR tree-optimization/111972 > > > > * tree-scalar-evolution.cc > > > > (analyze_and_compute_bitop_with_inv_effect): Handle bitop with > > > > INTEGER_CST. > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > > > * gcc.target/i386/pr105735-3.c: New test. > > > > --- > > > > gcc/testsuite/gcc.target/i386/pr105735-3.c | 87 ++++++++++++++++++++++ > > > > gcc/tree-scalar-evolution.cc | 3 + > > > > 2 files changed, 90 insertions(+) > > > > create mode 100644 gcc/testsuite/gcc.target/i386/pr105735-3.c > > > > > > > > diff --git a/gcc/testsuite/gcc.target/i386/pr105735-3.c > > > > b/gcc/testsuite/gcc.target/i386/pr105735-3.c > > > > new file mode 100644 > > > > index 00000000000..9e268a1a997 > > > > --- /dev/null > > > > +++ b/gcc/testsuite/gcc.target/i386/pr105735-3.c > > > > @@ -0,0 +1,87 @@ > > > > +/* { dg-do compile } */ > > > > +/* { dg-options "-O1 -fdump-tree-sccp-details" } */ > > > > +/* { dg-final { scan-tree-dump-times {final value replacement} 8 > > > > "sccp" } } */ > > > > + > > > > +unsigned int > > > > +__attribute__((noipa)) > > > > +foo (unsigned int tmp) > > > > +{ > > > > + for (int bit = 0; bit < 64; bit++) > > > > + tmp &= 11304; > > > > + return tmp; > > > > +} > > > > + > > > > +unsigned int > > > > +__attribute__((noipa)) > > > > +foo1 (unsigned int tmp) > > > > +{ > > > > + for (int bit = 63; bit >= 0; bit -=3) > > > > + tmp &= 11304; > > > > + return tmp; > > > > +} > > > > + > > > > +unsigned int > > > > +__attribute__((noipa)) > > > > +foo2 (unsigned int tmp) > > > > +{ > > > > + for (int bit = 0; bit < 64; bit++) > > > > + tmp |= 11304; > > > > + return tmp; > > > > +} > > > > + > > > > +unsigned int > > > > +__attribute__((noipa)) > > > > +foo3 (unsigned int tmp) > > > > +{ > > > > + for (int bit = 63; bit >= 0; bit -=3) > > > > + tmp |= 11304; > > > > + return tmp; > > > > +} > > > > + > > > > +unsigned int > > > > +__attribute__((noipa)) > > > > +foo4 (unsigned int tmp) > > > > +{ > > > > + for (int bit = 0; bit < 64; bit++) > > > > + tmp ^= 11304; > > > > + return tmp; > > > > +} > > > > + > > > > +unsigned int > > > > +__attribute__((noipa)) > > > > +foo5 (unsigned int tmp) > > > > +{ > > > > + for (int bit = 0; bit < 63; bit++) > > > > + tmp ^= 11304; > > > > + return tmp; > > > > +} > > > > + > > > > +unsigned int > > > > +__attribute__((noipa)) > > > > +f (unsigned int tmp, int bit) > > > > +{ > > > > + unsigned int res = tmp; > > > > + for (int i = 0; i < bit; i++) > > > > + res &= 11304; > > > > + return res; > > > > +} > > > > + > > > > +unsigned int > > > > +__attribute__((noipa)) > > > > +f1 (unsigned int tmp, int bit) > > > > +{ > > > > + unsigned int res = tmp; > > > > + for (int i = 0; i < bit; i++) > > > > + res |= 11304; > > > > + return res; > > > > +} > > > > + > > > > +unsigned int > > > > +__attribute__((noipa)) > > > > +f2 (unsigned int tmp, int bit) > > > > +{ > > > > + unsigned int res = tmp; > > > > + for (int i = 0; i < bit; i++) > > > > + res ^= 11304; > > > > + return res; > > > > +} > > > > diff --git a/gcc/tree-scalar-evolution.cc b/gcc/tree-scalar-evolution.cc > > > > index 70b17c5bca1..f61277c32df 100644 > > > > --- a/gcc/tree-scalar-evolution.cc > > > > +++ b/gcc/tree-scalar-evolution.cc > > > > @@ -3689,6 +3689,9 @@ analyze_and_compute_bitop_with_inv_effect (class > > > > loop* loop, tree phidef, > > > > match_op[0] = gimple_assign_rhs1 (def); > > > > match_op[1] = gimple_assign_rhs2 (def); > > > > > > > > + if (expr_invariant_in_loop_p (loop, match_op[1])) > > > > + std::swap (match_op[0], match_op[1]); > > > > + > > > > if (TREE_CODE (match_op[1]) != SSA_NAME > > > > || !expr_invariant_in_loop_p (loop, match_op[0]) > > > > || !(header_phi = dyn_cast <gphi *> (SSA_NAME_DEF_STMT > > > > (match_op[1]))) > > > > -- > > > > 2.31.1 > > > > > > > > > > > > -- > > BR, > > Hongtao -- BR, Hongtao