Message ID | 20140326021653.GA14614@kam.mff.cuni.cz |
---|---|
State | New |
Headers | show |
On 26 March 2014 03:17:11 Jan Hubicka <hubicka@ucw.cz> wrote: > Hi, Just 2 nits, cannot comment on the patch itself. s/clonning/cloning/g as usual :) And the content of the testcase is duplicated. Thanks, > this patch fixes compile time issue in the testcase that is caused by fact > that the inliner > is repeatedly inlining into an call it earlier proved to be unreachable. > The analysis part > knows that such calls are not accounted into overall function summaries, > but the transform > part sees them merely as cold calls and thus it attempts to do inlining for > size. > This patch makes analysis part to practively redirect all known to be > unreachable > calls to BUILTIN_UNREAHABLE. > > Doing so uncovered the bug in ipa-pure-const I fixed earlier and also another > but in set_cond_stmt_execution_predicate where invert_tree_comparison is > called and the condition is used further. The function returns ERROR_MARK > for FP > comparsions in some cases (though I think it should not since there seems to > be no toher way to invert comparsion without this and we have the unordered > codes). > This concide with ipa-inline-analysis.c internal use of ERROR_MARK and leads to > inconsistent predicates. > > Bootstrapped/regtested x86_64-linux, comitted. > > PR ipa/60315 > * cif-code.def (UNREACHABLE) New code. > * ipa-inline.c (inline_small_functions): Skip edges to __builtlin_unreachable. > (estimate_edge_growth): Allow edges to __builtlin_unreachable. > * ipa-inline-analysis.c (edge_set_predicate): Redirect edges with false > predicate to __bulitin_unreachable. > (set_cond_stmt_execution_predicate): Fix issue when invert_tree_comparison > returns ERROR_MARK. > * ipa-pure-const.c (propagate_pure_const, propagate_nothrow): Do not > propagate to inline clones. > * cgraph.c (verify_edge_corresponds_to_fndecl): Allow redirection > to unreachable. > * ipa-cp.c (create_specialized_node): Be ready for new node to appear. > * cgraphclones.c (cgraph_clone_node): If call destination is already > ureachable, do not redirect it back. > * tree-inline.c (fold_marked_statements): Hanlde calls becoming > unreachable. > > * testsuite/g++.dg/torture/pr60315.C: New testcase. > Index: cif-code.def > =================================================================== > --- cif-code.def (revision 208829) > +++ cif-code.def (working copy) > @@ -127,3 +127,7 @@ DEFCIFCODE(USES_COMDAT_LOCAL, CIF_FINAL_ > /* We can't inline because of mismatched caller/callee attributes. */ > DEFCIFCODE(ATTRIBUTE_MISMATCH, CIF_FINAL_NORMAL, > N_("function attribute mismatch")) > + > +/* We proved that the call is unreachable. */ > +DEFCIFCODE(UNREACHABLE, CIF_FINAL_NORMAL, > + N_("unreachable")) > Index: cgraphclones.c > =================================================================== > --- cgraphclones.c (revision 208829) > +++ cgraphclones.c (working copy) > @@ -238,8 +238,12 @@ cgraph_clone_node (struct cgraph_node *n > FOR_EACH_VEC_ELT (redirect_callers, i, e) > { > /* Redirect calls to the old version node to point to its new > - version. */ > - cgraph_redirect_edge_callee (e, new_node); > + version. The only exception is when the edge was proved to > + be unreachable during the clonning procedure. */ > + if (!e->callee > + || DECL_BUILT_IN_CLASS (e->callee->decl) != BUILT_IN_NORMAL > + || DECL_FUNCTION_CODE (e->callee->decl) != BUILT_IN_UNREACHABLE) > + cgraph_redirect_edge_callee (e, new_node); > } > > > Index: ipa-inline.c > =================================================================== > --- ipa-inline.c (revision 208829) > +++ ipa-inline.c (working copy) > @@ -1685,7 +1685,7 @@ inline_small_functions (void) > edge = (struct cgraph_edge *) fibheap_extract_min (edge_heap); > gcc_assert (edge->aux); > edge->aux = NULL; > - if (!edge->inline_failed) > + if (!edge->inline_failed || !edge->callee->analyzed) > continue; > > /* Be sure that caches are maintained consistent. > Index: ipa-inline.h > =================================================================== > --- ipa-inline.h (revision 208829) > +++ ipa-inline.h (working copy) > @@ -285,7 +285,8 @@ static inline int > estimate_edge_growth (struct cgraph_edge *edge) > { > #ifdef ENABLE_CHECKING > - gcc_checking_assert (inline_edge_summary (edge)->call_stmt_size); > + gcc_checking_assert (inline_edge_summary (edge)->call_stmt_size > + || !edge->callee->analyzed); > #endif > return (estimate_edge_size (edge) > - inline_edge_summary (edge)->call_stmt_size); > Index: testsuite/g++.dg/torture/pr60315.C > =================================================================== > --- testsuite/g++.dg/torture/pr60315.C (revision 0) > +++ testsuite/g++.dg/torture/pr60315.C (revision 0) > @@ -0,0 +1,32 @@ > +// { dg-do compile } > +struct Base { > + virtual int f() = 0; > +}; > + > +struct Derived : public Base { > + virtual int f() final override { > + return 42; > + } > +}; > + > +extern Base* b; > + > +int main() { > + return (static_cast<Derived*>(b)->*(&Derived::f))(); > +} > +// { dg-do compile } > +struct Base { > + virtual int f() = 0; > +}; > + > +struct Derived : public Base { > + virtual int f() final override { > + return 42; > + } > +}; > + > +extern Base* b; > + > +int main() { > + return (static_cast<Derived*>(b)->*(&Derived::f))(); > +} > Index: cgraph.c > =================================================================== > --- cgraph.c (revision 208829) > +++ cgraph.c (working copy) > @@ -2612,6 +2612,12 @@ verify_edge_corresponds_to_fndecl (struc > || node->in_other_partition > || e->callee->in_other_partition) > return false; > + > + /* Optimizers can redirect unreachable calls or calls triggering undefined > + behaviour to builtin_unreachable. */ > + if (DECL_BUILT_IN_CLASS (e->callee->decl) == BUILT_IN_NORMAL > + && DECL_FUNCTION_CODE (e->callee->decl) == BUILT_IN_UNREACHABLE) > + return false; > node = cgraph_function_or_thunk_node (node, NULL); > > if (e->callee->former_clone_of != node->decl > Index: ipa-inline-analysis.c > =================================================================== > --- ipa-inline-analysis.c (revision 208829) > +++ ipa-inline-analysis.c (working copy) > @@ -746,6 +746,20 @@ static void > edge_set_predicate (struct cgraph_edge *e, struct predicate *predicate) > { > struct inline_edge_summary *es = inline_edge_summary (e); > + > + /* If the edge is determined to be never executed, redirect it > + to BUILTIN_UNREACHABLE to save inliner from inlining into it. */ > + if (predicate && false_predicate_p (predicate) && e->callee) > + { > + struct cgraph_node *callee = !e->inline_failed ? e->callee : NULL; > + > + cgraph_redirect_edge_callee (e, > + cgraph_get_create_node > + (builtin_decl_implicit (BUILT_IN_UNREACHABLE))); > + e->inline_failed = CIF_UNREACHABLE; > + if (callee) > + cgraph_remove_node_and_inline_clones (callee, NULL); > + } > if (predicate && !true_predicate_p (predicate)) > { > if (!es->predicate) > @@ -1724,12 +1738,20 @@ set_cond_stmt_execution_predicate (struc > > FOR_EACH_EDGE (e, ei, bb->succs) > { > - struct predicate p = add_condition (summary, index, &aggpos, > - e->flags & EDGE_TRUE_VALUE > - ? code : inverted_code, > - gimple_cond_rhs (last)); > - e->aux = pool_alloc (edge_predicate_pool); > - *(struct predicate *) e->aux = p; > + enum tree_code this_code = (e->flags & EDGE_TRUE_VALUE > + ? code : inverted_code); > + /* invert_tree_comparison will return ERROR_MARK on FP > + comparsions that are not EQ/NE instead of returning proper > + unordered one. Be sure it is not confused with NON_CONSTANT. */ > + if (this_code != ERROR_MARK) > + { > + struct predicate p = add_condition (summary, index, &aggpos, > + e->flags & EDGE_TRUE_VALUE > + ? code : inverted_code, > + gimple_cond_rhs (last)); > + e->aux = pool_alloc (edge_predicate_pool); > + *(struct predicate *) e->aux = p; > + } > } > } > > Index: ipa-cp.c > =================================================================== > --- ipa-cp.c (revision 208829) > +++ ipa-cp.c (working copy) > @@ -2811,9 +2811,7 @@ create_specialized_node (struct cgraph_n > if (aggvals) > ipa_dump_agg_replacement_values (dump_file, aggvals); > } > - gcc_checking_assert (ipa_node_params_vector.exists () > - && (ipa_node_params_vector.length () > - > (unsigned) cgraph_max_uid)); > + ipa_check_create_node_params (); > update_profiling_info (node, new_node); > new_info = IPA_NODE_REF (new_node); > new_info->ipcp_orig_node = node; Sent with AquaMail for Android http://www.aqua-mail.com
> Bootstrapped/regtested x86_64-linux, comitted.
Not with Ada apparently, resulting in
=== acats tests ===
FAIL: c34007d
FAIL: c34007g
FAIL: c34007s
FAIL: c37213j
FAIL: c37213k
FAIL: c37213l
FAIL: ce2201g
FAIL: cxa5a03
FAIL: cxa5a04
FAIL: cxa5a06
FAIL: cxg2013
FAIL: cxg2015
=== acats Summary ===
# of expected passes 2308
# of unexpected failures 12
Reduced testcase attached, compile p.adb at -O2.
> > Bootstrapped/regtested x86_64-linux, comitted. > > Not with Ada apparently, resulting in > > === acats tests === > FAIL: c34007d > FAIL: c34007g > FAIL: c34007s > FAIL: c37213j > FAIL: c37213k > FAIL: c37213l > FAIL: ce2201g > FAIL: cxa5a03 > FAIL: cxa5a04 > FAIL: cxa5a06 > FAIL: cxg2013 > FAIL: cxg2015 > > === acats Summary === > # of expected passes 2308 > # of unexpected failures 12 > > Reduced testcase attached, compile p.adb at -O2. I will check, thanks for the reduced testcase. It seems like another case where we get predicate wrong that ought to be fixed, of course. Honza
Jan Hubicka <hubicka@ucw.cz> writes: > Index: testsuite/g++.dg/torture/pr60315.C > =================================================================== > --- testsuite/g++.dg/torture/pr60315.C (revision 0) > +++ testsuite/g++.dg/torture/pr60315.C (revision 0) > @@ -0,0 +1,32 @@ > +// { dg-do compile } > +struct Base { > + virtual int f() = 0; > +}; > + > +struct Derived : public Base { > + virtual int f() final override { > + return 42; > + } > +}; > + > +extern Base* b; > + > +int main() { > + return (static_cast<Derived*>(b)->*(&Derived::f))(); > +} FAIL: g++.dg/torture/pr60315.C -O0 (test for excess errors) Excess errors: /usr/local/gcc/gcc-20140327/gcc/testsuite/g++.dg/torture/pr60315.C:7:19: warning: override controls (override/final) only available with -std=c++11 or -std=gnu++11 /usr/local/gcc/gcc-20140327/gcc/testsuite/g++.dg/torture/pr60315.C:7:21: warning: override controls (override/final) only available with -std=c++11 or -std=gnu++11 Andreas.
> I will check, thanks for the reduced testcase. It seems like another case > where we get predicate wrong that ought to be fixed, of course. You're welcome. Another bug introduced by the patch: eric@polaris:~/build/gcc/native> gcc/xgcc -Bgcc -S opt33.adb -O /home/eric/install/gcc/lib64/gcc/x86_64-suse-linux/4.9.0/adainclude/a- coorse.adb: In function 'Opt33.My_Ordered_Sets.To_Set': /home/eric/install/gcc/lib64/gcc/x86_64-suse-linux/4.9.0/adainclude/a- coorse.adb:1983:4: error: static chain with function that doesn't use one .builtin_unreachable (&tree, node_54, 1, 0B); [static-chain: &FRAME.437] /home/eric/install/gcc/lib64/gcc/x86_64-suse-linux/4.9.0/adainclude/a- coorse.adb:1983:4: error: static chain with function that doesn't use one .builtin_unreachable (&tree, node_54, _60, node_57); [static-chain: &FRAME.437] +===========================GNAT BUG DETECTED==============================+ | 4.9.0 20140327 (experimental) [trunk revision 208879] (x86_64-suse-linux) GCC error:| | verify_gimple failed | | Error detected around /home/eric/install/gcc/lib64/gcc/x86_64-suse- linux/4.9.0/adainclude/a-coorse.adb:1983:4| You can install the testcase as gnat.dg/opt33.adb in the testsuite.
> > I will check, thanks for the reduced testcase. It seems like another case > > where we get predicate wrong that ought to be fixed, of course. > > You're welcome. Another bug introduced by the patch: > > eric@polaris:~/build/gcc/native> gcc/xgcc -Bgcc -S opt33.adb -O > /home/eric/install/gcc/lib64/gcc/x86_64-suse-linux/4.9.0/adainclude/a- > coorse.adb: In function 'Opt33.My_Ordered_Sets.To_Set': > /home/eric/install/gcc/lib64/gcc/x86_64-suse-linux/4.9.0/adainclude/a- > coorse.adb:1983:4: error: static chain with function that doesn't use one > .builtin_unreachable (&tree, node_54, 1, 0B); [static-chain: &FRAME.437] > /home/eric/install/gcc/lib64/gcc/x86_64-suse-linux/4.9.0/adainclude/a- > coorse.adb:1983:4: error: static chain with function that doesn't use one > .builtin_unreachable (&tree, node_54, _60, node_57); [static-chain: > &FRAME.437] > +===========================GNAT BUG DETECTED==============================+ > | 4.9.0 20140327 (experimental) [trunk revision 208879] (x86_64-suse-linux) > GCC error:| > | verify_gimple failed | > | Error detected around /home/eric/install/gcc/lib64/gcc/x86_64-suse- > linux/4.9.0/adainclude/a-coorse.adb:1983:4| Thanks, looks like another (semi)-latent bug. I will add code to cgraph redirection to get rid of static chain if it is uneeded. Honza
Index: cif-code.def =================================================================== --- cif-code.def (revision 208829) +++ cif-code.def (working copy) @@ -127,3 +127,7 @@ DEFCIFCODE(USES_COMDAT_LOCAL, CIF_FINAL_ /* We can't inline because of mismatched caller/callee attributes. */ DEFCIFCODE(ATTRIBUTE_MISMATCH, CIF_FINAL_NORMAL, N_("function attribute mismatch")) + +/* We proved that the call is unreachable. */ +DEFCIFCODE(UNREACHABLE, CIF_FINAL_NORMAL, + N_("unreachable")) Index: cgraphclones.c =================================================================== --- cgraphclones.c (revision 208829) +++ cgraphclones.c (working copy) @@ -238,8 +238,12 @@ cgraph_clone_node (struct cgraph_node *n FOR_EACH_VEC_ELT (redirect_callers, i, e) { /* Redirect calls to the old version node to point to its new - version. */ - cgraph_redirect_edge_callee (e, new_node); + version. The only exception is when the edge was proved to + be unreachable during the clonning procedure. */ + if (!e->callee + || DECL_BUILT_IN_CLASS (e->callee->decl) != BUILT_IN_NORMAL + || DECL_FUNCTION_CODE (e->callee->decl) != BUILT_IN_UNREACHABLE) + cgraph_redirect_edge_callee (e, new_node); } Index: ipa-inline.c =================================================================== --- ipa-inline.c (revision 208829) +++ ipa-inline.c (working copy) @@ -1685,7 +1685,7 @@ inline_small_functions (void) edge = (struct cgraph_edge *) fibheap_extract_min (edge_heap); gcc_assert (edge->aux); edge->aux = NULL; - if (!edge->inline_failed) + if (!edge->inline_failed || !edge->callee->analyzed) continue; /* Be sure that caches are maintained consistent. Index: ipa-inline.h =================================================================== --- ipa-inline.h (revision 208829) +++ ipa-inline.h (working copy) @@ -285,7 +285,8 @@ static inline int estimate_edge_growth (struct cgraph_edge *edge) { #ifdef ENABLE_CHECKING - gcc_checking_assert (inline_edge_summary (edge)->call_stmt_size); + gcc_checking_assert (inline_edge_summary (edge)->call_stmt_size + || !edge->callee->analyzed); #endif return (estimate_edge_size (edge) - inline_edge_summary (edge)->call_stmt_size); Index: testsuite/g++.dg/torture/pr60315.C =================================================================== --- testsuite/g++.dg/torture/pr60315.C (revision 0) +++ testsuite/g++.dg/torture/pr60315.C (revision 0) @@ -0,0 +1,32 @@ +// { dg-do compile } +struct Base { + virtual int f() = 0; +}; + +struct Derived : public Base { + virtual int f() final override { + return 42; + } +}; + +extern Base* b; + +int main() { + return (static_cast<Derived*>(b)->*(&Derived::f))(); +} +// { dg-do compile } +struct Base { + virtual int f() = 0; +}; + +struct Derived : public Base { + virtual int f() final override { + return 42; + } +}; + +extern Base* b; + +int main() { + return (static_cast<Derived*>(b)->*(&Derived::f))(); +} Index: cgraph.c =================================================================== --- cgraph.c (revision 208829) +++ cgraph.c (working copy) @@ -2612,6 +2612,12 @@ verify_edge_corresponds_to_fndecl (struc || node->in_other_partition || e->callee->in_other_partition) return false; + + /* Optimizers can redirect unreachable calls or calls triggering undefined + behaviour to builtin_unreachable. */ + if (DECL_BUILT_IN_CLASS (e->callee->decl) == BUILT_IN_NORMAL + && DECL_FUNCTION_CODE (e->callee->decl) == BUILT_IN_UNREACHABLE) + return false; node = cgraph_function_or_thunk_node (node, NULL); if (e->callee->former_clone_of != node->decl Index: ipa-inline-analysis.c =================================================================== --- ipa-inline-analysis.c (revision 208829) +++ ipa-inline-analysis.c (working copy) @@ -746,6 +746,20 @@ static void edge_set_predicate (struct cgraph_edge *e, struct predicate *predicate) { struct inline_edge_summary *es = inline_edge_summary (e); + + /* If the edge is determined to be never executed, redirect it + to BUILTIN_UNREACHABLE to save inliner from inlining into it. */ + if (predicate && false_predicate_p (predicate) && e->callee) + { + struct cgraph_node *callee = !e->inline_failed ? e->callee : NULL; + + cgraph_redirect_edge_callee (e, + cgraph_get_create_node + (builtin_decl_implicit (BUILT_IN_UNREACHABLE))); + e->inline_failed = CIF_UNREACHABLE; + if (callee) + cgraph_remove_node_and_inline_clones (callee, NULL); + } if (predicate && !true_predicate_p (predicate)) { if (!es->predicate) @@ -1724,12 +1738,20 @@ set_cond_stmt_execution_predicate (struc FOR_EACH_EDGE (e, ei, bb->succs) { - struct predicate p = add_condition (summary, index, &aggpos, - e->flags & EDGE_TRUE_VALUE - ? code : inverted_code, - gimple_cond_rhs (last)); - e->aux = pool_alloc (edge_predicate_pool); - *(struct predicate *) e->aux = p; + enum tree_code this_code = (e->flags & EDGE_TRUE_VALUE + ? code : inverted_code); + /* invert_tree_comparison will return ERROR_MARK on FP + comparsions that are not EQ/NE instead of returning proper + unordered one. Be sure it is not confused with NON_CONSTANT. */ + if (this_code != ERROR_MARK) + { + struct predicate p = add_condition (summary, index, &aggpos, + e->flags & EDGE_TRUE_VALUE + ? code : inverted_code, + gimple_cond_rhs (last)); + e->aux = pool_alloc (edge_predicate_pool); + *(struct predicate *) e->aux = p; + } } } Index: ipa-cp.c =================================================================== --- ipa-cp.c (revision 208829) +++ ipa-cp.c (working copy) @@ -2811,9 +2811,7 @@ create_specialized_node (struct cgraph_n if (aggvals) ipa_dump_agg_replacement_values (dump_file, aggvals); } - gcc_checking_assert (ipa_node_params_vector.exists () - && (ipa_node_params_vector.length () - > (unsigned) cgraph_max_uid)); + ipa_check_create_node_params (); update_profiling_info (node, new_node); new_info = IPA_NODE_REF (new_node); new_info->ipcp_orig_node = node;