Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[JSC] Optimize more cases of something-compared-to-null/undefined
https://bugs.webkit.org/show_bug.cgi?id=148157

Patch by Benjamin Poulain <bpoulain@apple.com> on 2015-08-18
Reviewed by Geoffrey Garen and Filip Pizlo.

Source/JavaScriptCore:

CompareEq is fairly trivial if you assert one of the operands is either
null or undefined. Under those conditions, the only way to have "true"
is to have the other operand be null/undefined or have an object
that masquerades to undefined.

JSC already had a fast path in CompareEqConstant.
With this patch, I generalize this fast path to more cases and try
to eliminate the checks whenever possible.

CompareEq now does the job of CompareEqConstant. If any operand can
be proved to be undefined/other, its edge is set to OtherUse. Whenever
any edge is OtherUse, we generate the fast code we had for CompareEqConstant.

The AbstractInterpreter has additional checks to reduce the node to a constant
whenever possible.

There are two additional changes in this patch:
-The Fixup Phase tries to set edges to OtherUse early. This is done correctly
 in ConstantFoldingPhase but setting it up early helps the phases relying
 on Clobberize.
-The codegen for CompareEqConstant was improved. The reason is the comparison
 for ObjectOrOther could be faster just because the codegen was better.

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseBlock):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize): Deleted.
* dfg/DFGConstantFoldingPhase.cpp:
(JSC::DFG::ConstantFoldingPhase::foldConstants):
* dfg/DFGDoesGC.cpp:
(JSC::DFG::doesGC): Deleted.
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGNode.h:
(JSC::DFG::Node::isUndefinedOrNullConstant):
* dfg/DFGNodeType.h:
* dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::propagate): Deleted.
* dfg/DFGSafeToExecute.h:
(JSC::DFG::safeToExecute): Deleted.
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compilePeepHoleBranch):
(JSC::DFG::SpeculativeJIT::compare):
* dfg/DFGSpeculativeJIT.h:
(JSC::DFG::SpeculativeJIT::isKnownNotOther):
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::nonSpeculativeNonPeepholeCompareNullOrUndefined):
(JSC::DFG::SpeculativeJIT::nonSpeculativePeepholeBranchNullOrUndefined):
(JSC::DFG::SpeculativeJIT::nonSpeculativeNonPeepholeCompareNull): Deleted.
(JSC::DFG::SpeculativeJIT::nonSpeculativePeepholeBranchNull): Deleted.
(JSC::DFG::SpeculativeJIT::nonSpeculativeCompareNull): Deleted.
(JSC::DFG::SpeculativeJIT::compile): Deleted.
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::nonSpeculativeNonPeepholeCompareNullOrUndefined):
(JSC::DFG::SpeculativeJIT::nonSpeculativePeepholeBranchNullOrUndefined):
(JSC::DFG::SpeculativeJIT::nonSpeculativeNonPeepholeCompareNull): Deleted.
(JSC::DFG::SpeculativeJIT::nonSpeculativePeepholeBranchNull): Deleted.
(JSC::DFG::SpeculativeJIT::nonSpeculativeCompareNull): Deleted.
(JSC::DFG::SpeculativeJIT::compile): Deleted.
* dfg/DFGValidate.cpp:
(JSC::DFG::Validate::validate): Deleted.
* dfg/DFGWatchpointCollectionPhase.cpp:
(JSC::DFG::WatchpointCollectionPhase::handle):
* ftl/FTLCapabilities.cpp:
(JSC::FTL::canCompile):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::DFG::LowerDFGToLLVM::compileCompareEq):
(JSC::FTL::DFG::LowerDFGToLLVM::compileNode): Deleted.
(JSC::FTL::DFG::LowerDFGToLLVM::compileCompareEqConstant): Deleted.
* tests/stress/compare-eq-on-null-and-undefined-non-peephole.js: Added.
(string_appeared_here.useForMath):
(testUseForMath):
* tests/stress/compare-eq-on-null-and-undefined-optimized-in-constant-folding.js: Added.
(string_appeared_here.unreachableCodeTest):
(inlinedCompareToNull):
(inlinedComparedToUndefined):
(warmupInlineFunctions):
(testInlineFunctions):
* tests/stress/compare-eq-on-null-and-undefined.js: Added.
(string_appeared_here.compareConstants):
(opaqueNull):
(opaqueUndefined):
(compareConstantsAndDynamicValues):
(compareDynamicValues):
(compareDynamicValueToItself):
(arrayTesting):
(opaqueCompare1):
(testNullComparatorUpdate):
(opaqueCompare2):
(testUndefinedComparatorUpdate):
(opaqueCompare3):
(testNullAndUndefinedComparatorUpdate):

