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

[Richard Biener <rguenther@suse.de>]

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



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

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