On Thu, 3 Dec 2015, Richard Biener wrote:
> On Thu, 3 Dec 2015, Alan Lawrence wrote:
>
> > On 02/12/15 14:13, Jeff Law wrote:
> > > On 12/02/2015 01:33 AM, Richard Biener wrote:
> > > > > Right. So the question I have is how/why did DOM leave anything in
> > > > > the
> > > > > map.
> > > > > And if DOM is fixed to not leave stuff lying around, can we then
> > > > > assert
> > > > > that
> > > > > nothing is ever left in those maps between passes? There's certainly
> > > > > no
> > > > > good
> > > > > reason I'm aware of why DOM would leave things in this state.
> > > >
> > > > It happens not only with DOM but with all passes doing edge redirection.
> > > > This is because the map is populated by GIMPLE cfg hooks just in case
> > > > it might be used. But there is no such thing as a "start CFG manip"
> > > > and "end CFG manip" to cleanup such dead state.
> > > Sigh.
> > >
> > > >
> > > > IMHO the redirect-edge-var-map stuff is just the very most possible
> > > > unclean implementation possible. :( (see how remove_edge "clears"
> > > > stale info from the map to avoid even more "interesting" stale
> > > > data)
> > > >
> > > > Ideally we could assert the map is empty whenever we leave a pass,
> > > > but as said it triggers all over the place. Even cfg-cleanup causes
> > > > such stale data.
> > > >
> > > > I agree that the patch is only a half-way "solution", but a full
> > > > solution would require sth more explicit, like we do with
> > > > initialize_original_copy_tables/free_original_copy_tables. Thus
> > > > require passes to explicitely request the edge data to be preserved
> > > > with a initialize_edge_var_map/free_edge_var_map call pair.
> > > >
> > > > Not appropriate at this stage IMHO (well, unless it turns out to be
> > > > a very localized patch).
> > > So maybe as a follow-up to aid folks in the future, how about a debugging
> > > verify_whatever function that we can call manually if debugging a problem
> > > in
> > > this space. With a comment indicating why we can't call it
> > > unconditionally
> > > (yet).
> > >
> > >
> > > jeff
> >
> > I did a (fwiw disable bootstrap) build with the map-emptying code in
> > passes.c
> > (not functions.c), printing out passes after which the map was non-empty
> > (before emptying it, to make sure passes weren't just carrying through stale
> > data from earlier). My (non-exhaustive!) list of passes after which the
> > edge_var_redirect_map can be non-empty stands at...
> >
> > aprefetch ccp cddce ch ch_vect copyprop crited crited cselim cunroll
> > cunrolli
> > dce dom ehcleanup einline esra fab fnsplit forwprop fre graphite ifcvt
> > isolate-paths ldist lim local-pure-const mergephi oaccdevlow ompexpssa
> > optimized parloops pcom phicprop phiopt phiprop pre profile profile_estimate
> > sccp sink slsr split-paths sra switchconv tailc tailr tracer unswitch
> > veclower2 vect vrm vrp whole-program
>
> Yeah, exactly my findings... note that most of the above are likely
> due to cfgcleanup even though it already does sth like
>
> e = redirect_edge_and_branch (e, dest);
> redirect_edge_var_map_clear (e);
>
> so eventually placing a redirect_edge_var_map_empty () at the end
> of the cleanup_tree_cfg function should prune down the above list
> considerably (well, then assert the map is empty on entry to that
> function of course)
Maybe
Index: gcc/tree-cfgcleanup.c
===================================================================
--- gcc/tree-cfgcleanup.c (revision 231221)
+++ gcc/tree-cfgcleanup.c (working copy)
@@ -456,6 +456,7 @@ remove_forwarder_block (basic_block bb)
}
else
s = redirect_edge_and_branch (e, dest);
+ redirect_edge_var_map_clear (s);
if (s == e)
{
also helps...
Richard.
>
> > FWIW, the route by which dom added the edge to the redirect map was:
> > #0 redirect_edge_var_map_add (e=e@entry=0x7fb7a5f508, result=0x7fb725a000,
> > def=0x7fb78eaea0, locus=2147483884) at ../../gcc/gcc/tree-ssa.c:54
> > #1 0x0000000000cccf58 in ssa_redirect_edge (e=e@entry=0x7fb7a5f508,
> > dest=dest@entry=0x7fb79cc680) at ../../gcc/gcc/tree-ssa.c:158
> > #2 0x0000000000b00738 in gimple_redirect_edge_and_branch (e=0x7fb7a5f508,
> > dest=0x7fb79cc680) at ../../gcc/gcc/tree-cfg.c:5662
> > #3 0x00000000006ec678 in redirect_edge_and_branch (e=e@entry=0x7fb7a5f508,
> > dest=<optimised out>) at ../../gcc/gcc/cfghooks.c:356
> > #4 0x0000000000cb4530 in ssa_fix_duplicate_block_edges (rd=0x1a29f10,
> > local_info=local_info@entry=0x7fffffed40)
> > at ../../gcc/gcc/tree-ssa-threadupdate.c:1184
> > #5 0x0000000000cb5520 in ssa_fixup_template_block (slot=<optimised out>,
> > local_info=0x7fffffed40) at ../../gcc/gcc/tree-ssa-threadupdate.c:1369
> > #6 traverse_noresize<ssa_local_info_t*, ssa_fixup_template_block> (
> > argument=0x7fffffed40, this=0x1a21a00) at ../../gcc/gcc/hash-table.h:911
> > #7 traverse<ssa_local_info_t*, ssa_fixup_template_block> (
> > argument=0x7fffffed40, this=0x1a21a00) at ../../gcc/gcc/hash-table.h:933
> > #8 thread_block_1 (bb=bb@entry=0x7fb7485bc8,
> > noloop_only=noloop_only@entry=true, joiners=joiners@entry=true)
> > at ../../gcc/gcc/tree-ssa-threadupdate.c:1592
> > #9 0x0000000000cb5a40 in thread_block (bb=0x7fb7485bc8,
> > noloop_only=noloop_only@entry=true)
> > at ../../gcc/gcc/tree-ssa-threadupdate.c:1629
> > ---Type <return> to continue, or q <return> to quit---
> > #10 0x0000000000cb6bf8 in thread_through_all_blocks (
> > may_peel_loop_headers=true) at
> > ../../gcc/gcc/tree-ssa-threadupdate.c:2736
> > #11 0x0000000000becf6c in (anonymous namespace)::pass_dominator::execute (
> > this=<optimised out>, fun=0x7fb77d1b28)
> > at ../../gcc/gcc/tree-ssa-dom.c:622
> > #12 0x00000000009feef4 in execute_one_pass (pass=pass@entry=0x16d1a80)
> > at ../../gcc/gcc/passes.c:2311
> >
> > The edge is then deleted much later:
> > #3 0x0000000000f858e4 in free_edge (fn=<optimised out>, e=<optimised out>)
> > at ../../gcc/gcc/cfg.c:91
> > #4 remove_edge_raw (e=<optimised out>) at ../../gcc/gcc/cfg.c:350
> > #5 0x00000000006ec814 in remove_edge (e=<optimised out>)
> > at ../../gcc/gcc/cfghooks.c:418
> > #6 0x00000000006ecaec in delete_basic_block (bb=bb@entry=0x7fb74b3618)
> > at ../../gcc/gcc/cfghooks.c:597
> > #7 0x0000000000f8d1d4 in try_optimize_cfg (mode=32)
> > at ../../gcc/gcc/cfgcleanup.c:2701
> > #8 cleanup_cfg (mode=mode@entry=32) at ../../gcc/gcc/cfgcleanup.c:3028
> > #9 0x000000000070180c in cfg_layout_initialize (flags=flags@entry=0)
> > at ../../gcc/gcc/cfgrtl.c:4264
> > #10 0x0000000000f7cdc8 in (anonymous
> > namespace)::pass_duplicate_computed_gotos::execute (this=<optimised out>,
> > fun=0x7fb77d1b28)
> > at ../../gcc/gcc/bb-reorder.c:2622
> > #11 0x00000000009feef4 in execute_one_pass (pass=pass@entry=0x16d5680)
> > at ../../gcc/gcc/passes.c:2311
> >
> > Adding a call to redirect_edge_var_map_clear in cfg.c (free_edge), does fix
> > the ICE in polynom.c, but still leaves many passes ending with the redirect
> > map non-empty.
>
> It's also not correct - I think it's supposed to survive multiple
> edge redirections / deletions.
>
> That said I think we need to refactor this bookkeeping facility
> so something more sensible.
>
> Richard.
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