LayoutTests:

* js/dom/document-all-watchpoint-covers-eliminated-compare-eq-expected.txt: Added.
* js/dom/document-all-watchpoint-covers-eliminated-compare-eq.html: Added.
* js/dom/script-tests/document-all-watchpoint-covers-eliminated-compare-eq.js: Added.
(compareFunction):


Canonical link: https://commits.webkit.org/166282@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@188624 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
Benjamin Poulain authored and BenjaminPoulain committed Aug 19, 2015
1 parent 7ba605f commit ab82b54
Show file tree
Hide file tree
Showing 26 changed files with 649 additions and 162 deletions.
12 changes: 12 additions & 0 deletions LayoutTests/ChangeLog
@@ -1,3 +1,15 @@
2015-08-18 Benjamin Poulain <bpoulain@apple.com>

[JSC] Optimize more cases of something-compared-to-null/undefined
https://bugs.webkit.org/show_bug.cgi?id=148157

Reviewed by Geoffrey Garen and Filip Pizlo.

* js/dom/document-all-watchpoint-covers-eliminated-compare-eq-expected.txt: Added.
* js/dom/document-all-watchpoint-covers-eliminated-compare-eq.html: Added.
* js/dom/script-tests/document-all-watchpoint-covers-eliminated-compare-eq.js: Added.
(compareFunction):

2015-08-18 Wenson Hsieh <wenson_hsieh@apple.com>

Attempt to fix the failing search-padding-cancel-results-buttons.html test by making
Expand Down
@@ -0,0 +1,12 @@
Test to make sure that document.all works correctly with elminated CompareEq in DFG.

On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".


PASS documentAllCompare.isNull is true
PASS documentAllCompare.isUndefined is true
PASS documentAllCompare.length is 13
PASS successfullyParsed is true

TEST COMPLETE

@@ -0,0 +1,10 @@
<!DOCTYPE HTML>
<html>
<head>
<script src="../../resources/js-test-pre.js"></script>
</head>
<body>
<script src="script-tests/document-all-watchpoint-covers-eliminated-compare-eq.js"></script>
<script src="../../resources/js-test-post.js"></script>
</body>
</html>
@@ -0,0 +1,49 @@
description("Test to make sure that document.all works correctly with elminated CompareEq in DFG.");

function compareFunction(a)
{
var length = a.length;

var aIsNull = (a == null) || (null == a);
var aIsUndefined = (a == undefined) || (undefined == a);

if (a == null || undefined == a)
return { isNull: aIsNull, isUndefined: aIsUndefined, length: length };
else
return { isNull: aIsNull, isUndefined: aIsUndefined };
}

// Warmup with sane objects.
for (let i = 0; i < 1e4; ++i) {
let result = compareFunction({ length: 5});
if (result.isNull || result.isUndefined)
debug("Failed warmup with compareFunction({ length: 5}).");

let object = new Object;
object.length = 1;
result = compareFunction(object);
if (result.isNull || result.isUndefined)
debug("Failed warmup with compareFunction(object).");
}

let documentAll = document.all;
var documentAllCompare = compareFunction(documentAll);
shouldBeTrue("documentAllCompare.isNull");
shouldBeTrue("documentAllCompare.isUndefined");
shouldBe("documentAllCompare.length", "13");

for (let i = 0; i < 1e3; ++i) {
let result = compareFunction({ length: 5});
if (result.isNull || result.isUndefined)
debug("Failed tail with compareFunction({ length: 5}).");

result = compareFunction(documentAll);
if (!result.isNull || !result.isUndefined)
debug("Failed tail with compareFunction(documentAll).");

let object = new Object;
object.length = 1;
result = compareFunction(object);
if (result.isNull || result.isUndefined)
debug("Failed tail with compareFunction(object).");
}
102 changes: 102 additions & 0 deletions Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,105 @@
2015-08-18 Benjamin Poulain <bpoulain@apple.com>

[JSC] Optimize more cases of something-compared-to-null/undefined
https://bugs.webkit.org/show_bug.cgi?id=148157

Reviewed by Geoffrey Garen and Filip Pizlo.

