Bug 77797 - DFG's child references from one node to another should have room for type information
Summary: DFG's child references from one node to another should have room for type inf...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-02-03 18:46 PST by Filip Pizlo
Modified: 2012-02-05 22:45 PST (History)
2 users (show)

See Also:


Attachments
it begins. (27.94 KB, patch)
2012-02-03 18:47 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
getting closer (38.36 KB, patch)
2012-02-03 19:05 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
64-bit works (94.65 KB, patch)
2012-02-04 23:37 PST, Filip Pizlo
gyuyoung.kim: commit-queue-
Details | Formatted Diff | Diff
the patch (120.45 KB, patch)
2012-02-05 15:12 PST, Filip Pizlo
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
the patch (120.45 KB, patch)
2012-02-05 15:27 PST, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (120.68 KB, patch)
2012-02-05 17:22 PST, Filip Pizlo
oliver: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 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.
Comment 1 Filip Pizlo 2012-02-03 18:47:51 PST
Created attachment 125460 [details]
it begins.

Work in progress.  Three files down, 51 to go.
Comment 2 Filip Pizlo 2012-02-03 19:05:26 PST
Created attachment 125466 [details]
getting closer
Comment 3 Filip Pizlo 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.
Comment 4 Gyuyoung Kim 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
Comment 5 Early Warning System Bot 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
Comment 6 Gustavo Noronha (kov) 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
Comment 7 Filip Pizlo 2012-02-05 15:09:25 PST
<rdar://problem/10810732>
Comment 8 Filip Pizlo 2012-02-05 15:12:20 PST
Created attachment 125537 [details]
the patch

Made both 32_64 and 64 work.
Comment 9 Early Warning System Bot 2012-02-05 15:21:11 PST
Comment on attachment 125537 [details]
the patch

Attachment 125537 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11427257
Comment 10 Filip Pizlo 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.
Comment 11 Oliver Hunt 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.
Comment 12 Filip Pizlo 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.
Comment 13 Filip Pizlo 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.
Comment 14 Filip Pizlo 2012-02-05 22:45:00 PST
Landed in http://trac.webkit.org/changeset/106775