| Message ID | ri6v9qfnmjr.fsf@suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | IPA-CP: Remove bogus static keyword (PR 92971) | expand |
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. > 2019-12-17 Martin Jambor <mjambor@suse.cz> > > * ipa-cp.c (cgraph_edge_brings_all_agg_vals_for_node): Remove > static from local variable definition. > --- > gcc/ipa-cp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c > index 1a80ccbde2d..6692eb7b878 100644 > --- a/gcc/ipa-cp.c > +++ b/gcc/ipa-cp.c > @@ -5117,7 +5117,7 @@ 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; > + 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) > -- > 2.24.0 Jakub
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 <mjambor@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" } } */
> > 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 <mjambor@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 (); Generally it seems to me that things would be more readable/leak safe if we used auto_vecs and passed them as function arguments to be filled in. But since same constructs are used in ipa-cp/prop elsewhere the patch is OK. Honza > } > 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 >
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c index 1a80ccbde2d..6692eb7b878 100644 --- a/gcc/ipa-cp.c +++ b/gcc/ipa-cp.c @@ -5117,7 +5117,7 @@ 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; + 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)