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

[Richard Biener <rguenther@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 4595 bytes --]

On Fri, 25 Nov 2022, Jan Hubicka wrote:

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

Or by doing the first call as

volatile int __attribute__((const)) (*foop)(int) = foo;

      a += (*foop) (a);

you'd need to get all calls that might possibly call an instrumented
function adjusted.

I think if we're taking this issue serious we'd have to simply
ignore const/pure attributes at parsing time and refrain from
auto-detecting as well, at least for address-taken functions?

That said, this adjustment in the "wrong" direction causes problems
downstream, which is what the fixed PR is about (simd cloning picks
up the wrong things, or rather possibly fails to clone the attributes?).

Richard.

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

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)