CompareEq is fairly trivial if you assert one of the operands is either
null or undefined. Under those conditions, the only way to have "true"
is to have the other operand be null/undefined or have an object
that masquerades to undefined.

JSC already had a fast path in CompareEqConstant.
With this patch, I generalize this fast path to more cases and try
to eliminate the checks whenever possible.

CompareEq now does the job of CompareEqConstant. If any operand can
be proved to be undefined/other, its edge is set to OtherUse. Whenever
any edge is OtherUse, we generate the fast code we had for CompareEqConstant.

The AbstractInterpreter has additional checks to reduce the node to a constant
whenever possible.

There are two additional changes in this patch:
-The Fixup Phase tries to set edges to OtherUse early. This is done correctly
in ConstantFoldingPhase but setting it up early helps the phases relying
on Clobberize.
-The codegen for CompareEqConstant was improved. The reason is the comparison
for ObjectOrOther could be faster just because the codegen was better.

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::parseBlock):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize): Deleted.
* dfg/DFGConstantFoldingPhase.cpp:
(JSC::DFG::ConstantFoldingPhase::foldConstants):
* dfg/DFGDoesGC.cpp:
(JSC::DFG::doesGC): Deleted.
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGNode.h:
(JSC::DFG::Node::isUndefinedOrNullConstant):
* dfg/DFGNodeType.h:
* dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::propagate): Deleted.
* dfg/DFGSafeToExecute.h:
(JSC::DFG::safeToExecute): Deleted.
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compilePeepHoleBranch):
(JSC::DFG::SpeculativeJIT::compare):
* dfg/DFGSpeculativeJIT.h:
(JSC::DFG::SpeculativeJIT::isKnownNotOther):
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::nonSpeculativeNonPeepholeCompareNullOrUndefined):
(JSC::DFG::SpeculativeJIT::nonSpeculativePeepholeBranchNullOrUndefined):
(JSC::DFG::SpeculativeJIT::nonSpeculativeNonPeepholeCompareNull): Deleted.
(JSC::DFG::SpeculativeJIT::nonSpeculativePeepholeBranchNull): Deleted.
(JSC::DFG::SpeculativeJIT::nonSpeculativeCompareNull): Deleted.
(JSC::DFG::SpeculativeJIT::compile): Deleted.
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::nonSpeculativeNonPeepholeCompareNullOrUndefined):
(JSC::DFG::SpeculativeJIT::nonSpeculativePeepholeBranchNullOrUndefined):
(JSC::DFG::SpeculativeJIT::nonSpeculativeNonPeepholeCompareNull): Deleted.
(JSC::DFG::SpeculativeJIT::nonSpeculativePeepholeBranchNull): Deleted.
(JSC::DFG::SpeculativeJIT::nonSpeculativeCompareNull): Deleted.
(JSC::DFG::SpeculativeJIT::compile): Deleted.
* dfg/DFGValidate.cpp:
(JSC::DFG::Validate::validate): Deleted.
* dfg/DFGWatchpointCollectionPhase.cpp:
(JSC::DFG::WatchpointCollectionPhase::handle):
* ftl/FTLCapabilities.cpp:
(JSC::FTL::canCompile):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::DFG::LowerDFGToLLVM::compileCompareEq):
(JSC::FTL::DFG::LowerDFGToLLVM::compileNode): Deleted.
(JSC::FTL::DFG::LowerDFGToLLVM::compileCompareEqConstant): Deleted.
* tests/stress/compare-eq-on-null-and-undefined-non-peephole.js: Added.
(string_appeared_here.useForMath):
(testUseForMath):
* tests/stress/compare-eq-on-null-and-undefined-optimized-in-constant-folding.js: Added.
(string_appeared_here.unreachableCodeTest):
(inlinedCompareToNull):
(inlinedComparedToUndefined):
(warmupInlineFunctions):
(testInlineFunctions):
* tests/stress/compare-eq-on-null-and-undefined.js: Added.
(string_appeared_here.compareConstants):
(opaqueNull):
(opaqueUndefined):
(compareConstantsAndDynamicValues):
(compareDynamicValues):
(compareDynamicValueToItself):
(arrayTesting):
(opaqueCompare1):
(testNullComparatorUpdate):
(opaqueCompare2):
(testUndefinedComparatorUpdate):
(opaqueCompare3):
(testNullAndUndefinedComparatorUpdate):

