Bug 86905 – DFG CFA should record if a node can OSR exit

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

This will allow for easier understanding of backward flow, after CFA runs, which should in turn make it easier to implement redundant store elimination.

[Filip Pizlo]

Created attachment 142795 [details]
it begins

This is a work in progress and will take a while to get right.

[Filip Pizlo]

Created attachment 142835 [details]
it's starting to do things

But it doesn't completely work yet.

[Filip Pizlo]

Created attachment 142892 [details]
the patch

[Filip Pizlo]

Created attachment 142895 [details]
the patch

Found a few more places where we had unnecessary speculation checks.

[Oliver Hunt]

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

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/117931

[Filip Pizlo]

Merged in http://trac.webkit.org/changeset/118325