Re: [PATCH] Combine location with block using block_locations

[Martin Jambor <mjambor at suse dot cz>]

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