[JSC] Get rid of NonNegZeroDouble, it is broken
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 6 May 2016 02:30:51 +0000 (02:30 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 6 May 2016 02:30:51 +0000 (02:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=157399
rdar://problem/25339647

Patch by Benjamin Poulain <bpoulain@apple.com> on 2016-05-05
Reviewed by Mark Lam.

The profile "NonNegZeroDouble" is fundamentally broken.

It is used by DFG to predict the result of ArithMul as being a Double
or Int32.
The problem is you are likely to mispredict, and when you do, you are
guaranteed to end up in a recompile loop.

The compile loops usually happen like this:
-We speculate you have Int32 despite producing doubles.
-We OSR exit on another node (ValueToInt32 for example) from the result of this ArithMul.
-When we compile this block again, ArithMul will do the same misprediction
 because it unconditionally predicts Int32.

The flag NonNegZeroDouble was very unlikely to be set correctly
in the first place.

In LLINT, the flag is only set on the slow path.
Since double*double is on the fast path, those cases are ignored.

In Baseline, the flag is set for any case that falls back on double
multiplication. BUT, the DFG flag was only set for nodes that spend
many iteration in slow path, which obviously does not apply to double*double.

Given the perf drawbacks and the recompile loops, I removed
the whole flag for now.

* bytecode/ValueProfile.cpp:
(WTF::printInternal):
* bytecode/ValueProfile.h:
(JSC::ResultProfile::didObserveNonInt32): Deleted.
(JSC::ResultProfile::didObserveDouble): Deleted.
(JSC::ResultProfile::didObserveNonNegZeroDouble): Deleted.
(JSC::ResultProfile::setObservedNonNegZeroDouble): Deleted.
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::makeSafe): Deleted.
* dfg/DFGNode.h:
(JSC::DFG::Node::mayHaveNonIntResult): Deleted.
* dfg/DFGNodeFlags.cpp:
(JSC::DFG::dumpNodeFlags): Deleted.
* dfg/DFGNodeFlags.h:
* dfg/DFGPredictionPropagationPhase.cpp:
* jit/JITMulGenerator.cpp:
(JSC::JITMulGenerator::generateFastPath): Deleted.
* runtime/CommonSlowPaths.cpp:
(JSC::updateResultProfileForBinaryArithOp): Deleted.

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/ValueProfile.cpp
Source/JavaScriptCore/bytecode/ValueProfile.h
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGNode.h
Source/JavaScriptCore/dfg/DFGNodeFlags.cpp
Source/JavaScriptCore/dfg/DFGNodeFlags.h
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
Source/JavaScriptCore/jit/JITMulGenerator.cpp
Source/JavaScriptCore/runtime/CommonSlowPaths.cpp

index 8696d76..373a522 100644 (file)
@@ -1,3 +1,57 @@
+2016-05-05  Benjamin Poulain  <bpoulain@apple.com>
+
+        [JSC] Get rid of NonNegZeroDouble, it is broken
+        https://bugs.webkit.org/show_bug.cgi?id=157399
+        rdar://problem/25339647
+
+        Reviewed by Mark Lam.
+
+        The profile "NonNegZeroDouble" is fundamentally broken.
+
+        It is used by DFG to predict the result of ArithMul as being a Double
+        or Int32.
+        The problem is you are likely to mispredict, and when you do, you are
+        guaranteed to end up in a recompile loop.
+
+        The compile loops usually happen like this:
+        -We speculate you have Int32 despite producing doubles.
+        -We OSR exit on another node (ValueToInt32 for example) from the result of this ArithMul.
+        -When we compile this block again, ArithMul will do the same misprediction
+         because it unconditionally predicts Int32.
+
+        The flag NonNegZeroDouble was very unlikely to be set correctly
+        in the first place.
+
+        In LLINT, the flag is only set on the slow path.
+        Since double*double is on the fast path, those cases are ignored.
+
+        In Baseline, the flag is set for any case that falls back on double
+        multiplication. BUT, the DFG flag was only set for nodes that spend
+        many iteration in slow path, which obviously does not apply to double*double.
+
+        Given the perf drawbacks and the recompile loops, I removed
+        the whole flag for now.
+
+        * bytecode/ValueProfile.cpp:
+        (WTF::printInternal):
+        * bytecode/ValueProfile.h:
+        (JSC::ResultProfile::didObserveNonInt32): Deleted.
+        (JSC::ResultProfile::didObserveDouble): Deleted.
+        (JSC::ResultProfile::didObserveNonNegZeroDouble): Deleted.
+        (JSC::ResultProfile::setObservedNonNegZeroDouble): Deleted.
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::makeSafe): Deleted.
+        * dfg/DFGNode.h:
+        (JSC::DFG::Node::mayHaveNonIntResult): Deleted.
+        * dfg/DFGNodeFlags.cpp:
+        (JSC::DFG::dumpNodeFlags): Deleted.
+        * dfg/DFGNodeFlags.h:
+        * dfg/DFGPredictionPropagationPhase.cpp:
+        * jit/JITMulGenerator.cpp:
+        (JSC::JITMulGenerator::generateFastPath): Deleted.
+        * runtime/CommonSlowPaths.cpp:
+        (JSC::updateResultProfileForBinaryArithOp): Deleted.
+
 2016-05-05  Joseph Pecoraro  <pecoraro@apple.com>
 
         REGRESSION(r200422): Web Inspector: Make new Array Iterator objects play nice with Web Inspector
