| Message ID | 20150219105303.GZ23138@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, 19 Feb 2015, Marek Polacek wrote: > The problem exposed by this PR is (IIUC) that we hadn't gotten around to > recomputing the inline parameters in the case when optimize_inline_calls > introduces new statements. That results in ICEing later on because in > estimate_edge_growth we assert that estimated size of a statement is not > 0. > > This happens since r220359 - with this change, we started to perform early > inlining even in always_inline functions. So in the following testcase, > we have in A::A() at the start of early inlining: > > call_foo (this_2(D)); > > Since call_foo is always_inline, we inline it and apply the changes via > a call to optimize_inline_calls. That turns the above statement into: > > A::foo (this_2(D)); > > This statement is new and we don't have the inline params for it computed, > because when estimate_function_body_sizes walked the IL, the stmt wasn't > there. > > So fixed by doing what we do in early_inliner in a block below, that is, > recomputing the inline parameters. I didn't copied the if that calls > gimple_check_call_matching_types and sets edge->call_stmt_cannot_inline_p, > I'm not sure if it's needed. > > Does that make sense? > > Bootstrapped/regtested on x86_64-linux. > > 2015-02-19 Marek Polacek <polacek@redhat.com> > > PR ipa/65008 > * ipa-inline.c (early_inliner): Recompute inline parameters. > > * g++.dg/ipa/pr65008.C: New test. > > diff --git gcc/ipa-inline.c gcc/ipa-inline.c > index 025f7fc..c445f0a 100644 > --- gcc/ipa-inline.c > +++ gcc/ipa-inline.c > @@ -2559,6 +2559,19 @@ early_inliner (function *fun) > { > timevar_push (TV_INTEGRATION); > todo |= optimize_inline_calls (current_function_decl); > + /* optimize_inline_calls call above might have introduced new > + statements that don't have inline parameters computed. */ > + for (edge = node->callees; edge; edge = edge->next_callee) Are cgraph edges up-to-date here? I'd doubt that... if so, why not do this update in the inliner itself where it updates the cgraph edges? > + { > + if (inline_edge_summary_vec.length () > (unsigned) edge->uid) > + { > + struct inline_edge_summary *es = inline_edge_summary (edge); > + es->call_stmt_size > + = estimate_num_insns (edge->call_stmt, &eni_size_weights); > + es->call_stmt_time > + = estimate_num_insns (edge->call_stmt, &eni_time_weights); > + } > + } > inline_update_overall_summary (node); > inlined = false; > timevar_pop (TV_INTEGRATION); > diff --git gcc/testsuite/g++.dg/ipa/pr65008.C gcc/testsuite/g++.dg/ipa/pr65008.C > index e69de29..29b3a2f 100644 > --- gcc/testsuite/g++.dg/ipa/pr65008.C > +++ gcc/testsuite/g++.dg/ipa/pr65008.C > @@ -0,0 +1,19 @@ > +// PR ipa/65008 > +// { dg-do compile } > +// { dg-options "-O2" } > + > +struct A > +{ > + A (); > + virtual void foo () {} > +}; > + > +static inline int __attribute__ ((always_inline)) call_foo (A *a) > +{ > + a->foo (); > +} > + > +A::A () > +{ > + call_foo (this); > +} > > Marek > >
On Thu, Feb 19, 2015 at 12:06:40PM +0100, Richard Biener wrote: > On Thu, 19 Feb 2015, Marek Polacek wrote: > > > The problem exposed by this PR is (IIUC) that we hadn't gotten around to > > recomputing the inline parameters in the case when optimize_inline_calls > > introduces new statements. That results in ICEing later on because in > > estimate_edge_growth we assert that estimated size of a statement is not > > 0. > > > > This happens since r220359 - with this change, we started to perform early > > inlining even in always_inline functions. So in the following testcase, > > we have in A::A() at the start of early inlining: > > > > call_foo (this_2(D)); > > > > Since call_foo is always_inline, we inline it and apply the changes via > > a call to optimize_inline_calls. That turns the above statement into: > > > > A::foo (this_2(D)); > > > > This statement is new and we don't have the inline params for it computed, > > because when estimate_function_body_sizes walked the IL, the stmt wasn't > > there. > > > > So fixed by doing what we do in early_inliner in a block below, that is, > > recomputing the inline parameters. I didn't copied the if that calls > > gimple_check_call_matching_types and sets edge->call_stmt_cannot_inline_p, > > I'm not sure if it's needed. > > > > Does that make sense? > > > > Bootstrapped/regtested on x86_64-linux. > > > > 2015-02-19 Marek Polacek <polacek@redhat.com> > > > > PR ipa/65008 > > * ipa-inline.c (early_inliner): Recompute inline parameters. > > > > * g++.dg/ipa/pr65008.C: New test. > > > > diff --git gcc/ipa-inline.c gcc/ipa-inline.c > > index 025f7fc..c445f0a 100644 > > --- gcc/ipa-inline.c > > +++ gcc/ipa-inline.c > > @@ -2559,6 +2559,19 @@ early_inliner (function *fun) > > { > > timevar_push (TV_INTEGRATION); > > todo |= optimize_inline_calls (current_function_decl); > > + /* optimize_inline_calls call above might have introduced new > > + statements that don't have inline parameters computed. */ > > + for (edge = node->callees; edge; edge = edge->next_callee) > > Are cgraph edges up-to-date here? I'd doubt that... if so, why not > do this update in the inliner itself where it updates the cgraph edges? I've tried to move this hunk into tree-inliner.c, but I think that is not possible. E.g. inline_edge_summary is ipa-inline thing only. Also all my attemps to update the inline metrics somewhere in copy_bb or e.g. expand_call_inline failed, so I'm afraid I don't have anything better than the original patch ;). Honza, any opinion? Marek
Ping. On Thu, Feb 19, 2015 at 09:03:51PM +0100, Marek Polacek wrote: > On Thu, Feb 19, 2015 at 12:06:40PM +0100, Richard Biener wrote: > > On Thu, 19 Feb 2015, Marek Polacek wrote: > > > > > The problem exposed by this PR is (IIUC) that we hadn't gotten around to > > > recomputing the inline parameters in the case when optimize_inline_calls > > > introduces new statements. That results in ICEing later on because in > > > estimate_edge_growth we assert that estimated size of a statement is not > > > 0. > > > > > > This happens since r220359 - with this change, we started to perform early > > > inlining even in always_inline functions. So in the following testcase, > > > we have in A::A() at the start of early inlining: > > > > > > call_foo (this_2(D)); > > > > > > Since call_foo is always_inline, we inline it and apply the changes via > > > a call to optimize_inline_calls. That turns the above statement into: > > > > > > A::foo (this_2(D)); > > > > > > This statement is new and we don't have the inline params for it computed, > > > because when estimate_function_body_sizes walked the IL, the stmt wasn't > > > there. > > > > > > So fixed by doing what we do in early_inliner in a block below, that is, > > > recomputing the inline parameters. I didn't copied the if that calls > > > gimple_check_call_matching_types and sets edge->call_stmt_cannot_inline_p, > > > I'm not sure if it's needed. > > > > > > Does that make sense? > > > > > > Bootstrapped/regtested on x86_64-linux. > > > > > > 2015-02-19 Marek Polacek <polacek@redhat.com> > > > > > > PR ipa/65008 > > > * ipa-inline.c (early_inliner): Recompute inline parameters. > > > > > > * g++.dg/ipa/pr65008.C: New test. > > > > > > diff --git gcc/ipa-inline.c gcc/ipa-inline.c > > > index 025f7fc..c445f0a 100644 > > > --- gcc/ipa-inline.c > > > +++ gcc/ipa-inline.c > > > @@ -2559,6 +2559,19 @@ early_inliner (function *fun) > > > { > > > timevar_push (TV_INTEGRATION); > > > todo |= optimize_inline_calls (current_function_decl); > > > + /* optimize_inline_calls call above might have introduced new > > > + statements that don't have inline parameters computed. */ > > > + for (edge = node->callees; edge; edge = edge->next_callee) > > > > Are cgraph edges up-to-date here? I'd doubt that... if so, why not > > do this update in the inliner itself where it updates the cgraph edges? > > I've tried to move this hunk into tree-inliner.c, but I think that is > not possible. E.g. inline_edge_summary is ipa-inline thing only. > Also all my attemps to update the inline metrics somewhere in copy_bb or > e.g. expand_call_inline failed, so I'm afraid I don't have anything > better than the original patch ;). Honza, any opinion? > > Marek Marek
> > > > 2015-02-19 Marek Polacek <polacek@redhat.com> > > > > > > > > PR ipa/65008 > > > > * ipa-inline.c (early_inliner): Recompute inline parameters. > > > > > > > > * g++.dg/ipa/pr65008.C: New test. > > > > > > > > diff --git gcc/ipa-inline.c gcc/ipa-inline.c > > > > index 025f7fc..c445f0a 100644 > > > > --- gcc/ipa-inline.c > > > > +++ gcc/ipa-inline.c > > > > @@ -2559,6 +2559,19 @@ early_inliner (function *fun) > > > > { > > > > timevar_push (TV_INTEGRATION); > > > > todo |= optimize_inline_calls (current_function_decl); > > > > + /* optimize_inline_calls call above might have introduced new > > > > + statements that don't have inline parameters computed. */ > > > > + for (edge = node->callees; edge; edge = edge->next_callee) > > > > > > Are cgraph edges up-to-date here? I'd doubt that... if so, why not > > > do this update in the inliner itself where it updates the cgraph edges? > > > > I've tried to move this hunk into tree-inliner.c, but I think that is > > not possible. E.g. inline_edge_summary is ipa-inline thing only. > > Also all my attemps to update the inline metrics somewhere in copy_bb or > > e.g. expand_call_inline failed, so I'm afraid I don't have anything > > better than the original patch ;). Honza, any opinion? Sorry for taking time on this - I wanted to give it a quick test on Firefox that does use always inline and see how code quality compares to re-running the analysis after inlining (that is more correct thing to do). It seems tha tthere is not much of practical difference, because we do very minimal folding anyway. So patch is OK Honza > > > > Marek > > Marek
> > > Are cgraph edges up-to-date here? I'd doubt that... if so, why not > > > do this update in the inliner itself where it updates the cgraph edges? And for Richard's comment - edges are kept up-to-date by the inliner - that is also necessary for clonning and other stuff. Parameters are not kept up to doate though, because only early inliner needs to preserve them for this iteration. I think it is easier to deal with this in early inliner than adding yet another special case to tree-inline. Honza > > > > I've tried to move this hunk into tree-inliner.c, but I think that is > > not possible. E.g. inline_edge_summary is ipa-inline thing only. > > Also all my attemps to update the inline metrics somewhere in copy_bb or > > e.g. expand_call_inline failed, so I'm afraid I don't have anything > > better than the original patch ;). Honza, any opinion? > > > > Marek > > Marek
diff --git gcc/ipa-inline.c gcc/ipa-inline.c index 025f7fc..c445f0a 100644 --- gcc/ipa-inline.c +++ gcc/ipa-inline.c @@ -2559,6 +2559,19 @@ early_inliner (function *fun) { timevar_push (TV_INTEGRATION); todo |= optimize_inline_calls (current_function_decl); + /* optimize_inline_calls call above might have introduced new + statements that don't have inline parameters computed. */ + for (edge = node->callees; edge; edge = edge->next_callee) + { + if (inline_edge_summary_vec.length () > (unsigned) edge->uid) + { + struct inline_edge_summary *es = inline_edge_summary (edge); + es->call_stmt_size + = estimate_num_insns (edge->call_stmt, &eni_size_weights); + es->call_stmt_time + = estimate_num_insns (edge->call_stmt, &eni_time_weights); + } + } inline_update_overall_summary (node); inlined = false; timevar_pop (TV_INTEGRATION); diff --git gcc/testsuite/g++.dg/ipa/pr65008.C gcc/testsuite/g++.dg/ipa/pr65008.C index e69de29..29b3a2f 100644 --- gcc/testsuite/g++.dg/ipa/pr65008.C +++ gcc/testsuite/g++.dg/ipa/pr65008.C @@ -0,0 +1,19 @@ +// PR ipa/65008 +// { dg-do compile } +// { dg-options "-O2" } + +struct A +{ + A (); + virtual void foo () {} +}; + +static inline int __attribute__ ((always_inline)) call_foo (A *a) +{ + a->foo (); +} + +A::A () +{ + call_foo (this); +}