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)
> 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.