index 876ce30..f046a9d 100644 (file)
@@ -34,31 +34,23 @@ void printInternal(PrintStream& out, const ResultProfile& profile)
 {
     const char* separator = "";
 
-    if (!profile.didObserveNonInt32()) {
-        out.print("Int32");
+    if (profile.didObserveNegZeroDouble()) {
+        out.print(separator, "NegZeroDouble");
         separator = "|";
-    } else {
-        if (profile.didObserveNegZeroDouble()) {
-            out.print(separator, "NegZeroDouble");
-            separator = "|";
-        }
-        if (profile.didObserveNonNegZeroDouble()) {
-            out.print("NonNegZeroDouble");
-            separator = "|";
-        }
-        if (profile.didObserveNonNumber()) {
-            out.print("NonNumber");
-            separator = "|";
-        }
-        if (profile.didObserveInt32Overflow()) {
-            out.print("Int32Overflow");
-            separator = "|";
-        }
-        if (profile.didObserveInt52Overflow()) {
-            out.print("Int52Overflow");
-            separator = "|";
-        }
     }
+    if (profile.didObserveNonNumber()) {
+        out.print("NonNumber");
+        separator = "|";
+    }
+    if (profile.didObserveInt32Overflow()) {
+        out.print("Int32Overflow");
+        separator = "|";
+    }
+    if (profile.didObserveInt52Overflow()) {
+        out.print("Int52Overflow");
+        separator = "|";
+    }
+
     if (profile.specialFastPathCount()) {
         out.print(" special fast path: ");
         out.print(profile.specialFastPathCount());
index 48b47da..1fac6b4 100644 (file)
@@ -218,25 +218,20 @@ public:
     }
 
     enum ObservedResults {
-        NonNegZeroDouble = 1 << 0,
-        NegZeroDouble    = 1 << 1,
-        NonNumber        = 1 << 2,
-        Int32Overflow    = 1 << 3,
-        Int52Overflow    = 1 << 4,
+        NegZeroDouble    = 1 << 0,
+        NonNumber        = 1 << 1,
+        Int32Overflow    = 1 << 2,
+        Int52Overflow    = 1 << 3,
     };
 
     int bytecodeOffset() const { return m_bytecodeOffsetAndFlags >> numberOfFlagBits; }
     unsigned specialFastPathCount() const { return m_specialFastPathCount; }
 
-    bool didObserveNonInt32() const { return hasBits(NonNegZeroDouble | NegZeroDouble | NonNumber); }
-    bool didObserveDouble() const { return hasBits(NonNegZeroDouble | NegZeroDouble); }
-    bool didObserveNonNegZeroDouble() const { return hasBits(NonNegZeroDouble); }
     bool didObserveNegZeroDouble() const { return hasBits(NegZeroDouble); }
     bool didObserveNonNumber() const { return hasBits(NonNumber); }
     bool didObserveInt32Overflow() const { return hasBits(Int32Overflow); }
     bool didObserveInt52Overflow() const { return hasBits(Int52Overflow); }
 
-    void setObservedNonNegZeroDouble() { setBit(NonNegZeroDouble); }
     void setObservedNegZeroDouble() { setBit(NegZeroDouble); }
     void setObservedNonNumber() { setBit(NonNumber); }
     void setObservedInt32Overflow() { setBit(Int32Overflow); }
index 6c11941..c21afa1 100644 (file)
@@ -934,8 +934,6 @@ private:
                 node->mergeFlags(NodeMayOverflowInt32InBaseline);
             if (resultProfile.didObserveNegZeroDouble() || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, NegativeZero))
                 node->mergeFlags(NodeMayNegZeroInBaseline);
-            if (resultProfile.didObserveNonInt32())
-                node->mergeFlags(NodeMayHaveNonIntResult);
             break;
         }
 
