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 DEFD73858D28 for ; Tue, 11 Apr 2023 08:15:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org DEFD73858D28 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 0F8BF21AB3; Tue, 11 Apr 2023 08:15:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1681200955; 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=nBhiZDIpF4s6uCY1WB2YkuwmJ3AZoxCftHE4ekW8yhE=; b=tpsZ8HLjcb9pCaWti0PEwSnkucj5WsLyN9Z9RhA5Q+U+mpRTpjlr+x7POB0b6u1EwoTelC 5t8EyDIqOFC5Ho3r7766xhU/A87u4liZgULPYkGcstu0duobrxS0xkYSc2QREa/zAtzd4q yFr7UnYCLlOU2UfSbPZtbeO7Pryhl9k= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1681200955; 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=nBhiZDIpF4s6uCY1WB2YkuwmJ3AZoxCftHE4ekW8yhE=; b=9Y0uHRxZQw7pGzUMwd+6BDbM0fPEXJuDgxGzL01pY89y10WM/vn8lo3b0MPEIi0lOfv2yC n1JJ9K1LU/OVd4AA== 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 042C22C16F; Tue, 11 Apr 2023 08:15:54 +0000 (UTC) Date: Tue, 11 Apr 2023 08:15:54 +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 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 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? 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? Richard.