Hi, On Tue, Dec 17 2019, Jakub Jelinek wrote: > On Tue, Dec 17, 2019 at 01:50:32PM +0100, Martin Jambor wrote: >> Hi, >> >> as reported in PR 92971, IPA-CP's >> cgraph_edge_brings_all_agg_vals_for_node defines one local variable with >> the static keyword which is a clear mistake, probabley a cut'n'paste >> error when I originally wrote the code. >> >> I'll commit the following as obvious after a round of bootstrap and >> testing. Early next year, I'll also commit it to all opened release >> branches. > > Is that what you want to do though? > Because when it is an automatic variable (shouldn't it be auto_vec, btw), > then the first use of it doesn't make much sense: > values = intersect_aggregates_with_edge (cs, i, values); > because it will be always (cs, i, vNULL). So maybe the var should live > across the iterations or live in the caller that should pass a pointer (or > reference) to it? > With the patch, there will be leaks too, because the values vector is only > released if the function returns false and is not released otherwise.
the leak is indeed a problem, thanks for spotting it. But apart from that, I really wanted to pass vNULL to intersect_aggregates_with_edge, and the patch below does it explicitely to make it clear, because while the function can do also intersecting its actual task here is to carry over aggregate constants from all types of callers (clones and non-clones) and all kinds of supported jump functions. That might be an overkill because the main goal of cgraph_edge_brings_all_agg_vals_for_node is to add the last edge within SCCs of nodes propagating the same constant to each other and I could not quickly come up with a testcase where the caller would be a non-clone (and the function something other than pass-through, but I suspect that could actually happen) but since the code is written we might as well use it. The function is written to bail out early before actual value comparing and that is why the code is rarely executed, in fact I found out that it is not covered by our testsuite (see https://users.suse.com/~mliska/lcov/gcc/ipa-cp.c.gcov.html) and so the patch also adds a testcase which does execute it. The way vectors are passed around by value rather than by reference is how I wrote this stuff shortly after conversion from our C VEC_ headers with which were used in the same way. I agree that a lot of code in ipa-cp would benefit from transitioning to auto_vecs but that is something for the next stage 1. The patch has been bootstrapped and LTO-profile-bootstrapped on x86-64-linux. OK for trunk? Thanks, Martin 2019-12-17 Martin Jambor <mjam...@suse.cz> PR ipa/92971 * Ipa-cp.c (cgraph_edge_brings_all_agg_vals_for_node): Fix definition of values, release memory on exit. testsuite/ * gcc.dg/ipa/ipcp-agg-12.c: New test. --- gcc/ipa-cp.c | 4 +- gcc/testsuite/gcc.dg/ipa/ipcp-agg-12.c | 53 ++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/ipa/ipcp-agg-12.c diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c index 1a80ccbde2d..243b064ee2c 100644 --- a/gcc/ipa-cp.c +++ b/gcc/ipa-cp.c @@ -5117,7 +5117,6 @@ cgraph_edge_brings_all_agg_vals_for_node (struct cgraph_edge *cs, for (i = 0; i < count; i++) { - static vec<ipa_agg_value> values = vNULL; class ipcp_param_lattices *plats; bool interesting = false; for (struct ipa_agg_replacement_value *av = aggval; av; av = av->next) @@ -5133,7 +5132,7 @@ cgraph_edge_brings_all_agg_vals_for_node (struct cgraph_edge *cs, if (plats->aggs_bottom) return false; - values = intersect_aggregates_with_edge (cs, i, values); + vec<ipa_agg_value> values = intersect_aggregates_with_edge (cs, i, vNULL); if (!values.exists ()) return false; @@ -5157,6 +5156,7 @@ cgraph_edge_brings_all_agg_vals_for_node (struct cgraph_edge *cs, return false; } } + values.release (); } return true; } diff --git a/gcc/testsuite/gcc.dg/ipa/ipcp-agg-12.c b/gcc/testsuite/gcc.dg/ipa/ipcp-agg-12.c new file mode 100644 index 00000000000..5c57913803e --- /dev/null +++ b/gcc/testsuite/gcc.dg/ipa/ipcp-agg-12.c @@ -0,0 +1,53 @@ +/* { dg-do compile } */ +/* { dg-options "-O3 -fno-ipa-sra -fdump-ipa-cp-details --param=ipa-cp-eval-threshold=2" } */ + +struct S +{ + int a, b, c; +}; + +int __attribute__((noinline)) foo (int i, struct S s); +int __attribute__((noinline)) bar (int i, struct S s); +int __attribute__((noinline)) baz (int i, struct S s); + + +int __attribute__((noinline)) +bar (int i, struct S s) +{ + return baz (i, s); +} + +int __attribute__((noinline)) +baz (int i, struct S s) +{ + return foo (i, s); +} + +int __attribute__((noinline)) +foo (int i, struct S s) +{ + if (i == 2) + return 0; + else + return s.b * s.b + bar (i - 1, s); +} + +volatile int g; + +void entry (void) +{ + struct S s; + s.b = 4; + g = bar (g, s); +} + + +void entry2 (void) +{ + struct S s; + s.b = 6; + g = baz (g, s); +} + + +/* { dg-final { scan-ipa-dump-times "adding an extra caller" 2 "cp" } } */ -- 2.24.0
