Re: Patch for PR39557

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

On Thu, Mar 26, 2009 at 5:48 PM, Xinliang David Li <davidxl@google.com> wrote:
> On Thu, Mar 26, 2009 at 2:43 AM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> 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 ...?
>>
>
> Sorry I missed the message --- bootstrapped and tested on linux-i686.
>
>
>
>> 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.
>
> Good to know.
>
>
>>
>> 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).
>>
>
> Sounds good. ?Please note that the other problems found with the check
> are not PDOM, but DOM info. Sounds more serious?

commit_edge_insertions uses split_edge which should update
dominator information, if that is not doing its job well it should
be fixed.

Richard.