Re: [PATCH] Combine location with block using block_locations

[Jan Hubicka <hubicka at ucw dot cz>]

> 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