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