Message ID | alpine.LNX.2.00.1209121612000.9940@wotan.suse.de |
---|---|
State | New |
Headers | show |
Hi, On Wed, Sep 12, 2012 at 04:17:45PM +0200, Michael Matz wrote: > Hi, > > On Wed, 12 Sep 2012, Michael Matz wrote: > > > > Hm, but we shouldn't end up streaming any BLOCKs at this point (nor > > > local TYPE_DECLs). Those are supposed to be in the local function > > > sections only where no fixup for prevailing decls happens. > > > > That's true, something is fishy with the patch, will try to investigate. > > ipa-prop creates the problem. Its tree mapping can contain expressions, > expressions can have locations, locations now have blocks. The tree maps > are stored as part of jump functions, and hence as part of node summaries. > Node summaries are global, hence blocks, and therefore block vars can be > placed in the global blob. > > That's not supposed to happen. The patch below fixes this instance of the > problem and makes the testcase work with Dehaos patch with the > LTO_NO_PREVAIL call added back in. > > > Ciao, > Michael. > ------------ > Index: lto-cgraph.c > =================================================================== > --- lto-cgraph.c (revision 190803) > +++ lto-cgraph.c (working copy) > @@ -1373,6 +1373,7 @@ output_node_opt_summary (struct output_b > mechanism to store function local declarations into summaries. */ > gcc_assert (parm); > streamer_write_uhwi (ob, parm_num); > + gcc_assert (IS_UNKNOWN_LOCATION (EXPR_LOCATION (map->new_tree))); > stream_write_tree (ob, map->new_tree, true); > bp = bitpack_create (ob->main_stream); > bp_pack_value (&bp, map->replace_p, 1); > Index: ipa-prop.c > =================================================================== > --- ipa-prop.c (revision 190803) > +++ ipa-prop.c (working copy) > @@ -1378,7 +1378,11 @@ ipa_compute_jump_functions_for_edge (str > tree arg = gimple_call_arg (call, n); > > if (is_gimple_ip_invariant (arg)) > - ipa_set_jf_constant (jfunc, arg); > + { > + arg = unshare_expr (arg); > + SET_EXPR_LOCATION (arg, UNKNOWN_LOCATION); > + ipa_set_jf_constant (jfunc, arg); > + } > else if (!is_gimple_reg_type (TREE_TYPE (arg)) > && TREE_CODE (arg) == PARM_DECL) Perhaps it would be better if ipa_set_jf_constant did that, just in case we ever add another caller? Note that arithmetic functions also have their second operand tree stored in them and so perhaps ipa_set_jf_arith_pass_through should do the same. And I it is also necessary to do the same thing at the end of determine_known_aggregate_parts, i.e. before assignment to item->value. I can post a separate patch if necessary. I wasn't following this thread but I hope that streaming types does not cause this problem. If they do, there are quite a few in various jump functions and indirect call graph edges. Thanks, Martin > { > @@ -3154,6 +3158,7 @@ ipa_write_jump_function (struct output_b > stream_write_tree (ob, jump_func->value.known_type.component_type, true); > break; > case IPA_JF_CONST: > + gcc_assert (IS_UNKNOWN_LOCATION (EXPR_LOCATION (jump_func->value.constant))); > stream_write_tree (ob, jump_func->value.constant, true); > break; > case IPA_JF_PASS_THROUGH:
On Wed, Sep 12, 2012 at 4:37 PM, Martin Jambor <mjambor@suse.cz> wrote: > Hi, > > On Wed, Sep 12, 2012 at 04:17:45PM +0200, Michael Matz wrote: >> Hi, >> >> On Wed, 12 Sep 2012, Michael Matz wrote: >> >> > > Hm, but we shouldn't end up streaming any BLOCKs at this point (nor >> > > local TYPE_DECLs). Those are supposed to be in the local function >> > > sections only where no fixup for prevailing decls happens. >> > >> > That's true, something is fishy with the patch, will try to investigate. >> >> ipa-prop creates the problem. Its tree mapping can contain expressions, >> expressions can have locations, locations now have blocks. The tree maps >> are stored as part of jump functions, and hence as part of node summaries. >> Node summaries are global, hence blocks, and therefore block vars can be >> placed in the global blob. >> >> That's not supposed to happen. The patch below fixes this instance of the >> problem and makes the testcase work with Dehaos patch with the >> LTO_NO_PREVAIL call added back in. >> >> >> Ciao, >> Michael. >> ------------ >> Index: lto-cgraph.c >> =================================================================== >> --- lto-cgraph.c (revision 190803) >> +++ lto-cgraph.c (working copy) >> @@ -1373,6 +1373,7 @@ output_node_opt_summary (struct output_b >> mechanism to store function local declarations into summaries. */ >> gcc_assert (parm); >> streamer_write_uhwi (ob, parm_num); >> + gcc_assert (IS_UNKNOWN_LOCATION (EXPR_LOCATION (map->new_tree))); >> stream_write_tree (ob, map->new_tree, true); >> bp = bitpack_create (ob->main_stream); >> bp_pack_value (&bp, map->replace_p, 1); >> Index: ipa-prop.c >> =================================================================== >> --- ipa-prop.c (revision 190803) >> +++ ipa-prop.c (working copy) >> @@ -1378,7 +1378,11 @@ ipa_compute_jump_functions_for_edge (str >> tree arg = gimple_call_arg (call, n); >> >> if (is_gimple_ip_invariant (arg)) >> - ipa_set_jf_constant (jfunc, arg); >> + { >> + arg = unshare_expr (arg); >> + SET_EXPR_LOCATION (arg, UNKNOWN_LOCATION); >> + ipa_set_jf_constant (jfunc, arg); >> + } >> else if (!is_gimple_reg_type (TREE_TYPE (arg)) >> && TREE_CODE (arg) == PARM_DECL) > > Perhaps it would be better if ipa_set_jf_constant did that, just in > case we ever add another caller? Note that arithmetic functions also > have their second operand tree stored in them and so perhaps > ipa_set_jf_arith_pass_through should do the same. > > And I it is also necessary to do the same thing at the end of > determine_known_aggregate_parts, i.e. before assignment to > item->value. I can post a separate patch if necessary. > > I wasn't following this thread but I hope that streaming types does > not cause this problem. If they do, there are quite a few in various > jump functions and indirect call graph edges. Well, or if jump functions would not use trees ... Richard. > Thanks, > > Martin > > >> { >> @@ -3154,6 +3158,7 @@ ipa_write_jump_function (struct output_b >> stream_write_tree (ob, jump_func->value.known_type.component_type, true); >> break; >> case IPA_JF_CONST: >> + gcc_assert (IS_UNKNOWN_LOCATION (EXPR_LOCATION (jump_func->value.constant))); >> stream_write_tree (ob, jump_func->value.constant, true); >> break; >> case IPA_JF_PASS_THROUGH:
Hi, On Wed, Sep 12, 2012 at 04:47:11PM +0200, Richard Guenther wrote: > On Wed, Sep 12, 2012 at 4:37 PM, Martin Jambor <mjambor@suse.cz> wrote: > > On Wed, Sep 12, 2012 at 04:17:45PM +0200, Michael Matz wrote: > >> On Wed, 12 Sep 2012, Michael Matz wrote: > >> > >> > > Hm, but we shouldn't end up streaming any BLOCKs at this point (nor > >> > > local TYPE_DECLs). Those are supposed to be in the local function > >> > > sections only where no fixup for prevailing decls happens. > >> > > >> > That's true, something is fishy with the patch, will try to investigate. > >> > >> ipa-prop creates the problem. Its tree mapping can contain expressions, > >> expressions can have locations, locations now have blocks. The tree maps > >> are stored as part of jump functions, and hence as part of node summaries. > >> Node summaries are global, hence blocks, and therefore block vars can be > >> placed in the global blob. > >> > >> That's not supposed to happen. The patch below fixes this instance of the > >> problem and makes the testcase work with Dehaos patch with the > >> LTO_NO_PREVAIL call added back in. > >> > >> > >> Ciao, > >> Michael. > >> ------------ > >> Index: lto-cgraph.c > >> =================================================================== > >> --- lto-cgraph.c (revision 190803) > >> +++ lto-cgraph.c (working copy) > >> @@ -1373,6 +1373,7 @@ output_node_opt_summary (struct output_b > >> mechanism to store function local declarations into summaries. */ > >> gcc_assert (parm); > >> streamer_write_uhwi (ob, parm_num); > >> + gcc_assert (IS_UNKNOWN_LOCATION (EXPR_LOCATION (map->new_tree))); > >> stream_write_tree (ob, map->new_tree, true); > >> bp = bitpack_create (ob->main_stream); > >> bp_pack_value (&bp, map->replace_p, 1); > >> Index: ipa-prop.c > >> =================================================================== > >> --- ipa-prop.c (revision 190803) > >> +++ ipa-prop.c (working copy) > >> @@ -1378,7 +1378,11 @@ ipa_compute_jump_functions_for_edge (str > >> tree arg = gimple_call_arg (call, n); > >> > >> if (is_gimple_ip_invariant (arg)) > >> - ipa_set_jf_constant (jfunc, arg); > >> + { > >> + arg = unshare_expr (arg); > >> + SET_EXPR_LOCATION (arg, UNKNOWN_LOCATION); > >> + ipa_set_jf_constant (jfunc, arg); > >> + } > >> else if (!is_gimple_reg_type (TREE_TYPE (arg)) > >> && TREE_CODE (arg) == PARM_DECL) > > > > Perhaps it would be better if ipa_set_jf_constant did that, just in > > case we ever add another caller? Note that arithmetic functions also > > have their second operand tree stored in them and so perhaps > > ipa_set_jf_arith_pass_through should do the same. > > > > And I it is also necessary to do the same thing at the end of > > determine_known_aggregate_parts, i.e. before assignment to > > item->value. I can post a separate patch if necessary. > > > > I wasn't following this thread but I hope that streaming types does > > not cause this problem. If they do, there are quite a few in various > > jump functions and indirect call graph edges. > > Well, or if jump functions would not use trees ... > Eventually, yes, but it is not a short-term goal. At least I'd like to replace the type-based devirtualization with pointer tracking first, which will avoid the need to store (hopefully) all types and I will have a better idea how to represent the jump functions in a different way. I will still need access to VMT initializers to keep indirect inlining, indirect call inlining hints and IPA-CP devirtualization working, though. But as I said, this is all a long-term plan. Martin
> Hi, > > On Wed, Sep 12, 2012 at 04:17:45PM +0200, Michael Matz wrote: > > Hi, > > > > On Wed, 12 Sep 2012, Michael Matz wrote: > > > > > > Hm, but we shouldn't end up streaming any BLOCKs at this point (nor > > > > local TYPE_DECLs). Those are supposed to be in the local function > > > > sections only where no fixup for prevailing decls happens. > > > > > > That's true, something is fishy with the patch, will try to investigate. > > > > ipa-prop creates the problem. Its tree mapping can contain expressions, > > expressions can have locations, locations now have blocks. The tree maps > > are stored as part of jump functions, and hence as part of node summaries. > > Node summaries are global, hence blocks, and therefore block vars can be > > placed in the global blob. > > > > That's not supposed to happen. The patch below fixes this instance of the > > problem and makes the testcase work with Dehaos patch with the > > LTO_NO_PREVAIL call added back in. > > > > > > Ciao, > > Michael. > > ------------ > > Index: lto-cgraph.c > > =================================================================== > > --- lto-cgraph.c (revision 190803) > > +++ lto-cgraph.c (working copy) > > @@ -1373,6 +1373,7 @@ output_node_opt_summary (struct output_b > > mechanism to store function local declarations into summaries. */ > > gcc_assert (parm); > > streamer_write_uhwi (ob, parm_num); > > + gcc_assert (IS_UNKNOWN_LOCATION (EXPR_LOCATION (map->new_tree))); > > stream_write_tree (ob, map->new_tree, true); > > bp = bitpack_create (ob->main_stream); > > bp_pack_value (&bp, map->replace_p, 1); > > Index: ipa-prop.c > > =================================================================== > > --- ipa-prop.c (revision 190803) > > +++ ipa-prop.c (working copy) > > @@ -1378,7 +1378,11 @@ ipa_compute_jump_functions_for_edge (str > > tree arg = gimple_call_arg (call, n); > > > > if (is_gimple_ip_invariant (arg)) > > - ipa_set_jf_constant (jfunc, arg); > > + { > > + arg = unshare_expr (arg); > > + SET_EXPR_LOCATION (arg, UNKNOWN_LOCATION); > > + ipa_set_jf_constant (jfunc, arg); > > + } > > else if (!is_gimple_reg_type (TREE_TYPE (arg)) > > && TREE_CODE (arg) == PARM_DECL) > > Perhaps it would be better if ipa_set_jf_constant did that, just in > case we ever add another caller? Note that arithmetic functions also > have their second operand tree stored in them and so perhaps > ipa_set_jf_arith_pass_through should do the same. > > And I it is also necessary to do the same thing at the end of > determine_known_aggregate_parts, i.e. before assignment to > item->value. I can post a separate patch if necessary. Yes, this seem resonable thing to do. Patchees for this are preapproved. > > I wasn't following this thread but I hope that streaming types does > not cause this problem. If they do, there are quite a few in various > jump functions and indirect call graph edges. We probably ought to ban streaming in BLOCK_DECL and other beasts that are not expected at WPA stage. Concerning Richi's suggestion to move jump functions completely away from trees, I am really not 100% sure how good idea it is in long term. As IPA analysis are getting more accurate we will stream more and more expressions. I am not convinced it makes sense to reinvent way of representing them rather than fixing what we have. Honza
Index: lto-cgraph.c =================================================================== --- lto-cgraph.c (revision 190803) +++ lto-cgraph.c (working copy) @@ -1373,6 +1373,7 @@ output_node_opt_summary (struct output_b mechanism to store function local declarations into summaries. */ gcc_assert (parm); streamer_write_uhwi (ob, parm_num); + gcc_assert (IS_UNKNOWN_LOCATION (EXPR_LOCATION (map->new_tree))); stream_write_tree (ob, map->new_tree, true); bp = bitpack_create (ob->main_stream); bp_pack_value (&bp, map->replace_p, 1); Index: ipa-prop.c =================================================================== --- ipa-prop.c (revision 190803) +++ ipa-prop.c (working copy) @@ -1378,7 +1378,11 @@ ipa_compute_jump_functions_for_edge (str tree arg = gimple_call_arg (call, n); if (is_gimple_ip_invariant (arg)) - ipa_set_jf_constant (jfunc, arg); + { + arg = unshare_expr (arg); + SET_EXPR_LOCATION (arg, UNKNOWN_LOCATION); + ipa_set_jf_constant (jfunc, arg); + } else if (!is_gimple_reg_type (TREE_TYPE (arg)) && TREE_CODE (arg) == PARM_DECL) { @@ -3154,6 +3158,7 @@ ipa_write_jump_function (struct output_b stream_write_tree (ob, jump_func->value.known_type.component_type, true); break; case IPA_JF_CONST: + gcc_assert (IS_UNKNOWN_LOCATION (EXPR_LOCATION (jump_func->value.constant))); stream_write_tree (ob, jump_func->value.constant, true); break; case IPA_JF_PASS_THROUGH: