[Webkit-unassigned] [Bug 80415] Eliminate redundant Phis in DFG

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=80415





--- Comment #6 from Yuqiang Xian <yuqiang.xian at intel.com>  2012-03-06 17:30:51 PST ---
(In reply to comment #4)
> I think this is great.  My intuition is that we could simplify this further by doing away with the phi use map.  But maybe I'm wrong?  Have you tried it without the use map?

No, I haven't tried it - I was just worried that traversing the whole graph for Phi replacement may be time consuming especially when the traversal can be iterative. We can try it and I think we should only need the iterative traversal on the Phi nodes (if we have the fix of bug 80361 it would be easier) and a one-time traversal on other nodes in the graph (actually, at current stage, the GetLocals) after all of the Phi replacements are identified.

> 
> > Source/JavaScriptCore/dfg/DFGAbstractState.cpp:1006
> > -    if (!node.refCount())
> > +    // We don't want to ignore the skipped Phi nodes, otherwise we may fail to
> > +    // propagate the type predictions to other blocks.
> > +    if (!node.refCount() && node.op != Phi)
> 
> Why?  The idea here is that if we have a node that claims that it is using a variable from another block, and its uses of that variable are dead, then we have no work to do.
> 

I will attach a case to explain this problem soon.

> 
> Not sure I buy that the phi use map is necessary.  I kind of don't like it because it means we cannot rerun the redundant phi replacement algorithm a second time unless we also maintain the consistency of this map.  In general, in the future I want to be able to have a phase that introduces new nodes or switches the children of nodes, and then rerun the phi simplification if I had also done some control flow simplification.  But playing with the children of nodes seems hard if I have to maintain the phi use map as I do it.
> 

Yes. In the redundant phi elimination, we maintain the consistency of this map. But on the other hand, it really brought troubles when we play with the children, though currently only GetLocals have Phi children (except for Phis), but in the future if we get rid of GetLocal/SetLocals (will?) that may be a problem.

> > Source/JavaScriptCore/dfg/DFGRedundantPhiEliminationPhase.cpp:87
> 
> Why not kill the switch statement and say useNode.children.child(use.childIndex() - 1)?
> 

Agree.

> > Source/JavaScriptCore/dfg/DFGRedundantPhiEliminationPhase.cpp:126
> 
> Can't we run this algorithm without having a phi use map?  Seems to me that we could:
> 
> 1) Wrap the algorithm in a fixpoint.  You're using a worklist; maybe we could do that too.
> 
> 2) Walk over the graph inside the fixpoint.
> 
> 3) For every child that is a phi and fits the simplification criteria, attempt to simplify.

It's possible to go without phi use map. I will try and see if it can be more efficient and simpler.

Thanks for the review!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.