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 AE7633855005 for ; Fri, 2 Jul 2021 09:50:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AE7633855005 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.cz Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 769AB22006 for ; Fri, 2 Jul 2021 09:50:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1625219410; 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=Jy/2QlN+CZoIif0Q4aXRF3utsU0rN+Je/m9Rkbx2+i8=; b=XeWIPujyXFzj8IWLxyt0u2qsszUDBqnNa0lUTtnVXhLaqU7XIhGbdRmP6S748rHdvPEl61 MKsDEwfveNFSjFcmpDeqaT/DlUwCJNJ/bhGM3APDXMMS0+QxxXDDqqloh7EqCftOETOQlv 8chRyYP9SRi4SDsVtyfpIn10hy4O94Q= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1625219410; 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=Jy/2QlN+CZoIif0Q4aXRF3utsU0rN+Je/m9Rkbx2+i8=; b=rUmfpW3cAxzery4kM27hTQMctgVE7S9VYIp+oVzRQuh+AohRMggj/bUeyHlu3fMgjTFIAM Z6YYB5ZH+P56TiDA== Received: from suse.cz (virgil.suse.cz [10.100.13.50]) (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 62F83A3B89; Fri, 2 Jul 2021 09:50:10 +0000 (UTC) From: Martin Jambor To: GCC Patches Cc: Jan Hubicka , Martin Liska Subject: Re: [PATCH] ipa-sra: Fix thinko when overriding safe_to_import_accesses (PR 101066) In-Reply-To: References: User-Agent: Notmuch/0.32 (https://notmuchmail.org) Emacs/27.2 (x86_64-suse-linux-gnu) Date: Fri, 02 Jul 2021 11:50:10 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-10.6 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.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Jul 2021 09:50:13 -0000 Ping. On Mon, Jun 21 2021, Martin Jambor wrote: > Hi, > > The "new" IPA-SRA has a more difficult job than the previous > not-truly-IPA version when identifying situations in which a parameter > passed by reference can be passed into a third function and only thee > converted to one passed by value (and possibly "split" at the same > time). > > In order to allow this, two conditions must be fulfilled. First the > call to the third function must happen before any modifications of > memory, because it could change the value passed by reference. > Second, in order to make sure we do not introduce new (invalid) > dereferences, the call must postdominate the entry BB. > > The second condition is actually not necessary if the caller function > is also certain to dereference the pointer but the first one must > still hold. Unfortunately, the code making this overriding decision > also happen to trigger when the first condition is not fulfilled. > This is fixed in the following patch. > > Bootstrapped, LTO-bootstrapped and tested on x86_64-linux, OK for trunk > and the gcc-11 branch? On gcc-10, I might just remove the override > altogether, the case might not be important enough to change LTO format. > > Thanks, > > Martin > > > > gcc/ChangeLog: > > 2021-06-16 Martin Jambor > > PR ipa/101066 > * ipa-sra.c (class isra_call_summary): New member > m_before_any_store, initialize it in the constructor. > (isra_call_summary::dump): Dump the new field. > (ipa_sra_call_summaries::duplicate): Copy it. > (process_scan_results): Set it. > (isra_write_edge_summary): Stream it. > (isra_read_edge_summary): Likewise. > (param_splitting_across_edge): Only override > safe_to_import_accesses if m_before_any_store is set. > > gcc/testsuite/ChangeLog: > > 2021-06-16 Martin Jambor > > PR ipa/101066 > * gcc.dg/ipa/pr101066.c: New test. > --- > gcc/ipa-sra.c | 15 +++++++++++++-- > gcc/testsuite/gcc.dg/ipa/pr101066.c | 20 ++++++++++++++++++++ > 2 files changed, 33 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/ipa/pr101066.c > > diff --git a/gcc/ipa-sra.c b/gcc/ipa-sra.c > index 3272daf56e4..965e246d788 100644 > --- a/gcc/ipa-sra.c > +++ b/gcc/ipa-sra.c > @@ -343,7 +343,7 @@ class isra_call_summary > public: > isra_call_summary () > : m_arg_flow (), m_return_ignored (false), m_return_returned (false), > - m_bit_aligned_arg (false) > + m_bit_aligned_arg (false), m_before_any_store (false) > {} > > void init_inputs (unsigned arg_count); > @@ -362,6 +362,10 @@ public: > > /* Set when any of the call arguments are not byte-aligned. */ > unsigned m_bit_aligned_arg : 1; > + > + /* Set to true if the call happend before any (other) store to memory in the > + caller. */ > + unsigned m_before_any_store : 1; > }; > > /* Class to manage function summaries. */ > @@ -491,6 +495,8 @@ isra_call_summary::dump (FILE *f) > fprintf (f, " return value ignored\n"); > if (m_return_returned) > fprintf (f, " return value used only to compute caller return value\n"); > + if (m_before_any_store) > + fprintf (f, " happens before any store to memory\n"); > for (unsigned i = 0; i < m_arg_flow.length (); i++) > { > fprintf (f, " Parameter %u:\n", i); > @@ -535,6 +541,7 @@ ipa_sra_call_summaries::duplicate (cgraph_edge *, cgraph_edge *, > new_sum->m_return_ignored = old_sum->m_return_ignored; > new_sum->m_return_returned = old_sum->m_return_returned; > new_sum->m_bit_aligned_arg = old_sum->m_bit_aligned_arg; > + new_sum->m_before_any_store = old_sum->m_before_any_store; > } > > > @@ -2374,6 +2381,7 @@ process_scan_results (cgraph_node *node, struct function *fun, > unsigned count = gimple_call_num_args (call_stmt); > isra_call_summary *csum = call_sums->get_create (cs); > csum->init_inputs (count); > + csum->m_before_any_store = uses_memory_as_obtained; > for (unsigned argidx = 0; argidx < count; argidx++) > { > if (!csum->m_arg_flow[argidx].pointer_pass_through) > @@ -2546,6 +2554,7 @@ isra_write_edge_summary (output_block *ob, cgraph_edge *e) > bp_pack_value (&bp, csum->m_return_ignored, 1); > bp_pack_value (&bp, csum->m_return_returned, 1); > bp_pack_value (&bp, csum->m_bit_aligned_arg, 1); > + bp_pack_value (&bp, csum->m_before_any_store, 1); > streamer_write_bitpack (&bp); > } > > @@ -2664,6 +2673,7 @@ isra_read_edge_summary (struct lto_input_block *ib, cgraph_edge *cs) > csum->m_return_ignored = bp_unpack_value (&bp, 1); > csum->m_return_returned = bp_unpack_value (&bp, 1); > csum->m_bit_aligned_arg = bp_unpack_value (&bp, 1); > + csum->m_before_any_store = bp_unpack_value (&bp, 1); > } > > /* Read intraprocedural analysis information about NODE and all of its outgoing > @@ -3420,7 +3430,8 @@ param_splitting_across_edge (cgraph_edge *cs) > } > else if (!ipf->safe_to_import_accesses) > { > - if (!all_callee_accesses_present_p (param_desc, arg_desc)) > + if (!csum->m_before_any_store > + || !all_callee_accesses_present_p (param_desc, arg_desc)) > { > if (dump_file && (dump_flags & TDF_DETAILS)) > fprintf (dump_file, " %u->%u: cannot import accesses.\n", > diff --git a/gcc/testsuite/gcc.dg/ipa/pr101066.c b/gcc/testsuite/gcc.dg/ipa/pr101066.c > new file mode 100644 > index 00000000000..1ceb6e43136 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/ipa/pr101066.c > @@ -0,0 +1,20 @@ > +/* { dg-do run } */ > +/* { dg-options "-Os -fno-ipa-cp -fno-inline" } */ > + > +int a = 1, c, d, e; > +int *b = &a; > +static int g(int *h) { > + c = *h; > + return d; > +} > +static void f(int *h) { > + e = *h; > + *b = 0; > + g(h); > +} > +int main() { > + f(b); > + if (c) > + __builtin_abort(); > + return 0; > +} > -- > 2.31.1