| Message ID | 20100916234812.GE6006@kam.mff.cuni.cz |
|---|---|
| State | New |
| Headers | show |
On Fri, 17 Sep 2010, Jan Hubicka wrote: > Hi, > this patch fixes missed devirtualization seen in the attached testcase. > (shamelessly stolen from the testsuite). > > The problem is that we never actually try to fold the statement at gimple level > and thus we never try gimple_fold_obj_type_ref_known_binfo. Adding it to > gimple_fold_obj_type_ref_known_binfo does not solve the problem since > gimple_fold_obj_type_ref_known_binfo. returns NULL. > > This is because of code I added into it to prevent referring static function > from other ltrans units. We call the function too early and cgraph is not built > yet and consequently it thinks function is not there. > > This patch adds static_object_in_other_unit_p that has fixed logic of detecting > this case and combine it with the other problem I encounter with my folding patch, > where we pick up values from external vtables that reffer to external static variables. > C++ represent these with EXTERN and STATIC flags together. > > The patch saves whopping 300 bytes on Mozilla binnary ;) > > Bootstrapped/regtested x86_64-linux, OK? Ok. Thanks, Richard. > > /* { dg-do compile } */ > /* { dg-options "-O1 -fdump-tree-ssa" } */ > > extern "C" { void abort(); } > > struct A > { > int d; > > A () { d = 123; } > A (const A & o) { d = o.d; } > A (volatile const A & o) { d = o.d + 2; } > }; > > A bar() > { > volatile A l; > return l; > } > > main() > { > A a = bar (); > > if (a.d != 125) > abort(); > > return 0; > } > /* We should devirtualize call to D::Run */ > /* { dg-final { scan-tree-dump-times "D::Run (" 1 "ssa"} } */ > /* { dg-final { cleanup-tree-dump "ssa" } } */ > > 2010-09-17 Jan Hubicka <jh@suse.cz> > Martin Jambor <mjabor@suse.cz> > Richard Guenther <rguenther@suse.de> > > * gimple-fold.c (static_object_in_other_unit_p): New function. > (canonicalize_constructor_val): Use it. > (gimple_fold_obj_type_ref_known_binfo): Likewise. > * gimplify.c (gimplify_call_expr): Use fold_stmt. > * cgraphunit.c (cgraph_analyze_function): Allocate bitmap obstack. > Index: gimple-fold.c > =================================================================== > --- gimple-fold.c (revision 164344) > +++ gimple-fold.c (working copy) > @@ -31,6 +31,60 @@ > #include "tree-ssa-propagate.h" > #include "target.h" > > +/* Return true when DECL is static object in other partition. > + In that case we must prevent folding as we can't refer to > + the symbol. > + > + We can get into it in two ways: > + 1) when analyzing C++ virtual tables. > + C++ virtual tables do have known constructors even > + when they are keyed to other compilation unit. > + Those tables can contain pointers to methods and vars > + in other units. Those methods have both STATIC and EXTERNAL > + set. > + 2) In WHOPR mode devirtualization might lead to reference > + to method that was partitioned elsehwere. > + In this case we have static VAR_DECL or FUNCTION_DECL > + that has no corresponding callgraph/varpool node > + declaring the body. */ > + > +static bool > +static_object_in_other_unit_p (tree decl) > +{ > + struct varpool_node *vnode; > + struct cgraph_node *node; > + if (!TREE_STATIC (decl) > + || TREE_PUBLIC (decl) || DECL_COMDAT (decl)) > + return false; > + /* External flag is set, so we deal with C++ reference > + to static object from other file. */ > + if (DECL_EXTERNAL (decl)) > + { > + /* Just be sure it is not big in frontend setting > + flags incorrectly. Those variables should never > + be finalized. */ > + gcc_checking_assert (!(vnode = varpool_get_node (decl)) > + || !vnode->finalized); > + return true; > + } > + /* We are not at ltrans stage; so don't worry about WHOPR. */ > + if (!flag_ltrans) > + return false; > + if (TREE_CODE (decl) == FUNCTION_DECL) > + { > + node = cgraph_get_node (decl); > + if (!node || !node->analyzed) > + return true; > + } > + else if (TREE_CODE (decl) == VAR_DECL) > + { > + vnode = varpool_get_node (decl); > + if (!vnode || !vnode->finalized) > + return true; > + } > + return false; > +} > + > /* CVAL is value taken from DECL_INITIAL of variable. Try to transorm it into > acceptable form for is_gimple_min_invariant. */ > > @@ -50,6 +104,11 @@ > if (TREE_CODE (cval) == ADDR_EXPR) > { > tree base = get_base_address (TREE_OPERAND (cval, 0)); > + if (base > + && (TREE_CODE (base) == VAR_DECL > + || TREE_CODE (base) == FUNCTION_DECL) > + && static_object_in_other_unit_p (base)) > + return NULL; > if (base && TREE_CODE (base) == VAR_DECL) > add_referenced_var (base); > } > @@ -1370,7 +1428,6 @@ > { > HOST_WIDE_INT i; > tree v, fndecl; > - struct cgraph_node *node; > > v = BINFO_VIRTUALS (known_binfo); > i = 0; > @@ -1382,13 +1439,11 @@ > } > > fndecl = TREE_VALUE (v); > - node = cgraph_get_node (fndecl); > /* When cgraph node is missing and function is not public, we cannot > devirtualize. This can happen in WHOPR when the actual method > ends up in other partition, because we found devirtualization > possibility too late. */ > - if ((!node || (!node->analyzed && !node->in_other_partition)) > - && (!TREE_PUBLIC (fndecl) || DECL_COMDAT (fndecl))) > + if (static_object_in_other_unit_p (fndecl)) > return NULL; > return build_fold_addr_expr (fndecl); > } > Index: gimplify.c > =================================================================== > --- gimplify.c (revision 164344) > +++ gimplify.c (working copy) > @@ -2479,8 +2479,11 @@ > { > /* The CALL_EXPR in *EXPR_P is already in GIMPLE form, so all we > have to do is replicate it as a GIMPLE_CALL tuple. */ > + gimple_stmt_iterator gsi; > call = gimple_build_call_from_tree (*expr_p); > gimplify_seq_add_stmt (pre_p, call); > + gsi = gsi_last (*pre_p); > + fold_stmt (&gsi); > *expr_p = NULL_TREE; > } > > Index: cgraphunit.c > =================================================================== > --- cgraphunit.c (revision 164344) > +++ cgraphunit.c (working copy) > @@ -861,6 +867,7 @@ > static struct varpool_node *first_analyzed_var; > struct cgraph_node *node, *next; > > + bitmap_obstack_initialize (NULL); > process_function_and_variable_attributes (first_processed, > first_analyzed_var); > first_processed = cgraph_nodes; > @@ -971,6 +978,7 @@ > fprintf (cgraph_dump_file, "\n\nReclaimed "); > dump_cgraph (cgraph_dump_file); > } > + bitmap_obstack_release (NULL); > first_analyzed = cgraph_nodes; > ggc_collect (); > } > >
Hi, On Fri, Sep 17, 2010 at 01:48:12AM +0200, Jan Hubicka wrote: > Hi, > this patch fixes missed devirtualization seen in the attached testcase. > (shamelessly stolen from the testsuite). please generate your patches with the -p switch, this one is short but often I get lost rather easily without it. > ... > Index: gimplify.c > =================================================================== > --- gimplify.c (revision 164344) > +++ gimplify.c (working copy) > @@ -2479,8 +2479,11 @@ > { > /* The CALL_EXPR in *EXPR_P is already in GIMPLE form, so all we > have to do is replicate it as a GIMPLE_CALL tuple. */ > + gimple_stmt_iterator gsi; > call = gimple_build_call_from_tree (*expr_p); > gimplify_seq_add_stmt (pre_p, call); > + gsi = gsi_last (*pre_p); > + fold_stmt (&gsi); > *expr_p = NULL_TREE; > } I have just checked and we fold the OBJ_TYPE_REF even at -O0, is that something we want to do? Thanks, Martin
On Fri, 17 Sep 2010, Martin Jambor wrote: > Hi, > > On Fri, Sep 17, 2010 at 01:48:12AM +0200, Jan Hubicka wrote: > > Hi, > > this patch fixes missed devirtualization seen in the attached testcase. > > (shamelessly stolen from the testsuite). > > please generate your patches with the -p switch, this one is short but > often I get lost rather easily without it. > > > > > ... > > > Index: gimplify.c > > =================================================================== > > --- gimplify.c (revision 164344) > > +++ gimplify.c (working copy) > > @@ -2479,8 +2479,11 @@ > > { > > /* The CALL_EXPR in *EXPR_P is already in GIMPLE form, so all we > > have to do is replicate it as a GIMPLE_CALL tuple. */ > > + gimple_stmt_iterator gsi; > > call = gimple_build_call_from_tree (*expr_p); > > gimplify_seq_add_stmt (pre_p, call); > > + gsi = gsi_last (*pre_p); > > + fold_stmt (&gsi); > > *expr_p = NULL_TREE; > > } > > I have just checked and we fold the OBJ_TYPE_REF even at -O0, is that > something we want to do? We indeed do folding of builtins at -O0 as well (so printf becomes puts, etc.), which would be in favor of doing the obj-type-ref folding at -O0 as well. Richard.
Hi, On Fri, Sep 17, 2010 at 01:48:12AM +0200, Jan Hubicka wrote: > Hi, > this patch fixes missed devirtualization seen in the attached testcase. > (shamelessly stolen from the testsuite). > > The problem is that we never actually try to fold the statement at gimple level > and thus we never try gimple_fold_obj_type_ref_known_binfo. Adding it to > gimple_fold_obj_type_ref_known_binfo does not solve the problem since > gimple_fold_obj_type_ref_known_binfo. returns NULL. > > This is because of code I added into it to prevent referring static function > from other ltrans units. We call the function too early and cgraph is not built > yet and consequently it thinks function is not there. > > This patch adds static_object_in_other_unit_p that has fixed logic of detecting > this case and combine it with the other problem I encounter with my folding patch, > where we pick up values from external vtables that reffer to external static variables. > C++ represent these with EXTERN and STATIC flags together. > > The patch saves whopping 300 bytes on Mozilla binnary ;) > > Bootstrapped/regtested x86_64-linux, OK? unfortunately, for some reason Ada bootstrap fails for me with this patch (and only this patch) applied (on top of Monday's rev. 164243). Martin > > > /* { dg-do compile } */ > /* { dg-options "-O1 -fdump-tree-ssa" } */ > > extern "C" { void abort(); } > > struct A > { > int d; > > A () { d = 123; } > A (const A & o) { d = o.d; } > A (volatile const A & o) { d = o.d + 2; } > }; > > A bar() > { > volatile A l; > return l; > } > > main() > { > A a = bar (); > > if (a.d != 125) > abort(); > > return 0; > } > /* We should devirtualize call to D::Run */ > /* { dg-final { scan-tree-dump-times "D::Run (" 1 "ssa"} } */ > /* { dg-final { cleanup-tree-dump "ssa" } } */ > > 2010-09-17 Jan Hubicka <jh@suse.cz> > Martin Jambor <mjabor@suse.cz> > Richard Guenther <rguenther@suse.de> > > * gimple-fold.c (static_object_in_other_unit_p): New function. > (canonicalize_constructor_val): Use it. > (gimple_fold_obj_type_ref_known_binfo): Likewise. > * gimplify.c (gimplify_call_expr): Use fold_stmt. > * cgraphunit.c (cgraph_analyze_function): Allocate bitmap obstack. > Index: gimple-fold.c > =================================================================== > --- gimple-fold.c (revision 164344) > +++ gimple-fold.c (working copy) > @@ -31,6 +31,60 @@ > #include "tree-ssa-propagate.h" > #include "target.h" > > +/* Return true when DECL is static object in other partition. > + In that case we must prevent folding as we can't refer to > + the symbol. > + > + We can get into it in two ways: > + 1) when analyzing C++ virtual tables. > + C++ virtual tables do have known constructors even > + when they are keyed to other compilation unit. > + Those tables can contain pointers to methods and vars > + in other units. Those methods have both STATIC and EXTERNAL > + set. > + 2) In WHOPR mode devirtualization might lead to reference > + to method that was partitioned elsehwere. > + In this case we have static VAR_DECL or FUNCTION_DECL > + that has no corresponding callgraph/varpool node > + declaring the body. */ > + > +static bool > +static_object_in_other_unit_p (tree decl) > +{ > + struct varpool_node *vnode; > + struct cgraph_node *node; > + if (!TREE_STATIC (decl) > + || TREE_PUBLIC (decl) || DECL_COMDAT (decl)) > + return false; > + /* External flag is set, so we deal with C++ reference > + to static object from other file. */ > + if (DECL_EXTERNAL (decl)) > + { > + /* Just be sure it is not big in frontend setting > + flags incorrectly. Those variables should never > + be finalized. */ > + gcc_checking_assert (!(vnode = varpool_get_node (decl)) > + || !vnode->finalized); > + return true; > + } > + /* We are not at ltrans stage; so don't worry about WHOPR. */ > + if (!flag_ltrans) > + return false; > + if (TREE_CODE (decl) == FUNCTION_DECL) > + { > + node = cgraph_get_node (decl); > + if (!node || !node->analyzed) > + return true; > + } > + else if (TREE_CODE (decl) == VAR_DECL) > + { > + vnode = varpool_get_node (decl); > + if (!vnode || !vnode->finalized) > + return true; > + } > + return false; > +} > + > /* CVAL is value taken from DECL_INITIAL of variable. Try to transorm it into > acceptable form for is_gimple_min_invariant. */ > > @@ -50,6 +104,11 @@ > if (TREE_CODE (cval) == ADDR_EXPR) > { > tree base = get_base_address (TREE_OPERAND (cval, 0)); > + if (base > + && (TREE_CODE (base) == VAR_DECL > + || TREE_CODE (base) == FUNCTION_DECL) > + && static_object_in_other_unit_p (base)) > + return NULL; > if (base && TREE_CODE (base) == VAR_DECL) > add_referenced_var (base); > } > @@ -1370,7 +1428,6 @@ > { > HOST_WIDE_INT i; > tree v, fndecl; > - struct cgraph_node *node; > > v = BINFO_VIRTUALS (known_binfo); > i = 0; > @@ -1382,13 +1439,11 @@ > } > > fndecl = TREE_VALUE (v); > - node = cgraph_get_node (fndecl); > /* When cgraph node is missing and function is not public, we cannot > devirtualize. This can happen in WHOPR when the actual method > ends up in other partition, because we found devirtualization > possibility too late. */ > - if ((!node || (!node->analyzed && !node->in_other_partition)) > - && (!TREE_PUBLIC (fndecl) || DECL_COMDAT (fndecl))) > + if (static_object_in_other_unit_p (fndecl)) > return NULL; > return build_fold_addr_expr (fndecl); > } > Index: gimplify.c > =================================================================== > --- gimplify.c (revision 164344) > +++ gimplify.c (working copy) > @@ -2479,8 +2479,11 @@ > { > /* The CALL_EXPR in *EXPR_P is already in GIMPLE form, so all we > have to do is replicate it as a GIMPLE_CALL tuple. */ > + gimple_stmt_iterator gsi; > call = gimple_build_call_from_tree (*expr_p); > gimplify_seq_add_stmt (pre_p, call); > + gsi = gsi_last (*pre_p); > + fold_stmt (&gsi); > *expr_p = NULL_TREE; > } > > Index: cgraphunit.c > =================================================================== > --- cgraphunit.c (revision 164344) > +++ cgraphunit.c (working copy) > @@ -861,6 +867,7 @@ > static struct varpool_node *first_analyzed_var; > struct cgraph_node *node, *next; > > + bitmap_obstack_initialize (NULL); > process_function_and_variable_attributes (first_processed, > first_analyzed_var); > first_processed = cgraph_nodes; > @@ -971,6 +978,7 @@ > fprintf (cgraph_dump_file, "\n\nReclaimed "); > dump_cgraph (cgraph_dump_file); > } > + bitmap_obstack_release (NULL); > first_analyzed = cgraph_nodes; > ggc_collect (); > }
> Hi, > > On Fri, Sep 17, 2010 at 01:48:12AM +0200, Jan Hubicka wrote: > > Hi, > > this patch fixes missed devirtualization seen in the attached testcase. > > (shamelessly stolen from the testsuite). > > > > The problem is that we never actually try to fold the statement at gimple level > > and thus we never try gimple_fold_obj_type_ref_known_binfo. Adding it to > > gimple_fold_obj_type_ref_known_binfo does not solve the problem since > > gimple_fold_obj_type_ref_known_binfo. returns NULL. > > > > This is because of code I added into it to prevent referring static function > > from other ltrans units. We call the function too early and cgraph is not built > > yet and consequently it thinks function is not there. > > > > This patch adds static_object_in_other_unit_p that has fixed logic of detecting > > this case and combine it with the other problem I encounter with my folding patch, > > where we pick up values from external vtables that reffer to external static variables. > > C++ represent these with EXTERN and STATIC flags together. > > > > The patch saves whopping 300 bytes on Mozilla binnary ;) > > > > Bootstrapped/regtested x86_64-linux, OK? > > unfortunately, for some reason Ada bootstrap fails for me with this > patch (and only this patch) applied (on top of Monday's rev. 164243). I've unforutnately reproduced this too with the final checking before commit. I tested other parts except for the folding itself in gimplify separately and comitted them. I guess we are seeing a previously latent issue in folding. I tried to debug it but gdb seems especially unhlepful on binaries produced by new GCC (I can't single step etc.) I saw this behaviour for a while but never really needed to debug new gcc. I guess I will need updated gdb... Honza
> I've unforutnately reproduced this too with the final checking before commit. > I tested other parts except for the folding itself in gimplify separately and comitted them. > I guess we are seeing a previously latent issue in folding. I tried to debug it but gdb > seems especially unhlepful on binaries produced by new GCC (I can't single step etc.) > I saw this behaviour for a while but never really needed to debug new gcc. > I guess I will need updated gdb... Hmm, actually I can use stage1 gnat ;) The problem is that get_symbol_constant_value expect varpool to be built (to check const_value_known) and it is not. I already have plan for this to deal with early folding, so I will push those patches first and then follow with the actual folding change. Honza
Index: gimple-fold.c =================================================================== --- gimple-fold.c (revision 164344) +++ gimple-fold.c (working copy) @@ -31,6 +31,60 @@ #include "tree-ssa-propagate.h" #include "target.h" +/* Return true when DECL is static object in other partition. + In that case we must prevent folding as we can't refer to + the symbol. + + We can get into it in two ways: + 1) when analyzing C++ virtual tables. + C++ virtual tables do have known constructors even + when they are keyed to other compilation unit. + Those tables can contain pointers to methods and vars + in other units. Those methods have both STATIC and EXTERNAL + set. + 2) In WHOPR mode devirtualization might lead to reference + to method that was partitioned elsehwere. + In this case we have static VAR_DECL or FUNCTION_DECL + that has no corresponding callgraph/varpool node + declaring the body. */ + +static bool +static_object_in_other_unit_p (tree decl) +{ + struct varpool_node *vnode; + struct cgraph_node *node; + if (!TREE_STATIC (decl) + || TREE_PUBLIC (decl) || DECL_COMDAT (decl)) + return false; + /* External flag is set, so we deal with C++ reference + to static object from other file. */ + if (DECL_EXTERNAL (decl)) + { + /* Just be sure it is not big in frontend setting + flags incorrectly. Those variables should never + be finalized. */ + gcc_checking_assert (!(vnode = varpool_get_node (decl)) + || !vnode->finalized); + return true; + } + /* We are not at ltrans stage; so don't worry about WHOPR. */ + if (!flag_ltrans) + return false; + if (TREE_CODE (decl) == FUNCTION_DECL) + { + node = cgraph_get_node (decl); + if (!node || !node->analyzed) + return true; + } + else if (TREE_CODE (decl) == VAR_DECL) + { + vnode = varpool_get_node (decl); + if (!vnode || !vnode->finalized) + return true; + } + return false; +} + /* CVAL is value taken from DECL_INITIAL of variable. Try to transorm it into acceptable form for is_gimple_min_invariant. */ @@ -50,6 +104,11 @@ if (TREE_CODE (cval) == ADDR_EXPR) { tree base = get_base_address (TREE_OPERAND (cval, 0)); + if (base + && (TREE_CODE (base) == VAR_DECL + || TREE_CODE (base) == FUNCTION_DECL) + && static_object_in_other_unit_p (base)) + return NULL; if (base && TREE_CODE (base) == VAR_DECL) add_referenced_var (base); } @@ -1370,7 +1428,6 @@ { HOST_WIDE_INT i; tree v, fndecl; - struct cgraph_node *node; v = BINFO_VIRTUALS (known_binfo); i = 0; @@ -1382,13 +1439,11 @@ } fndecl = TREE_VALUE (v); - node = cgraph_get_node (fndecl); /* When cgraph node is missing and function is not public, we cannot devirtualize. This can happen in WHOPR when the actual method ends up in other partition, because we found devirtualization possibility too late. */ - if ((!node || (!node->analyzed && !node->in_other_partition)) - && (!TREE_PUBLIC (fndecl) || DECL_COMDAT (fndecl))) + if (static_object_in_other_unit_p (fndecl)) return NULL; return build_fold_addr_expr (fndecl); } Index: gimplify.c =================================================================== --- gimplify.c (revision 164344) +++ gimplify.c (working copy) @@ -2479,8 +2479,11 @@ { /* The CALL_EXPR in *EXPR_P is already in GIMPLE form, so all we have to do is replicate it as a GIMPLE_CALL tuple. */ + gimple_stmt_iterator gsi; call = gimple_build_call_from_tree (*expr_p); gimplify_seq_add_stmt (pre_p, call); + gsi = gsi_last (*pre_p); + fold_stmt (&gsi); *expr_p = NULL_TREE; } Index: cgraphunit.c =================================================================== --- cgraphunit.c (revision 164344) +++ cgraphunit.c (working copy) @@ -861,6 +867,7 @@ static struct varpool_node *first_analyzed_var; struct cgraph_node *node, *next; + bitmap_obstack_initialize (NULL); process_function_and_variable_attributes (first_processed, first_analyzed_var); first_processed = cgraph_nodes; @@ -971,6 +978,7 @@ fprintf (cgraph_dump_file, "\n\nReclaimed "); dump_cgraph (cgraph_dump_file); } + bitmap_obstack_release (NULL); first_analyzed = cgraph_nodes; ggc_collect (); }
Hi, this patch fixes missed devirtualization seen in the attached testcase. (shamelessly stolen from the testsuite). The problem is that we never actually try to fold the statement at gimple level and thus we never try gimple_fold_obj_type_ref_known_binfo. Adding it to gimple_fold_obj_type_ref_known_binfo does not solve the problem since gimple_fold_obj_type_ref_known_binfo. returns NULL. This is because of code I added into it to prevent referring static function from other ltrans units. We call the function too early and cgraph is not built yet and consequently it thinks function is not there. This patch adds static_object_in_other_unit_p that has fixed logic of detecting this case and combine it with the other problem I encounter with my folding patch, where we pick up values from external vtables that reffer to external static variables. C++ represent these with EXTERN and STATIC flags together. The patch saves whopping 300 bytes on Mozilla binnary ;) Bootstrapped/regtested x86_64-linux, OK? /* { dg-do compile } */ /* { dg-options "-O1 -fdump-tree-ssa" } */ extern "C" { void abort(); } struct A { int d; A () { d = 123; } A (const A & o) { d = o.d; } A (volatile const A & o) { d = o.d + 2; } }; A bar() { volatile A l; return l; } main() { A a = bar (); if (a.d != 125) abort(); return 0; } /* We should devirtualize call to D::Run */ /* { dg-final { scan-tree-dump-times "D::Run (" 1 "ssa"} } */ /* { dg-final { cleanup-tree-dump "ssa" } } */ 2010-09-17 Jan Hubicka <jh@suse.cz> Martin Jambor <mjabor@suse.cz> Richard Guenther <rguenther@suse.de> * gimple-fold.c (static_object_in_other_unit_p): New function. (canonicalize_constructor_val): Use it. (gimple_fold_obj_type_ref_known_binfo): Likewise. * gimplify.c (gimplify_call_expr): Use fold_stmt. * cgraphunit.c (cgraph_analyze_function): Allocate bitmap obstack.