Re: [V2 PATCH] Handle bitop with INTEGER_CST in analyze_and_compute_bitop_with_inv_effect.

[Richard Biener <richard.guenther@gmail.com>]

On Tue, Nov 7, 2023 at 2:03 PM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Tue, Nov 7, 2023 at 4:10 PM Richard Biener
> <richard.guenther@gmail.com> wrote:
> >
> > On Tue, Nov 7, 2023 at 7:08 AM liuhongt <hongtao.liu@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?

> >
> >  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