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 <mjam...@suse.cz>
>
> 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 <mjam...@suse.cz>
>
> 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 <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
