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 A06633858D28 for ; Tue, 17 Jan 2023 14:32:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org A06633858D28 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 835022810EE; Tue, 17 Jan 2023 15:32:05 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ucw.cz; s=gen1; t=1673965925; 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=j4WkVIP1366ouaVtoOf3FK3ZSezDsTMV6Xe5jgs5dRM=; b=J/p8OJLX73jb+I+HmioDT7kxIyfy5wUELajpCKvnjU7U/87j3BeqBR6QIQkPPnwgPwug8B +Kkm2oUkJoVfX2UnJQ58wuIg0bJ+C5ZVx59u7jUfRHWs8wqs5zaozi+8W6Wc5CtLfbXbnD q3HwbGyVKNux/BZiz178gVnU56Bgzio= Date: Tue, 17 Jan 2023 15:32:05 +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.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,GIT_PATCH_0,HEADER_FROM_DIFFERENT_DOMAINS,KAM_SHORT,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: > On Tue, 17 Jan 2023, Jan Hubicka wrote: > > > > The following fixes a long-standing bug with DSE removing stores as > > > dead even though they are live across non-call exceptional flow. > > > This affects both GIMPLE and RTL DSE and the fix is similar in > > > making externally throwing statements uses of non-local stores. > > > Note this doesn't fix the GIMPLE side when the throwing statement > > > does not involve a load or a store because then the statement does > > > not have virtual operands and thus is not visited by GIMPLE DSE. > > > > Thanks for looking into this. > > My main motivation for poking on this is the patch to add fnspec to > > throw/catch machinery > > https://gcc.gnu.org/pipermail/gcc-patches/2022-June/597124.html > > > > The eh6.C testcase is misoptimized by current trun with the patch. > > I think I can adjust it for the throwing function to have no vops and it > > will still get misoptimized by DSE. > > I think conceptually the "throw" may not be 'const' - it has to be > 'pure' at most since the program point the control is transfered to > can inspect memory. 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. 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. > > 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)