> 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?
I think that is also not a good idea, since we would have to do that with -fprofile-use, too (so the CFG at the instrumentation time matches) and it would lead to poor perofrmance with FDO. The idea was to honor pure/const during early opt and undo the attributes when profiling is inserted. We have fixup_cfg to compensate for attribute changes. We could probably keep tract if any instrumented code was ever inlined into a given function and perhaps just start ignoring attributes set on types? Honza > > 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 <rguent...@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)