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 6B71E3858416 for ; Wed, 18 Jan 2023 08:51:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6B71E3858416 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 99F403EB8F; Wed, 18 Jan 2023 08:51:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1674031875; 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=qBVwuJwTlVfF54Xpi23oDS/nE56ytdC14q4kY8JtY3M=; b=pQqRtZ85C2YpkiDbRz2GgsN2o8UUwJXfqmymyteCGYr7FyFb2uAMoL/ZYtHxfLUcpwvkoC t4rUNGMYUMhL7rsDg+7a5H0n2yxVOjgvfPR8m8NaJUWou3CaDjVttNQTQv702o8YqqdHKP 7+uK24BzeMe96LTr32r+KzxV4HlTLns= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1674031875; 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=qBVwuJwTlVfF54Xpi23oDS/nE56ytdC14q4kY8JtY3M=; b=wD0XKGKPXzKrao7pgarSAkalPkMFuGtj3VpvxeKqbjEZK4u0uFrsMK8PJIVelF3WCnF1cH AzziDiSdyPUCNPBw== 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 8EA532C186; Wed, 18 Jan 2023 08:51:15 +0000 (UTC) Date: Wed, 18 Jan 2023 08:51:15 +0000 (UTC) From: Richard Biener To: Jan Hubicka cc: gcc-patches@gcc.gnu.org, ebotcazou@adacore.com Subject: Re: [PATCH] middle-end/106075 - non-call EH and DSE In-Reply-To: Message-ID: References: <20230117101319.BD5DD139C6@imap2.suse-dmz.suse.de> 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=-11.1 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: On Tue, 17 Jan 2023, Jan Hubicka wrote: > > > We don't use same argumentation about other control flow statements. > > > The following: > > > > > > fn() > > > { > > > try { > > > i_read_no_global_memory (); > > > } catch (...) > > > { > > > reutrn 1; > > > } > > > return 0; > > > } > > > > > > should be detected as const. Marking throw pure would make fn pure too. > > > > I suppose i_read_no_global_memory is const here. Not sure why that > Suppose we have: > > void > i_read_no_global_memory () > { > throw(0); > } > > If cxa_throw itself was annotated as 'p' rahter than 'c' ipa-modref will > believe that cxa_throw will read any global memory and will propagate it > to all callers. So fn() will be also marked as reading all global > memory. Sure - but for the purpose of local optimizations in i_read_no_global_memory cxa_throw has to appear to read memory. Having a VUSE there dependent on whether the function performs any load or store would be quite ugly. Instead modref could special-case cxa_throw and not treat it as reading memory (like it already does for the return stmt I suppose - that also has a VUSE). > > should make it pure? Only if anything throws externally (not catched > > in fn) should force it to be pure, no? > > > > Of course for IPA purposes whether 'fn' is to be considered const > > or pure depends on whether its exceptions are catched in the context > > where that's interesting - that is, whether the EH side-effect is > > explicitely or implicitely modeled. > > We have two things here. const/pure attributes 'c'/'p' fnspec > specifiers. const/pure implies that function call can be removed when > result is not necessary. This is not the case of funcitons calling > throw() (we have -fdelete-dead-exceptions for noncall exceptions and > those would be OK). However 'c'/'p' is about memory side effects only > and it is safe for i_read_no_global_memory. > > With the C++ FE change adding fnspec to EH handling modref will detect > both i_read_no_global_memory and fn() as 'c'. It won't infer const > attribute that is something I can implement later. > We are very poor on detecting scenarios where all exceptions thrown are > actually caught. It is long time on my TODO to fix that, so probably > next stage1 is time to look into that. > > > > > > With noncall exceptions a=b/c also can transfer to place that inspect > > > memory. We may want all statements with can_throw_extenral to have VUSE > > > on them (like with return) since they may cause function to return, but > > > I think fnspec is wrong place to model this. > > > > Yes, I think all control transfer instructions need a VUSE. > > I think it is right way to go. So operands_scanner::parse_ssa_operands > can add vuse to anything that can_throw_external_p (like it does for > GIMPLE_RETURN) and passes like DSE can test for it and understand that > on the EH path the globally accessible memory is live and thus "used" by > the statement. > > I can try to cook up a patch. The problem is IIRC GIMPLE_RESX which doesn't derive from gimple_statement_with_memory_ops_base. There's a bugzilla I can't find right now refering to this issue. Richard. > Thanks, > Honza > > > > Richard. > > > > > > > According to compiler explorer testcase: > > > > > struct a{int a,b,c,d,e;}; > > > > > void > > > > > test(struct a * __restrict a, struct a *b) > > > > > { > > > > > *a = (struct a){0,1,2,3,4}; > > > > > *a = *b; > > > > > } > > > > > Is compiled correctly by GCC 5.4 and first miscopmiled by 6.1, so I > > > > > think it is a regression. (For C++ not very important one as > > > > > -fnon-call-exceptions is not very common for C++) > > > > > > > > Ah, yes - RTL DSE probably is too weak for this and GIMPLE DSE > > > > didn't handle aggregates well at some point. > > > > > > Yep, we never handled it really correctly but were weaker on optimizing > > > and thus also producing wrong code :) > > > > > > Honza > > > > > > > > Richard. > > > > > > > > > > > > > > Honza > > > > > > > > > > > > PR middle-end/106075 > > > > > > * dse.cc (scan_insn): Consider externally throwing insns > > > > > > to read from not frame based memory. > > > > > > * tree-ssa-dse.cc (dse_classify_store): Consider externally > > > > > > throwing uses to read from global memory. > > > > > > > > > > > > * gcc.dg/torture/pr106075-1.c: New testcase. > > > > > > --- > > > > > > gcc/dse.cc | 5 ++++ > > > > > > gcc/testsuite/gcc.dg/torture/pr106075-1.c | 36 +++++++++++++++++++++++ > > > > > > gcc/tree-ssa-dse.cc | 8 ++++- > > > > > > 3 files changed, 48 insertions(+), 1 deletion(-) > > > > > > create mode 100644 gcc/testsuite/gcc.dg/torture/pr106075-1.c > > > > > > > > > > > > diff --git a/gcc/dse.cc b/gcc/dse.cc > > > > > > index a2db8d1cc32..7e258b81f66 100644 > > > > > > --- a/gcc/dse.cc > > > > > > +++ b/gcc/dse.cc > > > > > > @@ -2633,6 +2633,11 @@ scan_insn (bb_info_t bb_info, rtx_insn *insn, int max_active_local_stores) > > > > > > return; > > > > > > } > > > > > > > > > > > > + /* An externally throwing statement may read any memory that is not > > > > > > + relative to the frame. */ > > > > > > + if (can_throw_external (insn)) > > > > > > + add_non_frame_wild_read (bb_info); > > > > > > + > > > > > > /* Assuming that there are sets in these insns, we cannot delete > > > > > > them. */ > > > > > > if ((GET_CODE (PATTERN (insn)) == CLOBBER) > > > > > > diff --git a/gcc/testsuite/gcc.dg/torture/pr106075-1.c b/gcc/testsuite/gcc.dg/torture/pr106075-1.c > > > > > > new file mode 100644 > > > > > > index 00000000000..b9affbf1082 > > > > > > --- /dev/null > > > > > > +++ b/gcc/testsuite/gcc.dg/torture/pr106075-1.c > > > > > > @@ -0,0 +1,36 @@ > > > > > > +/* { dg-do run { target *-*-linux* } } */ > > > > > > +/* { dg-additional-options "-fnon-call-exceptions" } */ > > > > > > + > > > > > > +#include > > > > > > +#include > > > > > > +#include > > > > > > + > > > > > > +int a = 1; > > > > > > +short *b; > > > > > > +void __attribute__((noipa)) > > > > > > +test() > > > > > > +{ > > > > > > + a=12345; > > > > > > + *b=0; > > > > > > + a=1; > > > > > > +} > > > > > > + > > > > > > +void check (int i) > > > > > > +{ > > > > > > + if (a != 12345) > > > > > > + abort (); > > > > > > + exit (0); > > > > > > +} > > > > > > + > > > > > > +int > > > > > > +main () > > > > > > +{ > > > > > > + struct sigaction s; > > > > > > + sigemptyset (&s.sa_mask); > > > > > > + s.sa_handler = check; > > > > > > + s.sa_flags = 0; > > > > > > + sigaction (SIGSEGV, &s, NULL); > > > > > > + test(); > > > > > > + abort (); > > > > > > + return 0; > > > > > > +} > > > > > > diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc > > > > > > index 46ab57d5754..b2e2359c3da 100644 > > > > > > --- a/gcc/tree-ssa-dse.cc > > > > > > +++ b/gcc/tree-ssa-dse.cc > > > > > > @@ -960,6 +960,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt, > > > > > > auto_bitmap visited; > > > > > > std::unique_ptr > > > > > > dra (nullptr, free_data_ref); > > > > > > + bool maybe_global = ref_may_alias_global_p (ref, false); > > > > > > > > > > > > if (by_clobber_p) > > > > > > *by_clobber_p = true; > > > > > > @@ -1038,6 +1039,11 @@ dse_classify_store (ao_ref *ref, gimple *stmt, > > > > > > last_phi_def = as_a (use_stmt); > > > > > > } > > > > > > } > > > > > > + /* If the stmt can throw externally and the store is > > > > > > + visible in the context unwound to the store is live. */ > > > > > > + else if (maybe_global > > > > > > + && stmt_can_throw_external (cfun, use_stmt)) > > > > > > + return DSE_STORE_LIVE; > > > > > > /* If the statement is a use the store is not dead. */ > > > > > > else if (ref_maybe_used_by_stmt_p (use_stmt, ref)) > > > > > > { > > > > > > @@ -1116,7 +1122,7 @@ dse_classify_store (ao_ref *ref, gimple *stmt, > > > > > > just pretend the stmt makes itself dead. Otherwise fail. */ > > > > > > if (defs.is_empty ()) > > > > > > { > > > > > > - if (ref_may_alias_global_p (ref, false)) > > > > > > + if (maybe_global) > > > > > > return DSE_STORE_LIVE; > > > > > > > > > > > > if (by_clobber_p) > > > > > > -- > > > > > > 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) > > > > > > > -- > > 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) > -- 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)