From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from nikam.ms.mff.cuni.cz (nikam.ms.mff.cuni.cz [195.113.20.16]) by sourceware.org (Postfix) with ESMTPS id 544963858D28 for ; Tue, 17 Jan 2023 15:48:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 544963858D28 Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=ucw.cz Authentication-Results: sourceware.org; spf=none smtp.mailfrom=kam.mff.cuni.cz Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id DFB5F2810CC; Tue, 17 Jan 2023 16:48:53 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ucw.cz; s=gen1; t=1673970533; h=from:from:reply-to:subject:subject: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=FrV5s16AIgGZzfHSFVSpg2DaMnFOSUoc78qZaNyO5yk=; b=A4qq7U7MejjfzhXRVPKiKOz1beLplkesc2XlGqlASkfG+drTJfW7TWehr8RLENJm94cBRi sjFP3q6rXKmVC8xM9k0zm8n5aVVPR/XWCyyp3yby6iUALAIoAELfwvQZ0WO2vF0x8ZRseq GM1bicKV4KL7lvxBnOt4Kz6e/gaEOJk= Date: Tue, 17 Jan 2023 16:48:53 +0100 From: Jan Hubicka To: Richard Biener Cc: gcc-patches@gcc.gnu.org, ebotcazou@adacore.com Subject: Re: [PATCH] middle-end/106075 - non-call EH and DSE Message-ID: References: <20230117101319.BD5DD139C6@imap2.suse-dmz.suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,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: > > 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. > 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. 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)