Bug 86905 - DFG CFA should record if a node can OSR exit
Summary: DFG CFA should record if a node can OSR exit
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:
Depends on: 87385
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-18 15:03 PDT by Filip Pizlo
Modified: 2012-05-24 06:14 PDT (History)
4 users (show)

See Also:


Attachments
it begins (26.33 KB, patch)
2012-05-18 15:05 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
it's starting to do things (48.08 KB, patch)
2012-05-18 17:59 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (58.76 KB, patch)
2012-05-19 21:38 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (63.84 KB, patch)
2012-05-20 00:35 PDT, 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-05-18 15:03:29 PDT
This will allow for easier understanding of backward flow, after CFA runs, which should in turn make it easier to implement redundant store elimination.
Comment 1 Filip Pizlo 2012-05-18 15:05:47 PDT
Created attachment 142795 [details]
it begins

This is a work in progress and will take a while to get right.
Comment 2 Filip Pizlo 2012-05-18 17:59:20 PDT
Created attachment 142835 [details]
it's starting to do things

But it doesn't completely work yet.
Comment 3 Filip Pizlo 2012-05-19 21:38:45 PDT
Created attachment 142892 [details]
the patch
Comment 4 Filip Pizlo 2012-05-20 00:35:28 PDT
Created attachment 142895 [details]
the patch

Found a few more places where we had unnecessary speculation checks.
Comment 5 Oliver Hunt 2012-05-20 06:08:37 PDT
Comment on attachment 142895 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142895&action=review

I'm too tired to actually finish this review (and i'm concerned about my ability to follow some of it right now), so i'll finish up/re-review later today.  I only got about half way through and it mostly looks fine, aside from your attempts to save columns ;p

I think it might be worth waiting until you've got the dfgopt branch merged completely to trunk, as I think we've dropped ByteArray from the tree, so there's a bit of code that will just disappear from this patch.

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:441
> +            speculateInt32Binary(
> +                node, !nodeCanTruncateInteger(node.arithNodeFlags()));

one line we don't have an 80 column limit :P

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:473
> +            speculateInt32Binary(
> +                node, !nodeCanTruncateInteger(node.arithNodeFlags()));

and again

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:514
> +            speculateInt32Binary(
> +                node,
> +                !nodeCanTruncateInteger(node.arithNodeFlags())
> +                || !nodeCanIgnoreNegativeZero(node.arithNodeFlags()));

see how bad this is at one line.  I'd prefer a single line, but this may be excessive

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:555
> +        if (Node::shouldSpeculateInteger(
> +                m_graph[node.child1()], m_graph[node.child2()])
> +            && node.canSpeculateInteger()) {

one line

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:575
> +        if (m_graph[node.child1()].shouldSpeculateInteger()
> +            && node.canSpeculateInteger()) {
> +            speculateInt32Unary(node, true);

one line.  Are you doing this out of some desire to punish me?  What did i ever do to you? :-(

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:797
> +        if (Node::shouldSpeculateInteger(
> +                m_graph[node.child1()], m_graph[node.child2()])) {

one line

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:802
> +        if (Node::shouldSpeculateNumber(
> +                m_graph[node.child1()], m_graph[node.child2()])) {

one line

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:807
> +        if (Node::shouldSpeculateFinalObject(
> +                m_graph[node.child1()], m_graph[node.child2()])) {

one line

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:810
> +                !isFinalObjectPrediction(forNode(node.child1()).m_type)
> +                || !isFinalObjectPrediction(forNode(node.child2()).m_type));

i'd prefer one line here i think

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:819
> +        if (Node::shouldSpeculateArray(
> +                m_graph[node.child1()], m_graph[node.child2()])) {
> +            node.setCanExit(
> +                !isArrayPrediction(forNode(node.child1()).m_type)
> +                || !isArrayPrediction(forNode(node.child2()).m_type));

less lines, moar columns!!!

> Source/JavaScriptCore/dfg/DFGAbstractState.cpp:1065
> +        node.setCanExit(
> +            !isCellPrediction(forNode(node.child1()).m_type)
> +            || !isCellPrediction(forNode(node.child2()).m_type));

and again
Comment 6 Filip Pizlo 2012-05-22 10:18:54 PDT
Landed in http://trac.webkit.org/changeset/117931
Comment 7 Filip Pizlo 2012-05-23 23:24:16 PDT
Merged in http://trac.webkit.org/changeset/118325