From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id E305238381C5 for ; Fri, 25 Nov 2022 20:26:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org E305238381C5 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out1.suse.de (Postfix) with ESMTPS id A5A2021B09; Fri, 25 Nov 2022 20:26:34 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1669407994; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=9qOKXOsz1oHpX46g2uOfOsR40HB1+EI0D4fQazTNx/w=; b=LWrzBvQtCRUsHaOx0h03V5eDbg07exihes5qmt6T7IqGlOkT7sOS2xaxtQy4EMaN3OOgXE uTnUBQNX8VWND5BIbVzj8acJNEyQ3Pdg43g4CG1+9cdo6mlpfR9cCTwt2Y21chx+hCfzfw hRtxxpeQEN23BHgXQJt+rEhafU76CX0= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1669407994; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=9qOKXOsz1oHpX46g2uOfOsR40HB1+EI0D4fQazTNx/w=; b=rJGubGy1bGLMGEEgUMr4aWcKOEiCOZ1UtCrpFBxrv64j6X0RCEIhBKuQ1X9id1h0/XX24A dYyEXDVBwfWK0GCg== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 8ABEA1361C; Fri, 25 Nov 2022 20:26:34 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id AWl9IPokgWODCwAAMHmgww (envelope-from ); Fri, 25 Nov 2022 20:26:34 +0000 Date: Fri, 25 Nov 2022 21:26:34 +0100 (CET) From: Richard Biener X-X-Sender: rguenther@rguenther-XPS-13-9380 To: Jan Hubicka cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] tree-optimization/106912 - IPA profile and pure/const In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (DEB 394 2020-01-19) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323329-1523858103-1669407994=:3568" X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323329-1523858103-1669407994=:3568 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8BIT 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) > > --8323329-1523858103-1669407994=:3568--