On Fri, 25 Nov 2022, Jan Hubicka wrote: >> On Fri, 25 Nov 2022, Jan Hubicka wrote: >> >>>> >>>> >>>>> Am 25.11.2022 um 11:05 schrieb Jan Hubicka via Gcc-patches : >>>>> >>>>>  >>>>>> >>>>>> 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. Hmm, possibly, yes. > 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. Which works for pure (it's not an attribute on types) but fails for const (which is). That's a bug btw. > 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? But ignoring attributes on types makes all indirect calls not const/pure annotatable (OK, they are not pure annotatable because of the above bug). I really don't see how to conservatively solve this issue? Maybe we can ignore all pure/const when the cgraph state is in profile-instrumented state? Of course we have multiple "APIs" to query that. Richard. > 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 >> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, >> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; >> HRB 36809 (AG Nuernberg) > >