Get rid of forward exit on DoubleAsInt32
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Dec 2013 06:50:19 +0000 (06:50 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 11 Dec 2013 06:50:19 +0000 (06:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=125552

PerformanceTests/SunSpider:

Reviewed by Oliver Hunt.

Use SunSpider as a kind of spot-check for the
no-architecture-specific-optimization paths in the compiler.

* no-architecture-specific-optimizations.yaml: Added.

Source/JavaScriptCore:

Reviewed by Oliver Hunt.

The forward exit was just there so that we wouldn't have to keep the inputs alive up to
the DoubleAsInt32. That's dumb. Forward exits are a complicated piece of machinery and
we shouldn't have it just for a bit of liveness micro-optimization.

Also add a bunch of machinery to test this case on X86.

* assembler/AbstractMacroAssembler.h:
(JSC::optimizeForARMv7s):
(JSC::optimizeForARM64):
(JSC::optimizeForX86):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGNodeType.h:
* dfg/DFGSpeculativeJIT.cpp:
(JSC::DFG::SpeculativeJIT::compileDoubleAsInt32):
* runtime/Options.h:
* tests/stress/double-as-int32.js: Added.
(foo):
(test):

Tools:

Reviewed by Oliver Hunt.

Add some support for testing the generic (non-X86) paths on X86 by disabling
architecture-specific optimizations (ASO's).

* Scripts/run-javascriptcore-tests:
* Scripts/run-jsc-stress-tests:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@160411 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
PerformanceTests/SunSpider/ChangeLog
PerformanceTests/SunSpider/no-architecture-specific-optimizations.yaml [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/assembler/AbstractMacroAssembler.h
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGNodeType.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/runtime/Options.h
Source/JavaScriptCore/tests/stress/double-as-int32.js [new file with mode: 0644]
Tools/ChangeLog
Tools/Scripts/run-javascriptcore-tests
Tools/Scripts/run-jsc-stress-tests

index 4f61b11..50df8d7 100644 (file)
@@ -1,3 +1,15 @@
+2013-12-10  Filip Pizlo  <fpizlo@apple.com>
+
+        Get rid of forward exit on DoubleAsInt32
+        https://bugs.webkit.org/show_bug.cgi?id=125552
+
+        Reviewed by Oliver Hunt.
+        
+        Use SunSpider as a kind of spot-check for the
+        no-architecture-specific-optimization paths in the compiler.
+
+        * no-architecture-specific-optimizations.yaml: Added.
+
 2013-10-08  Geoffrey Garen  <ggaren@apple.com>
 
         Refined power management in SunSpider 1.0.2
diff --git a/PerformanceTests/SunSpider/no-architecture-specific-optimizations.yaml b/PerformanceTests/SunSpider/no-architecture-specific-optimizations.yaml
new file mode 100644 (file)
index 0000000..92d8a13
--- /dev/null
@@ -0,0 +1,30 @@
+# Copyright (C) 2013 Apple Inc. All rights reserved.
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions
+# are met:
+#
+# 1.  Redistributions of source code must retain the above copyright
+#     notice, this list of conditions and the following disclaimer. 
+# 2.  Redistributions in binary form must reproduce the above copyright
+#     notice, this list of conditions and the following disclaimer in the
+#     documentation and/or other materials provided with the distribution. 
+#
+# THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
+# EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
+# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+# DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
+# DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
+# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
+# THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+# Tests SunSpider with architecture-specific optimizations disabled. This is
+# just here so you can't get away with murder when dealing with the generic
+# (i.e. no ASO) paths. You should still likely actually *test* all of the
+# architectures you care about.
+
+- path: tests/sunspider-1.0
+  cmd: runNoCJITNoASO
index 037c406..1b86955 100644 (file)
@@ -1,5 +1,32 @@
 2013-12-10  Filip Pizlo  <fpizlo@apple.com>
 
+        Get rid of forward exit on DoubleAsInt32
+        https://bugs.webkit.org/show_bug.cgi?id=125552
+
+        Reviewed by Oliver Hunt.
+        
+        The forward exit was just there so that we wouldn't have to keep the inputs alive up to
+        the DoubleAsInt32. That's dumb. Forward exits are a complicated piece of machinery and
+        we shouldn't have it just for a bit of liveness micro-optimization.
+        
+        Also add a bunch of machinery to test this case on X86.
+
+        * assembler/AbstractMacroAssembler.h:
+        (JSC::optimizeForARMv7s):
+        (JSC::optimizeForARM64):
+        (JSC::optimizeForX86):
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        * dfg/DFGNodeType.h:
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileDoubleAsInt32):
+        * runtime/Options.h:
+        * tests/stress/double-as-int32.js: Added.
+        (foo):
+        (test):
+
+2013-12-10  Filip Pizlo  <fpizlo@apple.com>
+
         Simplify CSE's treatment of NodeRelevantToOSR
         https://bugs.webkit.org/show_bug.cgi?id=125538
 
index a0b7b73..2cb452b 100644 (file)
@@ -29,6 +29,7 @@
 #include "AssemblerBuffer.h"
 #include "CodeLocation.h"
 #include "MacroAssemblerCodeRef.h"
+#include "Options.h"
 #include <wtf/CryptographicallyRandomNumber.h>
 #include <wtf/Noncopyable.h>
 
@@ -63,6 +64,21 @@ inline bool isX86()
 #endif
 }
 
+inline bool optimizeForARMv7s()
+{
+    return isARMv7s() && Options::enableArchitectureSpecificOptimizations();
+}
+
+inline bool optimizeForARM64()
+{
+    return isARM64() && Options::enableArchitectureSpecificOptimizations();
+}
+
+inline bool optimizeForX86()
+{
+    return isX86() && Options::enableArchitectureSpecificOptimizations();
+}
+
 class LinkBuffer;
 class RepatchBuffer;
 class Watchpoint;
index 77076df..d792064 100644 (file)
@@ -233,11 +233,14 @@ private:
         case ArithMod: {
             if (Node::shouldSpeculateInt32ForArithmetic(node->child1().node(), node->child2().node())
                 && node->canSpeculateInt32()) {
-                if (isX86() || isARM64() || isARMv7s()) {
+                if (optimizeForX86() || optimizeForARM64() || optimizeForARMv7s()) {
                     fixEdge<Int32Use>(node->child1());
                     fixEdge<Int32Use>(node->child2());
                     break;
                 }
+                Edge child1 = node->child1();
+                Edge child2 = node->child2();
+                
                 injectInt32ToDoubleNode(node->child1());
                 injectInt32ToDoubleNode(node->child2());
 
@@ -248,6 +251,8 @@ private:
                 
                 node->setOp(DoubleAsInt32);
                 node->children.initialize(Edge(newDivision, KnownNumberUse), Edge(), Edge());
+                
+                m_insertionSet.insertNode(m_indexInBlock + 1, SpecNone, Phantom, node->codeOrigin, child1, child2);
                 break;
             }
             fixEdge<NumberUse>(node->child1());
index 131edfc..1d862f5 100644 (file)
@@ -111,7 +111,7 @@ namespace JSC { namespace DFG {
     /* of the value from the integer form. */\
     macro(Int32ToDouble, NodeResultNumber) \
     /* Used to speculate that a double value is actually an integer. */\
-    macro(DoubleAsInt32, NodeResultInt32 | NodeExitsForward) \
+    macro(DoubleAsInt32, NodeResultInt32) \
     /* Used to separate representation and register allocation of Int52's represented */\
     /* as values. */\
     macro(Int52ToValue, NodeResultJS) \
index 617acaa..24c403e 100644 (file)
@@ -2188,7 +2188,7 @@ void SpeculativeJIT::compileDoubleAsInt32(Node* node)
     JITCompiler::JumpList failureCases;
     bool negZeroCheck = !bytecodeCanIgnoreNegativeZero(node->arithNodeFlags());
     m_jit.branchConvertDoubleToInt32(valueFPR, resultGPR, failureCases, scratchFPR, negZeroCheck);
-    forwardSpeculationCheck(Overflow, JSValueRegs(), 0, failureCases, ValueRecovery::inFPR(valueFPR));
+    speculationCheck(Overflow, JSValueRegs(), 0, failureCases);
 
     int32Result(resultGPR, node);
 }
index 4aaa80e..c88b3ac 100644 (file)
@@ -144,6 +144,8 @@ typedef OptionRange optionRange;
     v(bool, forceUDis86Disassembler, false) \
     v(bool, forceLLVMDisassembler, false) \
     \
+    v(bool, enableArchitectureSpecificOptimizations, true) \
+    \
     v(unsigned, maximumOptimizationCandidateInstructionCount, 10000) \
     \
     v(unsigned, maximumFunctionForCallInlineCandidateInstructionCount, 180) \
diff --git a/Source/JavaScriptCore/tests/stress/double-as-int32.js b/Source/JavaScriptCore/tests/stress/double-as-int32.js
new file mode 100644 (file)
index 0000000..1939823
--- /dev/null
@@ -0,0 +1,22 @@
+//@ defaultRun; runNoCJITNoASO
+
+function foo(a, b) {
+    return a.f / b.f;
+}
+
+noInline(foo);
+
+function test(a, b, e) {
+    var result = foo({f:a}, {f:b});
+    if (result != e)
+        throw "Error: " + a + " / " + b + " should be " + e + " but was " + result;
+}
+
+for (var i = 1; i < 101; ++i)
+    test(i * 2, i, 2);
+
+test(9, 3, 3);
+test(12, 4, 3);
+test(-32, 8, -4);
+test(-21, 7, -3);
+test(7, 2, 3.5);
index cffbfaf..84f15a0 100644 (file)
@@ -1,3 +1,16 @@
+2013-12-10  Filip Pizlo  <fpizlo@apple.com>
+
+        Get rid of forward exit on DoubleAsInt32
+        https://bugs.webkit.org/show_bug.cgi?id=125552
+
+        Reviewed by Oliver Hunt.
+        
+        Add some support for testing the generic (non-X86) paths on X86 by disabling
+        architecture-specific optimizations (ASO's).
+
+        * Scripts/run-javascriptcore-tests:
+        * Scripts/run-jsc-stress-tests:
+
 2013-12-10  Ryuan Choi  <ryuan.choi@samsung.com>
 
         Unreviewed EFL build fix attempt
index 8b9231f..f620f67 100755 (executable)
@@ -237,6 +237,7 @@ if ($runJSCStress) {
         "/usr/bin/env", "ruby", "Tools/Scripts/run-jsc-stress-tests",
         "-j", jscPath($productDir), "-o", $jscStressResultsDir,
         "PerformanceTests/SunSpider/tests/sunspider-1.0",
+        "PerformanceTests/SunSpider/no-architecture-specific-optimizations.yaml",
         "PerformanceTests/SunSpider/tests/v8-v6",
         "Source/JavaScriptCore/tests/mozilla/mozilla-tests.yaml",
         "Source/JavaScriptCore/tests/stress",
index 83f88ac..00cf86e 100755 (executable)
@@ -563,6 +563,10 @@ def runAlwaysTriggerCopyPhase
     run("always-trigger-copy-phase", "--minHeapUtilization=2.0", "--minCopiedBlockUtilization=2.0")
 end
 
+def runNoCJITNoASO
+    run("no-cjit-no-aso", "--enableConcurrentJIT=false", "--enableArchitectureSpecificOptimizations=false")
+end
+
 def defaultRun
     runDefault
     runNoLLInt