Re: [PATCH] tree-optimization/106912 - IPA profile and pure/const

[Jan Hubicka <hubicka@ucw.cz>]

> 
> 
> > Am 25.11.2022 um 11:05 schrieb Jan Hubicka via Gcc-patches <gcc-patches@gcc.gnu.org>:
> > 
> > 
> >> 
> >> IPA profile instrumentation tries to clear the pure and const
> >> flags of functions but that's quite hopeless in particular for
> >> const since that attribute prevails on the type and thus on each
> >> call to the function leading to inconsistencies in the IL and
> >> eventual checking ICEs.  There's no good reason to do this and
> >> it wouldn't fixup any indirect calls so just don't.  No other
> >> instrumentation GCC does bothers about this.
> > 
> > This was mostly meant to deadl with situation where we auto-detect
> > function to be const and then partially inline it to a loop.
> > Then both caller and callee accesses same counters in the memory and if
> > you move load/stores out of the loop in caller you lose counters and get
> > inconsistencies at profile read-in time.
> 
> Don’t we Instrument after partial inlining now?  As said, since we have the fntype on the call this doesn’t work anymore for const functions via attributes.

Full inlining can cause problem already.  So for code like

do
{
  if (__builtin_expect (test,1))
    a+=foo (a);
  else
    a+=foo (b);
} while (....);
we may end up inlining one of the two invocation. Then caller and callee
will modify the same counter.  If we handle the remaining call as const,
we can hoist the counter modifications out of the loop and mix them up.

I remember I run into an actual example of this problem during GCC
bootstrap. There the function was auto-detected to be const by
early pure-const pass so type was not an problem. You are right we ought
to do something about types since the scenario above can happen with foo
being declared with an attribute as well.

Honza
> 
> Richard 
> > Honza
> >> 
> >> Bootstrap and regtest pending on x86_64-unknown-linux-gnu, OK?
> >> 
> >> Thanks,
> >> Richard.
> >> 
> >>    PR tree-optimization/106912
> >>    * tree-profile.cc (tree_profiling): Do not clear pure/const
> >>    flags.
> >> 
> >>    * gcc.dg/pr106912.c: New testcase.
> >> ---
> >> gcc/testsuite/gcc.dg/pr106912.c | 16 ++++++++++++++++
> >> gcc/tree-profile.cc             |  3 ---
> >> 2 files changed, 16 insertions(+), 3 deletions(-)
> >> create mode 100644 gcc/testsuite/gcc.dg/pr106912.c
> >> 
> >> diff --git a/gcc/testsuite/gcc.dg/pr106912.c b/gcc/testsuite/gcc.dg/pr106912.c
> >> new file mode 100644
> >> index 00000000000..8faa877d8b3
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.dg/pr106912.c
> >> @@ -0,0 +1,16 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-O2 -fPIC -ftree-vectorize -fprofile-generate" } */
> >> +
> >> +__attribute__ ((__simd__))
> >> +__attribute__ ((__nothrow__ , __leaf__ , __const__))
> >> +double foo (double x);
> >> +void bar(double *f, int n)
> >> +{
> >> +  int i;
> >> +  for (i = 0; i < n; i++)
> >> +    f[i] = foo(f[i]);
> >> +}
> >> +double foo(double x)
> >> +{
> >> +  return x * x / 3.0;
> >> +}
> >> diff --git a/gcc/tree-profile.cc b/gcc/tree-profile.cc
> >> index 2beb49241f2..5491b398870 100644
> >> --- a/gcc/tree-profile.cc
> >> +++ b/gcc/tree-profile.cc
> >> @@ -814,9 +814,6 @@ tree_profiling (void)
> >>    /* Don't profile functions produced for builtin stuff.  */
> >>    if (DECL_SOURCE_LOCATION (node->decl) == BUILTINS_LOCATION)
> >>      continue;
> >> -
> >> -    node->set_const_flag (false, false);
> >> -    node->set_pure_flag (false, false);
> >>       }
> >> 
> >>   /* Update call statements and rebuild the cgraph.  */
> >> -- 
> >> 2.35.3