From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [195.135.220.28]) by sourceware.org (Postfix) with ESMTPS id 0F7D03858CDB for ; Mon, 17 Apr 2023 06:35:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0F7D03858CDB 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 relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 1CC2B21A3F; Mon, 17 Apr 2023 06:35:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1681713347; 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=Af6Wam0CzJ+daZd6q40bj+Yf8ejSpBIUkzbR0XuY7WY=; b=xLPcxcYwsNudecAYezEBYlIEkjhwNnP++F2DqgQIyr0EXskez2ezfXXjL4Iuk9UT+cBi50 xIh1frb9FYqOy6ozIyQVj3S+72BIhs3Nkkt5Kun+hjoM9Dz471wiz3olvSdBdD8834lvO5 QjdCpH76+IjWPpnLduwr6m1V/tN/q40= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1681713347; 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=Af6Wam0CzJ+daZd6q40bj+Yf8ejSpBIUkzbR0XuY7WY=; b=icNbS8DbMFLGXTSzkKKbCMd0FFOffAVsvU+nUHE42cbqTAkRSFZXh8evsD0ltjxxmA6kHB zr280CZGjTbUnuDQ== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 02ED52C141; Mon, 17 Apr 2023 06:35:46 +0000 (UTC) Date: Mon, 17 Apr 2023 06:35:46 +0000 (UTC) From: Richard Biener To: Jan Hubicka cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] tree-optimization/109304 - properly handle instrumented aliases In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-5.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE 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: On Fri, 14 Apr 2023, Jan Hubicka wrote: > > On Tue, 4 Apr 2023, Jan Hubicka wrote: > > > > > > On Tue, 28 Mar 2023, Richard Biener wrote: > > > > > > > > > When adjusting calls to reflect instrumentation we failed to handle > > > > > calls to aliases since they appear to have no body. Instead resort > > > > > to symtab node availability. The patch also avoids touching > > > > > internal function calls in a more obvious way (builtins might > > > > > have a body available). > > > > > > > > > > profiledbootstrap & regtest running on x86_64-unknown-linux-gnu. > > > > > > > > > > Honza - does this look OK? > > > > > PR tree-optimization/109304 > > > > > * tree-profile.cc (tree_profiling): Use symtab node > > > > > availability to decide whether to skip adjusting calls. > > > > > Do not adjust calls to internal functions. > > > > > @@ -842,12 +842,15 @@ tree_profiling (void) > > > > > for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi)) > > > > > { > > > > > gcall *call = dyn_cast (gsi_stmt (gsi)); > > > > > - if (!call) > > > > > + if (!call || gimple_call_internal_p (call)) > > > > > continue; > > > > > > > > > > /* We do not clear pure/const on decls without body. */ > > > > > tree fndecl = gimple_call_fndecl (call); > > > > > - if (fndecl && !gimple_has_body_p (fndecl)) > > > > > + cgraph_node *callee; > > > > > + if (fndecl > > > > > + && (callee = cgraph_node::get (fndecl)) > > > > > + && callee->get_availability (node) == AVAIL_NOT_AVAILABLE) > > > > > > As discussed earlier, the testcase I posted can be adjusted to put the > > > const declared wrapper into another translation unit, so I think we will > > > need to drop the visibility check completely. But as discussed, it is > > > wrong code issue, but not a regression, so we may go with the > > > availability check as you suggest. So the patch is OK. > > > > > > > > > I wonder if we do not want to drop it everywhere (as we plan for next > > > stage1 anyway). I think similar ICE as in the PR can be produced with > > > LTO. In normal situation declaration merging will do the right thing: > > > If you have unit A calling const foo externally, it won't get processed > > > by the code above. However unit B declaring foo will get it downgraded > > > to non-const. > > > > > > Now at WPA time we will read both A and B and in declaration merging B's > > > definition will prevail. This won't happen if lto_symtab_merge_p > > > returns false which can probably be triggered by adding warning/error > > > attribute to B's declaration but not to A's. > > > > > > It is however really side case and I am worried about dropping > > > pure/const from builtin declarations... > > > > Yeah, that's what I'm worried about as well. I guess we'd need to > > do the demotion to non-const/pure at WPA time and have a flag > > in the cgraph node indicating instrument_add_{read,write}? But > > then how should we deal with C++ comdats instrumented in one TU > > but not in another? Does this mean we should do instrumentation > > at IPA time instead of as small IPA pass before IPA? > > I do not think LTO is of any help here. You can allways call non-LTO > const function from outer-world and that function can will end up > calling back to instrumented const function in your unit which > effectively makes the extenral const function non-const. Hmm, true. > > > > That said, when there's a definition of say strlen in a TU and > > that's instrumented we do want to drop pure from calls but if > > not then we shouldn't worry. > > > > Without LTO we'd still run into coverage issues but at least > > with LTO we shouldn't ICE? > > I am not sure I see your point here... > We could avoid demoting builtins to avoid ICEs and have coverage > mismathces, but how LTO makes difference? At least we get more functions local, but yes, we can still trigger the issue. So what's the solution? All functions that are not leaf or possibly instrumented have to be called as if they were not pure/const, including builtins? As we've said we're going to ICE quite a bit when const/pure builtins suddenly are no longer const/pure. Richard.