From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 78764 invoked by alias); 19 Feb 2020 16:28:18 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 78752 invoked by uid 89); 19 Feb 2020 16:28:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_SHORT,SPF_PASS autolearn=ham version=3.3.1 spammy=papers, avail, primary, 93707 X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 19 Feb 2020 16:28:16 +0000 X-Amavis-Alert: BAD HEADER SECTION, Duplicate header field: "Cc" Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id DD5E4AC42; Wed, 19 Feb 2020 16:28:13 +0000 (UTC) From: Martin Jambor To: Feng Xue OS , Tamar Christina , Jan Hubicka , "gcc-patches\@gcc.gnu.org" Cc: nd Cc: Subject: Re: [PATCH] Fix bug in recursiveness check for function to be cloned (ipa/pr93707) In-Reply-To: References: User-Agent: Notmuch/0.29.3 (https://notmuchmail.org) Emacs/26.3 (x86_64-suse-linux-gnu) Date: Wed, 19 Feb 2020 16:28:00 -0000 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-IsSubscribed: yes X-SW-Source: 2020-02/txt/msg01110.txt.bz2 Hi, On Thu, Feb 13 2020, Feng Xue OS wrote: > I've submitted a bug tracker, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93707. > > The root cause is that for a self-recursive function, a for-all-contexts clone could generate > an edge whose callee is not the function. Therefore, to check whether an edge stands for a > recursive call during cloning, we should not use ordinary way of comparing caller and callee > of the edge. > > Bootstrapped/regtested on x86_64-linux and aarch64-linux, and also tested Spec 2017 with > option "--param ipa-cp-eval-threshold=1". > > Feng > --- > 2020-02-13 Feng Xue > > PR ipa/93707 > * ipa-cp.c (self_recursive_pass_through_p): Add a new parameter "node", > and check recursiveness by comparing "node" and cs->caller. > (self_recursive_agg_pass_through_p): Likewise. > (find_more_scalar_values_for_callers_subset): Add "node" argument to > self_recursive_pass_through_p call. > (intersect_aggregates_with_edge): Add a new parameter "node", and add > "node" argument to self_cursive_agg_pass_through_p call. > (find_aggregate_values_for_callers_subset): Add "node" argument to > self_recursive_pass_through_p and intersect_aggregates_with_edge calls. > (cgraph_edge_brings_all_agg_vals_for_node): Add "node" argument to > intersect_aggregates_with_edge call. first, thanks for a very nice testcase and for working on this. However, I believe that his approach mostly papers over a bug that happens earlier, specifically that cgraph_edge_brings_value_p returned true for the self-recursive edge from all-context clone to itself, even though when evaluating the second argument. We assume that all context clones get at least as many constants as any other potential clone, but that does not work for self-recursive edges with pass-through parameters that that just pass along the received constant. That is how we got a wrong edge in the vector of edges bringing the constant and it is the primary reason why self_recursive_pass_through_p and its users did not work correctly. I would therefore like to fix the problem with the following patch. It has passed bootstrap and testing on x86_64-linux, LTO bootstrap is underway. Honza, is it OK for trunk? Tamar, can you please double check it fixes your problem with perlbench? Thanks, Martin ipa-cp: Avoid wrongly gathering self-recursive edges (PR 93707) 2020-02-18 Martin Jambor Feng Xue PR ipa/93707 * ipa-cp.c (same_node_or_its_all_contexts_clone_p): Replaced with new function calls_same_node_or_its_all_contexts_clone_p. (cgraph_edge_brings_value_p): Use it. Fix comment. (cgraph_edge_brings_value_p): Likewise. testsuite/ * gcc.dg/ipa/pr93707.c: New test. --- gcc/ChangeLog | 9 +++++++++ gcc/ipa-cp.c | 29 ++++++++++++++--------------- gcc/testsuite/ChangeLog | 6 ++++++ gcc/testsuite/gcc.dg/ipa/pr93707.c | 29 +++++++++++++++++++++++++++++ 4 files changed, 58 insertions(+), 15 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/ipa/pr93707.c diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c index 4f5b72e6994..4609375bf8d 100644 --- a/gcc/ipa-cp.c +++ b/gcc/ipa-cp.c @@ -4033,15 +4033,23 @@ edge_clone_summary_t::duplicate (cgraph_edge *src_edge, cgraph_edge *dst_edge, src_data->next_clone = dst_edge; } -/* Return true is NODE is DEST or its clone for all contexts. */ +/* Return true is CS calls DEST or its clone for all contexts, except for + self-recursive nodes in which it has to be DEST itself. */ static bool -same_node_or_its_all_contexts_clone_p (cgraph_node *node, cgraph_node *dest) +calls_same_node_or_its_all_contexts_clone_p (cgraph_edge *cs, cgraph_node *dest) { - if (node == dest) + enum availability availability; + cgraph_node *callee = cs->callee->function_symbol (&availability); + + if (availability <= AVAIL_INTERPOSABLE) + return false; + if (callee == dest) return true; + if (cs->caller == callee) + return false; - class ipa_node_params *info = IPA_NODE_REF (node); + class ipa_node_params *info = IPA_NODE_REF (callee); return info->is_all_contexts_clone && info->ipcp_orig_node == dest; } @@ -4053,11 +4061,8 @@ cgraph_edge_brings_value_p (cgraph_edge *cs, ipcp_value_source *src, cgraph_node *dest, ipcp_value *dest_val) { class ipa_node_params *caller_info = IPA_NODE_REF (cs->caller); - enum availability availability; - cgraph_node *real_dest = cs->callee->function_symbol (&availability); - if (availability <= AVAIL_INTERPOSABLE - || !same_node_or_its_all_contexts_clone_p (real_dest, dest) + if (!calls_same_node_or_its_all_contexts_clone_p (cs, dest) || caller_info->node_dead) return false; @@ -4076,9 +4081,6 @@ cgraph_edge_brings_value_p (cgraph_edge *cs, ipcp_value_source *src, } else { - /* At the moment we do not propagate over arithmetic jump functions in - SCCs, so it is safe to detect self-feeding recursive calls in this - way. */ if (src->val == dest_val) return true; @@ -4113,11 +4115,8 @@ cgraph_edge_brings_value_p (cgraph_edge *cs, ipcp_value *) { class ipa_node_params *caller_info = IPA_NODE_REF (cs->caller); - enum availability avail; - cgraph_node *real_dest = cs->callee->function_symbol (&avail); - if (avail <= AVAIL_INTERPOSABLE - || !same_node_or_its_all_contexts_clone_p (real_dest, dest) + if (!calls_same_node_or_its_all_contexts_clone_p (cs, dest) || caller_info->node_dead) return false; if (!src->val) diff --git a/gcc/testsuite/gcc.dg/ipa/pr93707.c b/gcc/testsuite/gcc.dg/ipa/pr93707.c new file mode 100644 index 00000000000..e200a3a432b --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/pr93707.c @@ -0,0 +1,29 @@ +/* { dg-do compile } */ +/* { dg-options "-O3 --param ipa-cp-eval-threshold=1" } */ + +int foo(); +int data[100]; + +__attribute__((noinline)) static int recur_fn (int i, int j, int depth) +{ + if (depth > 10) + return 1; + + data[i + j]++; + + if (depth & 3) + recur_fn (i, 1, depth + 1); + else + recur_fn (i, j & 1, depth + 1); + + foo(); + + return i + j; +} + +int caller (int v, int depth) +{ + recur_fn (1, v, depth); + + return 0; +} -- 2.25.0