Bug 86905

Created attachment 142795 [details] it begins This is a work in progress and will take a while to get right.

Created attachment 142835 [details] it's starting to do things But it doesn't completely work yet.

Created attachment 142892 [details] the patch

Created attachment 142895 [details] the patch Found a few more places where we had unnecessary speculation checks.

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

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

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