Re: [PATCH] Add extra copy of the ifcombine pass after pre [PR102793]

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

On Fri, Apr 5, 2024 at 8:14 PM Andrew Pinski <pinskia@gmail.com> wrote:
>
> On Fri, Apr 5, 2024 at 5:28 AM Manolis Tsamis <manolis.tsamis@vrull.eu> wrote:
> >
> > If we consider code like:
> >
> >     if (bar1 == x)
> >       return foo();
> >     if (bar2 != y)
> >       return foo();
> >     return 0;
> >
> > We would like the ifcombine pass to convert this to:
> >
> >     if (bar1 == x || bar2 != y)
> >       return foo();
> >     return 0;
> >
> > The ifcombine pass can handle this transformation but it is ran very early and
> > it misses the opportunity because there are two seperate blocks for foo().
> > The pre pass is good at removing duplicate code and blocks and due to that
> > running ifcombine again after it can increase the number of successful
> > conversions.
>
> I do think we should have something similar to re-running
> ssa-ifcombine but I think it should be much later, like after the loop
> optimizations are done.
> Maybe just a simplified version of it (that does the combining and not
> the optimizations part) included in isel or pass_optimize_widening_mul
> (which itself should most likely become part of isel or renamed since
> it handles more than just widening multiply these days).

I've long wished we had a (late?) pass that can also undo if-conversion
(basically do what RTL expansion would later do).  Maybe
gimple-predicate-analysis.cc (what's used by uninit analysis) can
represent mixed CFG + if-converted conditions so we can optimize
it and code-gen the condition in a more optimal manner much like
we have if-to-switch, switch-conversion and switch-expansion.

That said, I agree that re-running ifcombine should be later.  And there's
still the old task of splitting tail-merging from PRE (and possibly making
it more effective).

Richard.

>
> Thanks,
> Andrew Pinski
>
>
> >
> >         PR 102793
> >
> > gcc/ChangeLog:
> >
> >         * common.opt: -ftree-ifcombine option, enabled by default.
> >         * doc/invoke.texi: Document.
> >         * passes.def: Re-run ssa-ifcombine after pre.
> >         * tree-ssa-ifcombine.cc: Make ifcombine cloneable. Add gate function.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.dg/tree-ssa/20030922-2.c: Change flag to -fno-tree-ifcombine.
> >         * gcc.dg/uninit-pred-6_c.c: Remove inconsistent check.
> >         * gcc.target/aarch64/pr102793.c: New test.
> >
> > Signed-off-by: Manolis Tsamis <manolis.tsamis@vrull.eu>
> > ---
> >
> >  gcc/common.opt                              |  4 +++
> >  gcc/doc/invoke.texi                         |  5 ++++
> >  gcc/passes.def                              |  1 +
> >  gcc/testsuite/gcc.dg/tree-ssa/20030922-2.c  |  2 +-
> >  gcc/testsuite/gcc.dg/uninit-pred-6_c.c      |  4 ---
> >  gcc/testsuite/gcc.target/aarch64/pr102793.c | 30 +++++++++++++++++++++
> >  gcc/tree-ssa-ifcombine.cc                   |  5 ++++
> >  7 files changed, 46 insertions(+), 5 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.target/aarch64/pr102793.c
> >
> > diff --git a/gcc/common.opt b/gcc/common.opt
> > index ad348844775..e943202bcf1 100644
> > --- a/gcc/common.opt
> > +++ b/gcc/common.opt
> > @@ -3163,6 +3163,10 @@ ftree-phiprop
> >  Common Var(flag_tree_phiprop) Init(1) Optimization
> >  Enable hoisting loads from conditional pointers.
> >
> > +ftree-ifcombine
> > +Common Var(flag_tree_ifcombine) Init(1) Optimization
> > +Merge some conditional branches to simplify control flow.
> > +
> >  ftree-pre
> >  Common Var(flag_tree_pre) Optimization
> >  Enable SSA-PRE optimization on trees.
> > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> > index e2edf7a6c13..8d2ff6b4512 100644
> > --- a/gcc/doc/invoke.texi
> > +++ b/gcc/doc/invoke.texi
> > @@ -13454,6 +13454,11 @@ This flag is enabled by default at @option{-O1} and higher.
> >  Perform hoisting of loads from conditional pointers on trees.  This
> >  pass is enabled by default at @option{-O1} and higher.
> >
> > +@opindex ftree-ifcombine
> > +@item -ftree-ifcombine
> > +Merge some conditional branches to simplify control flow.  This pass
> > +is enabled by default at @option{-O1} and higher.
> > +
> >  @opindex fhoist-adjacent-loads
> >  @item -fhoist-adjacent-loads
> >  Speculatively hoist loads from both branches of an if-then-else if the
> > diff --git a/gcc/passes.def b/gcc/passes.def
> > index 1cbbd413097..1765b476131 100644
> > --- a/gcc/passes.def
> > +++ b/gcc/passes.def
> > @@ -270,6 +270,7 @@ along with GCC; see the file COPYING3.  If not see
> >        NEXT_PASS (pass_lim);
> >        NEXT_PASS (pass_walloca, false);
> >        NEXT_PASS (pass_pre);
> > +      NEXT_PASS (pass_tree_ifcombine);
> >        NEXT_PASS (pass_sink_code, false /* unsplit edges */);
> >        NEXT_PASS (pass_sancov);
> >        NEXT_PASS (pass_asan);
> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/20030922-2.c b/gcc/testsuite/gcc.dg/tree-ssa/20030922-2.c
> > index 16c79da9521..66c9f481a2f 100644
> > --- a/gcc/testsuite/gcc.dg/tree-ssa/20030922-2.c
> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/20030922-2.c
> > @@ -1,5 +1,5 @@
> >  /* { dg-do compile } */
> > -/* { dg-options "-O1 -fdump-tree-dom2 -fdisable-tree-ifcombine" } */
> > +/* { dg-options "-O1 -fdump-tree-dom2 -fno-tree-ifcombine" } */
> >
> >  struct rtx_def;
> >  typedef struct rtx_def *rtx;
> > diff --git a/gcc/testsuite/gcc.dg/uninit-pred-6_c.c b/gcc/testsuite/gcc.dg/uninit-pred-6_c.c
> > index f60868dad23..2d8e6501a45 100644
> > --- a/gcc/testsuite/gcc.dg/uninit-pred-6_c.c
> > +++ b/gcc/testsuite/gcc.dg/uninit-pred-6_c.c
> > @@ -20,10 +20,6 @@ int foo (int n, int l, int m, int r)
> >    if ( (n > 10) && l)
> >        blah(v); /* { dg-bogus "uninitialized" "bogus warning" } */
> >
> > -  if (l)
> > -    if (n > 12)
> > -      blah(v); /* { dg-bogus "uninitialized" "bogus warning" } */
> > -
> >    return 0;
> >  }
> >
> > diff --git a/gcc/testsuite/gcc.target/aarch64/pr102793.c b/gcc/testsuite/gcc.target/aarch64/pr102793.c
> > new file mode 100644
> > index 00000000000..78d48e01637
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/aarch64/pr102793.c
> > @@ -0,0 +1,30 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2" } */
> > +
> > +typedef unsigned long uint64_t;
> > +
> > +int ccmp1(uint64_t* s1, uint64_t* s2, int(*foo)(void))
> > +{
> > +  uint64_t d1, d2, bar;
> > +  d1 = *s1++;
> > +  d2 = *s2++;
> > +  bar = (d1 + d2) & 0xabcd;
> > +  if (bar == 0 || d1 != d2)
> > +    return foo();
> > +  return 0;
> > +}
> > +
> > +int ccmp2(uint64_t* s1, uint64_t* s2, int(*foo)(void))
> > +{
> > +  uint64_t d1, d2, bar;
> > +  d1 = *s1++;
> > +  d2 = *s2++;
> > +  bar = (d1 + d2) & 0xabcd;
> > +  if (bar == 0)
> > +    return foo();
> > +  if (d1 != d2)
> > +    return foo();
> > +  return 0;
> > +}
> > +
> > +/* { dg-final { scan-assembler-times "ccmp\t" 2 } } */
> > \ No newline at end of file
> > diff --git a/gcc/tree-ssa-ifcombine.cc b/gcc/tree-ssa-ifcombine.cc
> > index 6a3bc99190d..0bf9fe8b692 100644
> > --- a/gcc/tree-ssa-ifcombine.cc
> > +++ b/gcc/tree-ssa-ifcombine.cc
> > @@ -838,6 +838,11 @@ public:
> >    {}
> >
> >    /* opt_pass methods: */
> > +  opt_pass * clone () final override
> > +  {
> > +    return new pass_tree_ifcombine (m_ctxt);
> > +  }
> > +  bool gate (function *) final override { return flag_tree_ifcombine; }
> >    unsigned int execute (function *) final override;
> >
> >  }; // class pass_tree_ifcombine
> > --
> > 2.44.0
> >