From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out1.suse.de (smtp-out1.suse.de [IPv6:2001:67c:2178:6::1c]) by sourceware.org (Postfix) with ESMTPS id 57EA23858404 for ; Tue, 15 Aug 2023 09:34:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 57EA23858404 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out1.suse.de (Postfix) with ESMTP id 8867721910; Tue, 15 Aug 2023 09:34:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1692092079; 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=uBriNwnkXDAxYpnrSrhsFz4apJbHHWt+rt7jKLtyr3A=; b=zxIcjwmYCeJAw5Wa3esuExDQ5KzuNLmKLvBQSI6jz8wCFRiJ1qT6FS6eR6/fO6nytW/t1D 8L6Rxgs8fER2HfGQZ+Hg7+5tq7VCXzOrO5g/QA54XnuWlNiPZ0Y4wOyZYenz0yFrHgEs8O JWhsa4H5Jt69PClE+HLnOuR8KSKo0YQ= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1692092079; 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=uBriNwnkXDAxYpnrSrhsFz4apJbHHWt+rt7jKLtyr3A=; b=zrKdIWSSI8i4bLnBuu3D5ecFOTV+FjswW9RDAYsOS1/voKoh8prYu0Dhx3hqNmUMys5m50 wO6GEnYbJe7gFSBA== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (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 7C4EE2C143; Tue, 15 Aug 2023 09:34:39 +0000 (UTC) Date: Tue, 15 Aug 2023 09:34:39 +0000 (UTC) From: Richard Biener To: Martin Jambor cc: GCC Patches , Jan Hubicka Subject: Re: [PATCH 2/2] ipa-cp: Feed results of IPA-CP into value numbering In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-11.0 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.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Sun, 13 Aug 2023, Martin Jambor wrote: > Hello Richi, > > it took me quite time to get back to this but it might have actually > helped because it forced me to re-read the code around and in turn > simplify the patch. > > On Mon, Jun 12 2023, Richard Biener wrote: > > On Fri, 9 Jun 2023, Martin Jambor wrote: > > > > [...] > > >> @@ -2327,7 +2330,7 @@ vn_walk_cb_data::push_partial_def (pd_data pd, > >> with the current VUSE and performs the expression lookup. */ > >> > >> static void * > >> -vn_reference_lookup_2 (ao_ref *op ATTRIBUTE_UNUSED, tree vuse, void *data_) > >> +vn_reference_lookup_2 (ao_ref *op, tree vuse, void *data_) > >> { > >> vn_walk_cb_data *data = (vn_walk_cb_data *)data_; > >> vn_reference_t vr = data->vr; > >> @@ -2361,6 +2364,38 @@ vn_reference_lookup_2 (ao_ref *op ATTRIBUTE_UNUSED, tree vuse, void *data_) > >> return *slot; > >> } > >> > >> + if (SSA_NAME_IS_DEFAULT_DEF (vuse) > > && data->partial_defs.is_empty ()) > > > > ^^ do this check early > > The check is actually done right at the beginning of the function > already so I simply removed it. > > > > >> + { > >> + HOST_WIDE_INT offset, size; > >> + tree v = NULL_TREE; > > tree base = ao_ref_base (op); > > if ((TREE_CODE (base) == PARM_DECL > > || TREE_CODE (base) == MEM_REF) > > > > handle both kind of bases with ... > > > >> + && op->offset.is_constant (&offset) > >> + && op->size.is_constant (&size) > >> + && op->max_size_known_p () > >> + && known_eq (op->size, op->max_size)) > > > > ^^^ this preconditions (would have been missing in the MEM_REF branch > > before) > > I missed that call to ao_ref_base fills in these fields - and in the > pointer case that they are not filled in without it. I hope the patch > below is the simplified version you wanted. Yes, looks good now. > The patch passed bootstrap and testing and also LTO bootstrap on > x86_64-linux. OK. Thanks, Richard. > Thanks, > > Martin > > > > PRs 68930 and 92497 show that when IPA-CP figures out constants in > aggregate parameters or when passed by reference but the loads happen > in an inlined function the information is lost. This happens even > when the inlined function itself was known to have - or even cloned to > have - such constants in incoming parameters because the transform > phase of IPA passes is not run on them. See discussion in the bugs > for reasons why. > > Honza suggested that we can plug the results of IPA-CP analysis into > value numbering, so that FRE can figure out that some loads fetch > known constants. This is what this patch attempts to do. The patch > does not attempt to populate partial_defs with information from > IPA-CP, this can be hopefully added as a follow-up. > > gcc/ChangeLog: > > 2023-08-11 Martin Jambor > > PR ipa/68930 > PR ipa/92497 > * ipa-prop.h (ipcp_get_aggregate_const): Declare. > * ipa-prop.cc (ipcp_get_aggregate_const): New function. > (ipcp_transform_function): Do not deallocate transformation info. > * tree-ssa-sccvn.cc: Include alloc-pool.h, symbol-summary.h and > ipa-prop.h. > (vn_reference_lookup_2): When hitting default-def vuse, query > IPA-CP transformation info for any known constants. > > gcc/testsuite/ChangeLog: > > 2023-06-07 Martin Jambor > > PR ipa/68930 > PR ipa/92497 > * gcc.dg/ipa/pr92497-1.c: New test. > * gcc.dg/ipa/pr92497-2.c: Likewise. > --- > gcc/ipa-prop.cc | 33 +++++++++++++++++++++++---- > gcc/ipa-prop.h | 3 +++ > gcc/testsuite/gcc.dg/ipa/pr92497-1.c | 26 +++++++++++++++++++++ > gcc/testsuite/gcc.dg/ipa/pr92497-2.c | 26 +++++++++++++++++++++ > gcc/tree-ssa-sccvn.cc | 34 +++++++++++++++++++++++++++- > 5 files changed, 116 insertions(+), 6 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/ipa/pr92497-1.c > create mode 100644 gcc/testsuite/gcc.dg/ipa/pr92497-2.c > > diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc > index 4f6ed7b89bd..9efaa5cb848 100644 > --- a/gcc/ipa-prop.cc > +++ b/gcc/ipa-prop.cc > @@ -5760,6 +5760,34 @@ ipcp_modif_dom_walker::before_dom_children (basic_block bb) > return NULL; > } > > +/* If IPA-CP discovered a constant in parameter PARM at OFFSET of a given SIZE > + - whether passed by reference or not is given by BY_REF - return that > + constant. Otherwise return NULL_TREE. */ > + > +tree > +ipcp_get_aggregate_const (struct function *func, tree parm, bool by_ref, > + HOST_WIDE_INT bit_offset, HOST_WIDE_INT bit_size) > +{ > + cgraph_node *node = cgraph_node::get (func->decl); > + ipcp_transformation *ts = ipcp_get_transformation_summary (node); > + > + if (!ts || !ts->m_agg_values) > + return NULL_TREE; > + > + int index = ts->get_param_index (func->decl, parm); > + if (index < 0) > + return NULL_TREE; > + > + ipa_argagg_value_list avl (ts); > + unsigned unit_offset = bit_offset / BITS_PER_UNIT; > + tree v = avl.get_value (index, unit_offset, by_ref); > + if (!v > + || maybe_ne (tree_to_poly_int64 (TYPE_SIZE (TREE_TYPE (v))), bit_size)) > + return NULL_TREE; > + > + return v; > +} > + > /* Return true if we have recorded VALUE and MASK about PARM. > Set VALUE and MASk accordingly. */ > > @@ -6031,11 +6059,6 @@ ipcp_transform_function (struct cgraph_node *node) > free_ipa_bb_info (bi); > fbi.bb_infos.release (); > > - ipcp_transformation *s = ipcp_transformation_sum->get (node); > - s->m_agg_values = NULL; > - s->bits = NULL; > - s->m_vr = NULL; > - > vec_free (descriptors); > if (cfg_changed) > delete_unreachable_blocks_update_callgraph (node, false); > diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h > index 410c951a256..7e033d2a7b8 100644 > --- a/gcc/ipa-prop.h > +++ b/gcc/ipa-prop.h > @@ -1235,6 +1235,9 @@ void ipa_dump_param (FILE *, class ipa_node_params *info, int i); > void ipa_release_body_info (struct ipa_func_body_info *); > tree ipa_get_callee_param_type (struct cgraph_edge *e, int i); > bool ipcp_get_parm_bits (tree, tree *, widest_int *); > +tree ipcp_get_aggregate_const (struct function *func, tree parm, bool by_ref, > + HOST_WIDE_INT bit_offset, > + HOST_WIDE_INT bit_size); > bool unadjusted_ptr_and_unit_offset (tree op, tree *ret, > poly_int64 *offset_ret); > > diff --git a/gcc/testsuite/gcc.dg/ipa/pr92497-1.c b/gcc/testsuite/gcc.dg/ipa/pr92497-1.c > new file mode 100644 > index 00000000000..dcece15963c > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/ipa/pr92497-1.c > @@ -0,0 +1,26 @@ > +/* { dg-do run } */ > +/* { dg-options "-O2 -fno-early-inlining" } */ > + > +struct a {int a;}; > +static int > +foo (struct a a) > +{ > + if (!__builtin_constant_p (a.a)) > + __builtin_abort (); > + return a.a; > +} > + > +static int __attribute__ ((noinline)) > +bar (struct a a) > +{ > + return foo(a); > +} > + > +volatile int r; > + > +int main() > +{ > + struct a a={1}; > + r = bar (a); > + return 0; > +} > diff --git a/gcc/testsuite/gcc.dg/ipa/pr92497-2.c b/gcc/testsuite/gcc.dg/ipa/pr92497-2.c > new file mode 100644 > index 00000000000..c64090d1a7a > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/ipa/pr92497-2.c > @@ -0,0 +1,26 @@ > +/* { dg-do run } */ > +/* { dg-options "-O2 -fno-early-inlining -fno-ipa-sra" } */ > + > +struct a {int a;}; > +static int > +foo (struct a *a) > +{ > + if (!__builtin_constant_p (a->a)) > + __builtin_abort (); > + return a->a; > +} > + > +static int __attribute__ ((noinline)) > +bar (struct a *a) > +{ > + return foo(a); > +} > + > +volatile int r; > + > +int main() > +{ > + struct a a={1}; > + r = bar (&a); > + return 0; > +} > diff --git a/gcc/tree-ssa-sccvn.cc b/gcc/tree-ssa-sccvn.cc > index 32e06fae3b9..b1678911671 100644 > --- a/gcc/tree-ssa-sccvn.cc > +++ b/gcc/tree-ssa-sccvn.cc > @@ -74,6 +74,9 @@ along with GCC; see the file COPYING3. If not see > #include "ipa-modref-tree.h" > #include "ipa-modref.h" > #include "tree-ssa-sccvn.h" > +#include "alloc-pool.h" > +#include "symbol-summary.h" > +#include "ipa-prop.h" > > /* This algorithm is based on the SCC algorithm presented by Keith > Cooper and L. Taylor Simpson in "SCC-Based Value numbering" > @@ -2327,7 +2330,7 @@ vn_walk_cb_data::push_partial_def (pd_data pd, > with the current VUSE and performs the expression lookup. */ > > static void * > -vn_reference_lookup_2 (ao_ref *op ATTRIBUTE_UNUSED, tree vuse, void *data_) > +vn_reference_lookup_2 (ao_ref *op, tree vuse, void *data_) > { > vn_walk_cb_data *data = (vn_walk_cb_data *)data_; > vn_reference_t vr = data->vr; > @@ -2361,6 +2364,35 @@ vn_reference_lookup_2 (ao_ref *op ATTRIBUTE_UNUSED, tree vuse, void *data_) > return *slot; > } > > + if (SSA_NAME_IS_DEFAULT_DEF (vuse)) > + { > + HOST_WIDE_INT op_offset, op_size; > + tree v = NULL_TREE; > + tree base = ao_ref_base (op); > + > + if (base > + && op->offset.is_constant (&op_offset) > + && op->size.is_constant (&op_size) > + && op->max_size_known_p () > + && known_eq (op->size, op->max_size)) > + { > + if (TREE_CODE (base) == PARM_DECL) > + v = ipcp_get_aggregate_const (cfun, base, false, op_offset, > + op_size); > + else if (TREE_CODE (base) == MEM_REF > + && integer_zerop (TREE_OPERAND (base, 1)) > + && TREE_CODE (TREE_OPERAND (base, 0)) == SSA_NAME > + && SSA_NAME_IS_DEFAULT_DEF (TREE_OPERAND (base, 0)) > + && (TREE_CODE (SSA_NAME_VAR (TREE_OPERAND (base, 0))) > + == PARM_DECL)) > + v = ipcp_get_aggregate_const (cfun, > + SSA_NAME_VAR (TREE_OPERAND (base, 0)), > + true, op_offset, op_size); > + } > + if (v) > + return data->finish (vr->set, vr->base_set, v); > + } > + > return NULL; > } > > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)