Re: Patch for PR39557

[Richard Guenther <richard dot guenther at gmail dot com>]

On Thu, Mar 26, 2009 at 12:40 AM, Xinliang David Li <davidxl@google.com> wrote:
> Hi, ?please find the attached patch for fixing PR39557.
>
> Some explanations:
>
> There are three parts of the patch:
>
> 1) tree-ssa.c change.
>
> In fact, this is the only change that is needed to fix the problem.
>
> 2) dominance.c
> ? ? params.def
>
> As suggested by Diego, I added the check. It was quite painful to
> triage the runtime problem in a very large multithreaded server
> program, so anything that helps converted to compile time is good.
> Diego suggested moving the check to verify_interpass_invariants -- but
> this doe not work during the lowering passes when the cfg is not even
> created (of course I can add the check for that). ?Doing the check on
> the fly when a pass need the dom info seems ok to me.
>
> The check is guarded under a parameter because more test failures will
> occur when it is turned on by default -- bugs will be filed later when
> this patch is checked in.
>
> 3) Ira related changes -- strictly not necessary for this -- but the
> problem was detected with the dom-validation check and it is easy to
> fix.
>
> About the fix itself:
>
> Independent of the invalid PDOM bug, ?the way tree-ssa-dce's cfg
> patchup when removing control statement looks suspicious (
> remove_dead_stmt):
>
> ? ? ?if (! post_dom_bb
> ? ? ? ? ?|| post_dom_bb == EXIT_BLOCK_PTR
> ? ? ? ? ?|| phi_nodes (post_dom_bb)) ? ? ? ? ? ? ? ? <------ ? ? ? ? ?????
> What is this?
> ? ? ? ?;
> ? ? ?else
> ? ? ? ?{
> ? ? ? ? ?/* Redirect the first edge out of BB to reach POST_DOM_BB. ?*/
> ? ? ? ? ?redirect_edge_and_branch (EDGE_SUCC (bb, 0), post_dom_bb);
> ? ? ? ? ?PENDING_STMT (EDGE_SUCC (bb, 0)) = NULL;
>
> ? ? ? ? ?/* It is not sufficient to set cfg_altered below during edge
> ? ? ? ? ? ? removal, in case BB has two successors and one of them
> ? ? ? ? ? ? is POST_DOM_BB. ?*/
> ? ? ? ? ?cfg_altered = true;
> ? ? ? ?}
> ? ? ?EDGE_SUCC (bb, 0)->probability = REG_BR_PROB_BASE;
> ? ? ?EDGE_SUCC (bb, 0)->count = bb->count;
>
> ? ? ?/* The edge is no longer associated with a conditional, so it does
> ? ? ? ? not have TRUE/FALSE flags. ?*/
> ? ? ?EDGE_SUCC (bb, 0)->flags &= ~(EDGE_TRUE_VALUE | EDGE_FALSE_VALUE);
>
> ? ? ?/* The lone outgoing edge from BB will be a fallthru edge. ?*/
> ? ? ?EDGE_SUCC (bb, 0)->flags |= EDGE_FALLTHRU;
>
> ? ? ?/* Remove the remaining the outgoing edges. ?*/
> ? ? ?while (!single_succ_p (bb))
> ? ? ? ? ? ?<---------------------- ? doing lottery if reaching from
> the 'if' statement above ??
> ? ? ? ?{
> ? ? ? ? ?/* FIXME. ?When we remove the edge, we modify the CFG, which
> ? ? ? ? ? ? in turn modifies the dominator and post-dominator tree.
> ? ? ? ? ? ? Is it safe to postpone recomputing the dominator and
> ? ? ? ? ? ? post-dominator tree until the end of this pass given that
> ? ? ? ? ? ? the post-dominators are used above? ?*/
> ? ? ? ? ?cfg_altered = true;
> ? ? ? ? ?remove_edge (EDGE_SUCC (bb, 1));
> ? ? ? ?}
> ? ?}
>
> The code marked with '<---" are direct cause (not the root one) that
> lead to the inifinte loop. ?Of course with the right PDOM info and the
> way loop is handled, the code won't be excersised, ?so ?I will leave
> it alone.
>
>
> By the way, is there a plan to make DOM/PDOM incremental update more robust?
>
> Thanks,

Bootstrapped and tested on ...?

The tree-ssa.c change is ok for trunk.  Note that the rule is that you
have to free pdom information if you compute it.

The other changes are not appropriate for this stage of development.
In particular the extra checking is in a way we don't want it IMHO
(instead you could add a check to passes.c to verify that pdom
information is never computed after a pass finished, or even better,
add a PROP_pdom which says that pdoms are kept valid by a pass
and otherwise delete the info).

Please use the reduced testcase from Jakub.

Thanks,
Richard.

> David
>