2015-08-18 Yusuke Suzuki <utatane.tea@gmail.com>

Introduce non-user-observable Promise functions to use Promises internally
Expand Down
33 changes: 29 additions & 4 deletions Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Expand Up @@ -1139,8 +1139,7 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
case CompareLessEq:
case CompareGreater:
case CompareGreaterEq:
case CompareEq:
case CompareEqConstant: {
case CompareEq: {
JSValue leftConst = forNode(node->child1()).value();
JSValue rightConst = forNode(node->child2()).value();
if (leftConst && rightConst) {
Expand Down Expand Up @@ -1180,13 +1179,40 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
}
}

if (node->op() == CompareEqConstant || node->op() == CompareEq) {
if (node->op() == CompareEq) {
SpeculatedType leftType = forNode(node->child1()).m_type;
SpeculatedType rightType = forNode(node->child2()).m_type;
if (!valuesCouldBeEqual(leftType, rightType)) {
setConstant(node, jsBoolean(false));
break;
}

if (leftType == SpecOther)
std::swap(leftType, rightType);
if (rightType == SpecOther) {
// Undefined and Null are always equal when compared to eachother.
if (!(leftType & ~SpecOther)) {
setConstant(node, jsBoolean(true));
break;
}

// Any other type compared to Null or Undefined is always false
// as long as the MasqueradesAsUndefined watchpoint is valid.
//
// MasqueradesAsUndefined only matters for SpecObjectOther, other
// cases are always "false".
if (!(leftType & (SpecObjectOther | SpecOther))) {
setConstant(node, jsBoolean(false));
break;
}

if (!(leftType & SpecOther) && m_graph.masqueradesAsUndefinedWatchpointIsStillValid(node->origin.semantic)) {
JSGlobalObject* globalObject = m_graph.globalObjectFor(node->origin.semantic);
m_graph.watchpoints().addLazily(globalObject->masqueradesAsUndefinedWatchpoint());
setConstant(node, jsBoolean(false));
break;
}
}
}

if (node->child1() == node->child2()) {
Expand All @@ -1206,7 +1232,6 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
case CompareLessEq:
case CompareGreaterEq:
case CompareEq:
case CompareEqConstant:
setConstant(node, jsBoolean(true));
break;
default:
Expand Down
12 changes: 8 additions & 4 deletions Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Expand Up @@ -3356,7 +3356,8 @@ bool ByteCodeParser::parseBlock(unsigned limit)

case op_eq_null: {
Node* value = get(VirtualRegister(currentInstruction[2].u.operand));
set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(CompareEqConstant, value, addToGraph(JSConstant, OpInfo(m_constantNull))));
Node* nullConstant = addToGraph(JSConstant, OpInfo(m_constantNull));
set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(CompareEq, value, nullConstant));
NEXT_OPCODE(op_eq_null);
}

Expand All @@ -3376,7 +3377,8 @@ bool ByteCodeParser::parseBlock(unsigned limit)

case op_neq_null: {
Node* value = get(VirtualRegister(currentInstruction[2].u.operand));
set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(LogicalNot, addToGraph(CompareEqConstant, value, addToGraph(JSConstant, OpInfo(m_constantNull)))));
Node* nullConstant = addToGraph(JSConstant, OpInfo(m_constantNull));
set(VirtualRegister(currentInstruction[1].u.operand), addToGraph(LogicalNot, addToGraph(CompareEq, value, nullConstant)));
NEXT_OPCODE(op_neq_null);
}

Expand Down Expand Up @@ -3523,15 +3525,17 @@ bool ByteCodeParser::parseBlock(unsigned limit)
case op_jeq_null: {
unsigned relativeOffset = currentInstruction[2].u.operand;
Node* value = get(VirtualRegister(currentInstruction[1].u.operand));
Node* condition = addToGraph(CompareEqConstant, value, addToGraph(JSConstant, OpInfo(m_constantNull)));
Node* nullConstant = addToGraph(JSConstant, OpInfo(m_constantNull));
Node* condition = addToGraph(CompareEq, value, nullConstant);
addToGraph(Branch, OpInfo(branchData(m_currentIndex + relativeOffset, m_currentIndex + OPCODE_LENGTH(op_jeq_null))), condition);
LAST_OPCODE(op_jeq_null);
}

