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 8895D3858D20 for ; Tue, 22 Nov 2022 14:20:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8895D3858D20 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 2E9E8284AB2; Tue, 22 Nov 2022 15:20:32 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ucw.cz; s=gen1; t=1669126832; 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=5NZVvX0YJsptK5GbDWgT76/ZY2O+3sCS74KipI5putI=; b=VW72vPAl8tgxIg48LAPc7IJfa8UbRD8+F6CH5M+JwMSZH5g4jY5UWi7Rz/x+/DaKpcFOWc Is6AtBcNfx79ipGYoPnP/2ossdAUFOaKjS6qa8x0r0x1UCteLUwRoao1OJJkbH1ZWSnAVJ aMCJbLsuMzim2euluwef4xKOOwD3CjU= Date: Tue, 22 Nov 2022 15:20:32 +0100 From: Jan Hubicka To: Martin Jambor Cc: GCC Patches Subject: Re: [PATCH] ipa-cp: Do not be too optimistic about self-recursive edges (PR 107661) Message-ID: References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Spam-Status: No, score=-10.8 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: > Hi, > > PR 107661 shows that function push_agg_values_for_index_from_edge > should not attempt to optimize self-recursive call graph edges when > called from cgraph_edge_brings_all_agg_vals_for_node. Unlike when > being called from find_aggregate_values_for_callers_subset, we cannot > expect that any cloning for constants would lead to the edge leading > from a new clone to the same new clone, in this case it would only be > redirected to a new callee. > > Fixed by adding a parameter to push_agg_values_from_edge whether being > optimistic about self-recursive edges is possible. > > Bootstrapped, LTO-bootstrapped and tested on x86_64-linux. OK for > trunk? OK, thanks! Honya > > Thanks, > > Martin > > > gcc/ChangeLog: > > 2022-11-22 Martin Jambor > > PR ipa/107661 > * ipa-cp.cc (push_agg_values_from_edge): New parameter > optimize_self_recursion, use it to decide whether to pass interim to > the helper function. > (find_aggregate_values_for_callers_subset): Pass true in the new > parameter of push_agg_values_from_edge. > (cgraph_edge_brings_all_agg_vals_for_node): Pass false in the new > parameter of push_agg_values_from_edge. > > gcc/testsuite/ChangeLog: > > 2022-11-22 Martin Jambor > > PR ipa/107661 > * g++.dg/ipa/pr107661.C: New test. > --- > gcc/ipa-cp.cc | 18 +++++++----- > gcc/testsuite/g++.dg/ipa/pr107661.C | 45 +++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+), 7 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/ipa/pr107661.C > > diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc > index d2bcd5e5e69..f0feb4beb8f 100644 > --- a/gcc/ipa-cp.cc > +++ b/gcc/ipa-cp.cc > @@ -5752,14 +5752,16 @@ push_agg_values_for_index_from_edge (struct cgraph_edge *cs, int index, > description of ultimate callee of CS or the one it was cloned from (the > summary where lattices are). If INTERIM is non-NULL, it contains the > current interim state of collected aggregate values which can be used to > - compute values passed over self-recursive edges and to skip values which > - clearly will not be part of intersection with INTERIM. */ > + compute values passed over self-recursive edges (if OPTIMIZE_SELF_RECURSION > + is true) and to skip values which clearly will not be part of intersection > + with INTERIM. */ > > static void > push_agg_values_from_edge (struct cgraph_edge *cs, > ipa_node_params *dest_info, > vec *res, > - const ipa_argagg_value_list *interim) > + const ipa_argagg_value_list *interim, > + bool optimize_self_recursion) > { > ipa_edge_args *args = ipa_edge_args_sum->get (cs); > if (!args) > @@ -5785,7 +5787,9 @@ push_agg_values_from_edge (struct cgraph_edge *cs, > ipcp_param_lattices *plats = ipa_get_parm_lattices (dest_info, index); > if (plats->aggs_bottom) > continue; > - push_agg_values_for_index_from_edge (cs, index, res, interim); > + push_agg_values_for_index_from_edge (cs, index, res, > + optimize_self_recursion ? interim > + : NULL); > } > } > > @@ -5804,7 +5808,7 @@ find_aggregate_values_for_callers_subset (struct cgraph_node *node, > /* gather_edges_for_value puts a non-recursive call into the first element of > callers if it can. */ > auto_vec interim; > - push_agg_values_from_edge (callers[0], dest_info, &interim, NULL); > + push_agg_values_from_edge (callers[0], dest_info, &interim, NULL, true); > > unsigned valid_entries = interim.length (); > if (!valid_entries) > @@ -5815,7 +5819,7 @@ find_aggregate_values_for_callers_subset (struct cgraph_node *node, > { > auto_vec last; > ipa_argagg_value_list avs (&interim); > - push_agg_values_from_edge (callers[i], dest_info, &last, &avs); > + push_agg_values_from_edge (callers[i], dest_info, &last, &avs, true); > > valid_entries = intersect_argaggs_with (interim, last); > if (!valid_entries) > @@ -5882,7 +5886,7 @@ cgraph_edge_brings_all_agg_vals_for_node (struct cgraph_edge *cs, > ipa_node_params *dest_info = ipa_node_params_sum->get (node); > gcc_checking_assert (dest_info->ipcp_orig_node); > dest_info = ipa_node_params_sum->get (dest_info->ipcp_orig_node); > - push_agg_values_from_edge (cs, dest_info, &edge_values, &existing); > + push_agg_values_from_edge (cs, dest_info, &edge_values, &existing, false); > const ipa_argagg_value_list avl (&edge_values); > return avl.superset_of_p (existing); > } > diff --git a/gcc/testsuite/g++.dg/ipa/pr107661.C b/gcc/testsuite/g++.dg/ipa/pr107661.C > new file mode 100644 > index 00000000000..cc6f8538dbf > --- /dev/null > +++ b/gcc/testsuite/g++.dg/ipa/pr107661.C > @@ -0,0 +1,45 @@ > +/* { dg-do run { target c++11 } } */ > +/* { dg-options "-O1 -fipa-cp -fipa-cp-clone" } */ > + > +struct R {} RGood; > +struct L {} LBad; > + > +volatile int vi; > +static void __attribute__((noipa)) L_run(void) { vi = 0; __builtin_abort (); } > +static void callback_fn_L(void) { vi = 1; L_run(); } > +static void callback_fn_R(void) { vi = 2; } > + > +struct function_ref { > + void (*callback)(void) = nullptr; > + > + function_ref(L * pl) { callback = callback_fn_L; } > + function_ref(R * pr) { callback = callback_fn_R; } > +}; > + > +// allow one level of recursion to call callback twice > +static int is_recur(void) { > + static int n = 0; > + switch (n++) { > + case 0: return 1; > + default: return 0; > + } > +} > + > +static void do3(volatile int * punused, function_ref Expired) { > + Expired.callback(); > + > + if (is_recur()) > + do3(punused, Expired); > +} > + > +static void do1(function_ref Expired) { > + volatile int unused = 42; > + > + do3(&unused, Expired); > +} > + > +int main(int, const char **) { do1(&RGood); return 0; } > + > +void seemingly_unused_foo(void) { do1(&LBad); } > + > +void (*fnptr)(void) = seemingly_unused_foo; > -- > 2.38.1 >