On Thu, Oct 05, 2023 at 01:52:30PM +0200, Jan Hubicka wrote:
> > From: Sergei Trofimovich <siarh...@google.com>
> > 
> > r14-3459-g0c78240fd7d519 "Check that passes do not forget to define profile"
> > exposed check failures in cases when gcc produces uninitialized profile
> > probabilities. In case of PR/111559 uninitialized profile is generated
> > by edges executed 0 times reported by IPA profile:
> > 
> >     $ gcc -O2 -fprofile-generate pr111559.c -o b -fopt-info
> >     $ ./b
> >     $ gcc -O2 -fprofile-use -fprofile-correction pr111559.c -o b -fopt-info
> > 
> >     pr111559.c: In function 'rule1':
> >     pr111559.c:6:13: error: probability of edge 3->4 not initialized
> >         6 | static void rule1(void) { if (p) edge(); }
> >           |             ^~~~~
> >     during GIMPLE pass: fixup_cfg
> >     pr111559.c:6:13: internal compiler error: verify_flow_info failed
> > 
> > The change conservatively ignores updates with uninitialized values and
> > uses initially assigned probabilities (`always` probability in case of
> > the example).
> > 
> >     PR ipa/111283
> >     PR gcov-profile/111559
> > 
> > gcc/
> >     * ipa-utils.cc (ipa_merge_profiles): Avoid producing
> >     uninitialized probabilities when merging counters with zero
> >     denominators.
> > 
> > gcc/testsuite/
> >     * gcc.dg/tree-prof/pr111559.c: New test.
> > ---
> >  gcc/ipa-utils.cc                          |  6 +++++-
> >  gcc/testsuite/gcc.dg/tree-prof/pr111559.c | 16 ++++++++++++++++
> >  2 files changed, 21 insertions(+), 1 deletion(-)
> >  create mode 100644 gcc/testsuite/gcc.dg/tree-prof/pr111559.c
> > 
> > diff --git a/gcc/ipa-utils.cc b/gcc/ipa-utils.cc
> > index 956c6294fd7..7c53ae9dd45 100644
> > --- a/gcc/ipa-utils.cc
> > +++ b/gcc/ipa-utils.cc
> > @@ -651,13 +651,17 @@ ipa_merge_profiles (struct cgraph_node *dst,
> >             {
> >               edge srce = EDGE_SUCC (srcbb, i);
> >               edge dste = EDGE_SUCC (dstbb, i);
> > -             dste->probability = 
> > +             profile_probability merged =
> >                 dste->probability * dstbb->count.ipa ().probability_in
> >                                              (dstbb->count.ipa ()
> >                                               + srccount.ipa ())
> >                 + srce->probability * srcbb->count.ipa ().probability_in
> >                                              (dstbb->count.ipa ()
> >                                               + srccount.ipa ());
> > +             /* We produce uninitialized probabilities when
> > +                denominator is zero: https://gcc.gnu.org/PR111559.  */
> > +             if (merged.initialized_p ())
> > +               dste->probability = merged;
> 
> Thanks for the patch.
> We usually avoid the uninitialized value here by simply checking that
> parameter of probability_in satifies nonzero_p.  So I think it would be
> more consistent doing it here to:
> 
>   profile_probability sum = dstbb->count.ipa () + srccount.ipa ()
>   if (sum.nonzero_p ())
>   {
>      dste->probability = .....
>   }

Aha, sounds good! I had to do `s/profile_probability/profile_count` as
it's a denominator value for probability.

Attached v3 just in case.

> OK with this change.
> Honza
> >             }
> >           dstbb->count = dstbb->count.ipa () + srccount.ipa ();
> >         }
> > diff --git a/gcc/testsuite/gcc.dg/tree-prof/pr111559.c 
> > b/gcc/testsuite/gcc.dg/tree-prof/pr111559.c
> > new file mode 100644
> > index 00000000000..43202c6c888
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tree-prof/pr111559.c
> > @@ -0,0 +1,16 @@
> > +/* { dg-options "-O2" } */
> > +
> > +__attribute__((noipa)) static void edge(void) {}
> > +
> > +int p = 0;
> > +
> > +__attribute__((noinline))
> > +static void rule1(void) { if (p) edge(); }
> > +
> > +__attribute__((noinline))
> > +static void rule1_same(void) { if (p) edge(); }
> > +
> > +__attribute__((noipa)) int main(void) {
> > +    rule1();
> > +    rule1_same();
> > +}
> > -- 
> > 2.42.0
> > 

