WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
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
<
rdar://problem/10810732
>
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
Comment on
attachment 125537
[details]
the patch
Attachment 125537
[details]
did not pass qt-ews (qt): Output:
http://queues.webkit.org/results/11427257
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
Landed in
http://trac.webkit.org/changeset/106775
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug