This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Patch for PR39557
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?
> Please use the reduced testcase from Jakub.
>
Sure do.
David
> Thanks,
> Richard.
>
>> David
>>
>