index 4b7fa9b..4fb694c 100644 (file)
@@ -950,11 +950,6 @@ struct Node {
         return result & ~NodeBytecodeNeedsNegZero;
     }
 
-    bool mayHaveNonIntResult()
-    {
-        return m_flags & NodeMayHaveNonIntResult;
-    }
-
     bool hasConstantBuffer()
     {
         return op() == NewArrayBuffer;
index 79f4b43..4894242 100644 (file)
@@ -85,9 +85,6 @@ void dumpNodeFlags(PrintStream& actualOut, NodeFlags flags)
             out.print(comma, "UseAsOther");
     }
 
-    if (flags & NodeMayHaveNonIntResult)
-        out.print(comma, "MayHaveNonIntResult");
-
     if (flags & NodeMayOverflowInt52)
         out.print(comma, "MayOverflowInt52");
 
index 626b9bb..308641e 100644 (file)
@@ -48,7 +48,6 @@ namespace JSC { namespace DFG {
 #define NodeHasVarArgs                   0x0010
     
 #define NodeBehaviorMask                 0x07e0
-#define NodeMayHaveNonIntResult          0x0020
 #define NodeMayOverflowInt52             0x0040
 #define NodeMayOverflowInt32InBaseline   0x0080
 #define NodeMayOverflowInt32InDFG        0x0100
index 3b99272..b14dccb 100644 (file)
@@ -275,12 +275,8 @@ private:
                         changed |= mergePrediction(SpecInt52Only);
                     else
                         changed |= mergePrediction(speculatedDoubleTypeForPredictions(left, right));
-                } else {
-                    if (node->mayHaveNonIntResult())
-                        changed |= mergePrediction(SpecInt32Only | SpecBytecodeDouble);
-                    else
-                        changed |= mergePrediction(SpecInt32Only);
-                }
+                } else
+                    changed |= mergePrediction(SpecInt32Only | SpecBytecodeDouble);
             }
             break;
         }
index b1fb0b0..1552707 100644 (file)
@@ -153,7 +153,6 @@ void JITMulGenerator::generateFastPath(CCallHelpers& jit)
         CCallHelpers::Jump done = jit.jump();
 
         notNegativeZero.link(&jit);
-        jit.or32(CCallHelpers::TrustedImm32(ResultProfile::NonNegZeroDouble), CCallHelpers::AbsoluteAddress(m_resultProfile->addressOfFlags()));
 
         jit.move(m_result.payloadGPR(), m_scratchGPR);
         jit.urshiftPtr(CCallHelpers::Imm32(52), m_scratchGPR);
@@ -175,7 +174,6 @@ void JITMulGenerator::generateFastPath(CCallHelpers& jit)
         CCallHelpers::Jump done = jit.jump();
 
         notNegativeZero.link(&jit);
-        jit.or32(CCallHelpers::TrustedImm32(ResultProfile::NonNegZeroDouble), CCallHelpers::AbsoluteAddress(m_resultProfile->addressOfFlags()));
 
         jit.move(m_result.tagGPR(), m_scratchGPR);
         jit.urshiftPtr(CCallHelpers::Imm32(52 - 32), m_scratchGPR);
index ad8327a..11dd2e2 100644 (file)
@@ -378,8 +378,6 @@ static void updateResultProfileForBinaryArithOp(ExecState* exec, Instruction* pc
             if (!doubleVal && std::signbit(doubleVal))
                 profile->setObservedNegZeroDouble();
             else {
-                profile->setObservedNonNegZeroDouble();
-
                 // The Int52 overflow check here intentionally omits 1ll << 51 as a valid negative Int52 value.
                 // Therefore, we will get a false positive if the result is that value. This is intentionally
                 // done to simplify the checking algorithm.