This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Patch for PR39557
Sounds good.
David
On Thu, Mar 26, 2009 at 9:58 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> 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.
>