[webkit-changes] [200502] trunk/Source/JavaScriptCore

Thu, 05 May 2016 19:31:12 -0700

Title: [200502] trunk/Source/_javascript_Core
Revision
200502
Author
[email protected]
Date
2016-05-05 19:30:51 -0700 (Thu, 05 May 2016)

Log Message

[JSC] Get rid of NonNegZeroDouble, it is broken
https://bugs.webkit.org/show_bug.cgi?id=157399
rdar://problem/25339647

Patch by Benjamin Poulain <[email protected]> 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.

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (200501 => 200502)


--- trunk/Source/_javascript_Core/ChangeLog	2016-05-06 01:26:26 UTC (rev 200501)
+++ trunk/Source/_javascript_Core/ChangeLog	2016-05-06 02:30:51 UTC (rev 200502)
@@ -1,3 +1,57 @@
+2016-05-05  Benjamin Poulain  <[email protected]>
+
+        [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  <[email protected]>
 
         REGRESSION(r200422): Web Inspector: Make new Array Iterator objects play nice with Web Inspector

Modified: trunk/Source/_javascript_Core/bytecode/ValueProfile.cpp (200501 => 200502)


--- trunk/Source/_javascript_Core/bytecode/ValueProfile.cpp	2016-05-06 01:26:26 UTC (rev 200501)
+++ trunk/Source/_javascript_Core/bytecode/ValueProfile.cpp	2016-05-06 02:30:51 UTC (rev 200502)
@@ -34,31 +34,23 @@
 {
     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());

Modified: trunk/Source/_javascript_Core/bytecode/ValueProfile.h (200501 => 200502)


--- trunk/Source/_javascript_Core/bytecode/ValueProfile.h	2016-05-06 01:26:26 UTC (rev 200501)
+++ trunk/Source/_javascript_Core/bytecode/ValueProfile.h	2016-05-06 02:30:51 UTC (rev 200502)
@@ -218,25 +218,20 @@
     }
 
     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); }

Modified: trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp (200501 => 200502)


--- trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp	2016-05-06 01:26:26 UTC (rev 200501)
+++ trunk/Source/_javascript_Core/dfg/DFGByteCodeParser.cpp	2016-05-06 02:30:51 UTC (rev 200502)
@@ -934,8 +934,6 @@
                 node->mergeFlags(NodeMayOverflowInt32InBaseline);
             if (resultProfile.didObserveNegZeroDouble() || m_inlineStackTop->m_exitProfile.hasExitSite(m_currentIndex, NegativeZero))
                 node->mergeFlags(NodeMayNegZeroInBaseline);
-            if (resultProfile.didObserveNonInt32())
-                node->mergeFlags(NodeMayHaveNonIntResult);
             break;
         }

Modified: trunk/Source/_javascript_Core/dfg/DFGNode.h (200501 => 200502)


--- trunk/Source/_javascript_Core/dfg/DFGNode.h	2016-05-06 01:26:26 UTC (rev 200501)
+++ trunk/Source/_javascript_Core/dfg/DFGNode.h	2016-05-06 02:30:51 UTC (rev 200502)
@@ -950,11 +950,6 @@
         return result & ~NodeBytecodeNeedsNegZero;
     }
 
-    bool mayHaveNonIntResult()
-    {
-        return m_flags & NodeMayHaveNonIntResult;
-    }
-
     bool hasConstantBuffer()
     {
         return op() == NewArrayBuffer;

Modified: trunk/Source/_javascript_Core/dfg/DFGNodeFlags.cpp (200501 => 200502)


--- trunk/Source/_javascript_Core/dfg/DFGNodeFlags.cpp	2016-05-06 01:26:26 UTC (rev 200501)
+++ trunk/Source/_javascript_Core/dfg/DFGNodeFlags.cpp	2016-05-06 02:30:51 UTC (rev 200502)
@@ -85,9 +85,6 @@
             out.print(comma, "UseAsOther");
     }
 
-    if (flags & NodeMayHaveNonIntResult)
-        out.print(comma, "MayHaveNonIntResult");
-
     if (flags & NodeMayOverflowInt52)
         out.print(comma, "MayOverflowInt52");

Modified: trunk/Source/_javascript_Core/dfg/DFGNodeFlags.h (200501 => 200502)


--- trunk/Source/_javascript_Core/dfg/DFGNodeFlags.h	2016-05-06 01:26:26 UTC (rev 200501)
+++ trunk/Source/_javascript_Core/dfg/DFGNodeFlags.h	2016-05-06 02:30:51 UTC (rev 200502)
@@ -48,7 +48,6 @@
 #define NodeHasVarArgs                   0x0010
     
 #define NodeBehaviorMask                 0x07e0
-#define NodeMayHaveNonIntResult          0x0020
 #define NodeMayOverflowInt52             0x0040
 #define NodeMayOverflowInt32InBaseline   0x0080
 #define NodeMayOverflowInt32InDFG        0x0100

Modified: trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp (200501 => 200502)


--- trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp	2016-05-06 01:26:26 UTC (rev 200501)
+++ trunk/Source/_javascript_Core/dfg/DFGPredictionPropagationPhase.cpp	2016-05-06 02:30:51 UTC (rev 200502)
@@ -275,12 +275,8 @@
                         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;
         }

Modified: trunk/Source/_javascript_Core/jit/JITMulGenerator.cpp (200501 => 200502)


--- trunk/Source/_javascript_Core/jit/JITMulGenerator.cpp	2016-05-06 01:26:26 UTC (rev 200501)
+++ trunk/Source/_javascript_Core/jit/JITMulGenerator.cpp	2016-05-06 02:30:51 UTC (rev 200502)
@@ -153,7 +153,6 @@
         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 @@
         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);

Modified: trunk/Source/_javascript_Core/runtime/CommonSlowPaths.cpp (200501 => 200502)


--- trunk/Source/_javascript_Core/runtime/CommonSlowPaths.cpp	2016-05-06 01:26:26 UTC (rev 200501)
+++ trunk/Source/_javascript_Core/runtime/CommonSlowPaths.cpp	2016-05-06 02:30:51 UTC (rev 200502)
@@ -378,8 +378,6 @@
             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.

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to