RESOLVED FIXED 77797
DFG's child references from one node to another should have room for type information
https://bugs.webkit.org/show_bug.cgi?id=77797
Summary DFG's child references from one node to another should have room for type inf...
Filip Pizlo
Reported 2012-02-03 18:46:01 PST
This will allow us to reason about not just whether a node is used, but whether it's used for a particular purpose.
Attachments
it begins. (27.94 KB, patch)
2012-02-03 18:47 PST, Filip Pizlo
no flags
getting closer (38.36 KB, patch)
2012-02-03 19:05 PST, Filip Pizlo
no flags
64-bit works (94.65 KB, patch)
2012-02-04 23:37 PST, Filip Pizlo
gyuyoung.kim: commit-queue-
the patch (120.45 KB, patch)
2012-02-05 15:12 PST, Filip Pizlo
webkit-ews: commit-queue-
the patch (120.45 KB, patch)
2012-02-05 15:27 PST, Filip Pizlo
no flags
the patch (120.68 KB, patch)
2012-02-05 17:22 PST, Filip Pizlo
oliver: review+
Filip Pizlo
Comment 1 2012-02-03 18:47:51 PST
Created attachment 125460 [details] it begins. Work in progress. Three files down, 51 to go.
Filip Pizlo
Comment 2 2012-02-03 19:05:26 PST
Created attachment 125466 [details] getting closer
Filip Pizlo
Comment 3 2012-02-04 23:37:59 PST
Created attachment 125517 [details] 64-bit works Still need to do 32-64 and make sure I didn't screw up performance.
Gyuyoung Kim
Comment 4 2012-02-04 23:47:36 PST
Comment on attachment 125517 [details] 64-bit works Attachment 125517 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11424834
Early Warning System Bot
Comment 5 2012-02-04 23:49:18 PST
Comment on attachment 125517 [details] 64-bit works Attachment 125517 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11421879
Gustavo Noronha (kov)
Comment 6 2012-02-04 23:58:46 PST
Comment on attachment 125517 [details] 64-bit works Attachment 125517 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11427085
Filip Pizlo
Comment 7 2012-02-05 15:09:25 PST
Filip Pizlo
Comment 8 2012-02-05 15:12:20 PST
Created attachment 125537 [details] the patch Made both 32_64 and 64 work.
Early Warning System Bot
Comment 9 2012-02-05 15:21:11 PST
Filip Pizlo
Comment 10 2012-02-05 15:27:56 PST
Created attachment 125538 [details] the patch Fix a really bad operator precedence bug found by Qt EWS. Instead of writing (a << b) - c, I had written a << b - c, which is totally different.
Oliver Hunt
Comment 11 2012-02-05 17:08:59 PST
Comment on attachment 125538 [details] the patch View in context: https://bugs.webkit.org/attachment.cgi?id=125538&action=review Other than the comments included (below?) this looks fairly sensible to me. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:173 > - nodeIndex = addToGraph(GetLocal, OpInfo(variableAccessData), nodePtr->child1()); > + nodeIndex = addToGraph(GetLocal, OpInfo(variableAccessData), nodePtr->child1().index()); It seems like it would be nicer to make addToGraph take a Node/NodeUse rather than a NodeIndex, which would remove some of the churn in this diff. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2273 > + phiNode->children.child1() = NodeUse(valueInPredecessor); I'd prefer setChild1(..) to returning a mutable reference.
Filip Pizlo
Comment 12 2012-02-05 17:13:58 PST
(In reply to comment #11) > (From update of attachment 125538 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=125538&action=review > > Other than the comments included (below?) this looks fairly sensible to me. > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:173 > > - nodeIndex = addToGraph(GetLocal, OpInfo(variableAccessData), nodePtr->child1()); > > + nodeIndex = addToGraph(GetLocal, OpInfo(variableAccessData), nodePtr->child1().index()); > > It seems like it would be nicer to make addToGraph take a Node/NodeUse rather than a NodeIndex, which would remove some of the churn in this diff. Do you really think that's a good idea? There are 4 addToGraph() methods, and only 2 places where this patch needs to convert a NodeUse to NodeIndex for a call to addToGraph(). In general, ByteCodeParser almost never needs to create a node that references a child of another node - which is the only case where it would make sense for addToGraph() to take NodeUse. > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:2273 > > + phiNode->children.child1() = NodeUse(valueInPredecessor); > > I'd prefer setChild1(..) to returning a mutable reference. I can change those things, but it is necessary for NodeReferenceBlob to return a mutable reference. Otherwise we'd have no way of later doing a pass that ascribes types to edges.
Filip Pizlo
Comment 13 2012-02-05 17:22:39 PST
Created attachment 125545 [details] the patch Changed all present uses of childX() = foo with setChildX(foo). I did not remove the childX() methods that returned mutable references, because that would defeat the entire purpose of this patch.
Filip Pizlo
Comment 14 2012-02-05 22:45:00 PST
Note You need to log in before you can comment on or make changes to this bug.