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 91AE53858D28 for ; Tue, 17 Jan 2023 13:48:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 91AE53858D28 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 975F92810D0; Tue, 17 Jan 2023 14:48:53 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ucw.cz; s=gen1; t=1673963333; 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=RUwrGDQKskoIjWYGTacC/akWnIDiC9I8qQLP8JPYQd0=; b=daG5GU67wzjy1OWYrgT3wYUeUZG+YMhq2g/K/KNkg/yL8OItXCKJCzGxmig5ng4iVtBGAz 0DK3iidTDxPetCqJxICcGxCrqhvJIHypF0iKUDjCwNnggJXLReC1I56ExOprwe/az10IDU x65nS3cYiJBy9oU/PySlwue6cS/KJdo= Date: Tue, 17 Jan 2023 14: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: <20230117101319.BD5DD139C6@imap2.suse-dmz.suse.de> 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: > 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. > > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress. > > This doesn't seem to be a regression and I'm unsure as to how > important it is for Ada (I consider it not important for C/C++), > so for now I'll queue it for next stage1. 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++) 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