-- 

  Sergei
>From 97122ebae5a7ed43b6c31574c761a54bee3a96ec Mon Sep 17 00:00:00 2001
From: Sergei Trofimovich <siarh...@google.com>
Date: Wed, 27 Sep 2023 14:29:12 +0100
Subject: [PATCH v3] ipa-utils: avoid uninitialized probabilities on ICF
 [PR111559]

r14-3459-g0c78240fd7d519 "Check that passes do not forget to define profile"
exposed check failures in cases when gcc produces uninitialized profile
probabilities. In case of PR/111559 uninitialized profile is generated
by edges executed 0 times reported by IPA profile:

    $ gcc -O2 -fprofile-generate pr111559.c -o b -fopt-info
    $ ./b
    $ gcc -O2 -fprofile-use -fprofile-correction pr111559.c -o b -fopt-info

    pr111559.c: In function 'rule1':
    pr111559.c:6:13: error: probability of edge 3->4 not initialized
        6 | static void rule1(void) { if (p) edge(); }
          |             ^~~~~
    during GIMPLE pass: fixup_cfg
    pr111559.c:6:13: internal compiler error: verify_flow_info failed

The change conservatively ignores updates with zero execution counts and
uses initially assigned probabilities (`always` probability in case of
the example).

        PR ipa/111283
        PR gcov-profile/111559

gcc/
        * ipa-utils.cc (ipa_merge_profiles): Avoid producing
        uninitialized probabilities when merging counters with zero
        denominators.

gcc/testsuite/
        * gcc.dg/tree-prof/pr111559.c: New test.
---
 gcc/ipa-utils.cc                          | 17 ++++++++++-------
 gcc/testsuite/gcc.dg/tree-prof/pr111559.c | 16 ++++++++++++++++
 2 files changed, 26 insertions(+), 7 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/tree-prof/pr111559.c

diff --git a/gcc/ipa-utils.cc b/gcc/ipa-utils.cc
index 956c6294fd7..1355ccac6f0 100644
--- a/gcc/ipa-utils.cc
+++ b/gcc/ipa-utils.cc
@@ -651,13 +651,16 @@ ipa_merge_profiles (struct cgraph_node *dst,
                {
                  edge srce = EDGE_SUCC (srcbb, i);
                  edge dste = EDGE_SUCC (dstbb, i);
-                 dste->probability = 
-                   dste->probability * dstbb->count.ipa ().probability_in
-                                                (dstbb->count.ipa ()
-                                                 + srccount.ipa ())
-                   + srce->probability * srcbb->count.ipa ().probability_in
-                                                (dstbb->count.ipa ()
-                                                 + srccount.ipa ());
+                 profile_count sum =
+                   dstbb->count.ipa () + srccount.ipa ();
+                 if (sum.nonzero_p ())
+                   dste->probability =
+                     dste->probability * dstbb->count.ipa ().probability_in
+                                                  (dstbb->count.ipa ()
+                                                   + srccount.ipa ())
+                     + srce->probability * srcbb->count.ipa ().probability_in
+                                                  (dstbb->count.ipa ()
+                                                   + srccount.ipa ());
                }
              dstbb->count = dstbb->count.ipa () + srccount.ipa ();
            }
diff --git a/gcc/testsuite/gcc.dg/tree-prof/pr111559.c 
b/gcc/testsuite/gcc.dg/tree-prof/pr111559.c
new file mode 100644
index 00000000000..43202c6c888
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-prof/pr111559.c
@@ -0,0 +1,16 @@
+/* { dg-options "-O2" } */
+
+__attribute__((noipa)) static void edge(void) {}
+
+int p = 0;
+
+__attribute__((noinline))
+static void rule1(void) { if (p) edge(); }
+
+__attribute__((noinline))
+static void rule1_same(void) { if (p) edge(); }
+
+__attribute__((noipa)) int main(void) {
+    rule1();
+    rule1_same();
+}
-- 
2.42.0

Reply via email to