case op_jneq_null: {
unsigned relativeOffset = currentInstruction[2].u.operand;
Node* value = get(VirtualRegister(currentInstruction[1].u.operand));
Node* condition = addToGraph(CompareEqConstant, value, addToGraph(JSConstant, OpInfo(m_constantNull)));
Node* nullConstant = addToGraph(JSConstant, OpInfo(m_constantNull));
Node* condition = addToGraph(CompareEq, value, nullConstant);
addToGraph(Branch, OpInfo(branchData(m_currentIndex + OPCODE_LENGTH(op_jneq_null), m_currentIndex + relativeOffset)), condition);
LAST_OPCODE(op_jneq_null);
}
Expand Down
1 change: 0 additions & 1 deletion Source/JavaScriptCore/dfg/DFGClobberize.h
Expand Up @@ -143,7 +143,6 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
case SkipScope:
case StringCharCodeAt:
case StringFromCharCode:
case CompareEqConstant:
case CompareStrictEq:
case IsUndefined:
case IsBoolean:
Expand Down
8 changes: 8 additions & 0 deletions Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp
Expand Up @@ -97,6 +97,14 @@ class ConstantFoldingPhase : public Phase {
node->child1().setUseKind(BooleanUse);
break;
}

case CompareEq: {
if (!m_interpreter.needsTypeCheck(node->child1(), SpecOther))
node->child1().setUseKind(OtherUse);
if (!m_interpreter.needsTypeCheck(node->child2(), SpecOther))
node->child2().setUseKind(OtherUse);
break;
}

case CheckStructure:
case ArrayifyToStructure: {
Expand Down
1 change: 0 additions & 1 deletion Source/JavaScriptCore/dfg/DFGDoesGC.cpp
Expand Up @@ -116,7 +116,6 @@ bool doesGC(Graph& graph, Node* node)
case CompareGreater:
case CompareGreaterEq:
case CompareEq:
case CompareEqConstant:
case CompareStrictEq:
case Call:
case Construct:
Expand Down
31 changes: 27 additions & 4 deletions Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Expand Up @@ -368,10 +368,6 @@ class FixupPhase : public Phase {
fixEdge<StringUse>(node->child1());
break;
}

case CompareEqConstant: {
break;
}

case CompareEq:
case CompareLess:
Expand Down Expand Up @@ -424,6 +420,32 @@ class FixupPhase : public Phase {
node->clearFlags(NodeMustGenerate);
break;
}

// If either child can be proved to be Null or Undefined, comparing them is greatly simplified.
bool oneArgumentIsUsedAsSpecOther = false;
if (node->child1()->isUndefinedOrNullConstant()) {
fixEdge<OtherUse>(node->child1());
oneArgumentIsUsedAsSpecOther = true;
} else if (node->child1()->shouldSpeculateOther()) {
m_insertionSet.insertNode(m_indexInBlock, SpecNone, Check, node->origin,
Edge(node->child1().node(), OtherUse));
fixEdge<OtherUse>(node->child1());
oneArgumentIsUsedAsSpecOther = true;
}
if (node->child2()->isUndefinedOrNullConstant()) {
fixEdge<OtherUse>(node->child2());
oneArgumentIsUsedAsSpecOther = true;
} else if (node->child2()->shouldSpeculateOther()) {
m_insertionSet.insertNode(m_indexInBlock, SpecNone, Check, node->origin,
Edge(node->child2().node(), OtherUse));
fixEdge<OtherUse>(node->child2());
oneArgumentIsUsedAsSpecOther = true;
}
if (oneArgumentIsUsedAsSpecOther) {
node->clearFlags(NodeMustGenerate);
break;
}

if (node->child1()->shouldSpeculateObject() && node->child2()->shouldSpeculateObjectOrOther()) {
fixEdge<ObjectUse>(node->child1());
fixEdge<ObjectOrOtherUse>(node->child2());
Expand All @@ -436,6 +458,7 @@ class FixupPhase : public Phase {
node->clearFlags(NodeMustGenerate);
break;
}

break;
}

Expand Down
7 changes: 6 additions & 1 deletion Source/JavaScriptCore/dfg/DFGNode.h
Expand Up @@ -690,7 +690,12 @@ struct Node {
{
return constant()->value().asBoolean();
}


bool isUndefinedOrNullConstant()
{
return isConstant() && constant()->value().isUndefinedOrNull();
}

bool isCellConstant()
{
return isConstant() && constant()->value() && constant()->value().isCell();
Expand Down

0 comments on commit ab82b54

Please sign in to comment.