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

Thu, 04 Jun 2020 12:55:09 -0700

Title: [262562] trunk/Source/_javascript_Core
Revision
262562
Author
mark....@apple.com
Date
2020-06-04 12:54:17 -0700 (Thu, 04 Jun 2020)

Log Message

Reduce DFGDoesGCCheck to only storing a uint32_t.
https://bugs.webkit.org/show_bug.cgi?id=212734

Reviewed by Saam Barati and Caio Lima.

This patch changes the encoding of DoesGCCheck so that it will fit better in a
uint32_t.  This has the following benefits:
1. speed improvement for debug builds because it now takes less instructions
   (especially in JITted code) to store to DoesGCCheck::m_value.
2. enables this check for 32-bit platforms as well.

Fun fact: we currently have 373 DFG::NodeTypes.  Hence, 9 bits for nodeOp.

The new encoding provides 21 bis for the nodeIndex.  This gives us up to 2097152
node indexes.  In my experience, I've never seen more than 3 decimal digits for
the nodeIndex so far.  If we ever find that we need more than 21 bits of nodeIndex,
we have 2 options to deal with it:

1. We can just ignore the high bits.  After all, it is the nodeOp that is the
   most interesting piece of data we need to debug doesGC issues.

2. We can make DoesGCCheck use uint64_t for storage.  This encoding automatically
   scales to 64-bit, while still allowing the more efficient form of storing a
   32-bit immediate to be used for the common cases.

This patch also makes ENABLE_DFG_DOES_GC_VALIDATION dependent on ENABLE(DFG_JIT).
DoesGC is only relevant for the DFG and FTL JITs.

* dfg/DFGDoesGCCheck.cpp:
(JSC::DFG::DoesGCCheck::verifyCanGC):
* dfg/DFGDoesGCCheck.h:
(JSC::DFG::DoesGCCheck::encode):
(JSC::DFG::DoesGCCheck::expectDoesGC const):
(JSC::DFG::DoesGCCheck::isSpecial const):
(JSC::DFG::DoesGCCheck::special):
(JSC::DFG::DoesGCCheck::nodeOp):
(JSC::DFG::DoesGCCheck::nodeIndex):
(JSC::DFG::DoesGCCheck::expectDoesGC): Deleted.
(JSC::DFG::DoesGCCheck::isSpecial): Deleted.
(JSC::DFG::DoesGCCheck::specialIndex): Deleted.
(JSC::DFG::DoesGCCheck::bits): Deleted.
* dfg/DFGNodeType.h:
* dfg/DFGOSRExit.cpp:
(JSC::DFG::OSRExit::compileExit):
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compileNode):
* ftl/FTLOSRExitCompiler.cpp:
(JSC::FTL::compileStub):
* heap/Heap.h:

Modified Paths

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (262561 => 262562)


--- trunk/Source/_javascript_Core/ChangeLog	2020-06-04 19:52:52 UTC (rev 262561)
+++ trunk/Source/_javascript_Core/ChangeLog	2020-06-04 19:54:17 UTC (rev 262562)
@@ -1,3 +1,59 @@
+2020-06-04  Mark Lam  <mark....@apple.com>
+
+        Reduce DFGDoesGCCheck to only storing a uint32_t.
+        https://bugs.webkit.org/show_bug.cgi?id=212734
+
+        Reviewed by Saam Barati and Caio Lima.
+
+        This patch changes the encoding of DoesGCCheck so that it will fit better in a
+        uint32_t.  This has the following benefits:
+        1. speed improvement for debug builds because it now takes less instructions
+           (especially in JITted code) to store to DoesGCCheck::m_value.
+        2. enables this check for 32-bit platforms as well.
+
+        Fun fact: we currently have 373 DFG::NodeTypes.  Hence, 9 bits for nodeOp.
+
+        The new encoding provides 21 bis for the nodeIndex.  This gives us up to 2097152
+        node indexes.  In my experience, I've never seen more than 3 decimal digits for
+        the nodeIndex so far.  If we ever find that we need more than 21 bits of nodeIndex,
+        we have 2 options to deal with it:
+
+        1. We can just ignore the high bits.  After all, it is the nodeOp that is the
+           most interesting piece of data we need to debug doesGC issues.
+
+        2. We can make DoesGCCheck use uint64_t for storage.  This encoding automatically
+           scales to 64-bit, while still allowing the more efficient form of storing a
+           32-bit immediate to be used for the common cases.
+
+        This patch also makes ENABLE_DFG_DOES_GC_VALIDATION dependent on ENABLE(DFG_JIT).
+        DoesGC is only relevant for the DFG and FTL JITs.
+
+        * dfg/DFGDoesGCCheck.cpp:
+        (JSC::DFG::DoesGCCheck::verifyCanGC):
+        * dfg/DFGDoesGCCheck.h:
+        (JSC::DFG::DoesGCCheck::encode):
+        (JSC::DFG::DoesGCCheck::expectDoesGC const):
+        (JSC::DFG::DoesGCCheck::isSpecial const):
+        (JSC::DFG::DoesGCCheck::special):
+        (JSC::DFG::DoesGCCheck::nodeOp):
+        (JSC::DFG::DoesGCCheck::nodeIndex):
+        (JSC::DFG::DoesGCCheck::expectDoesGC): Deleted.
+        (JSC::DFG::DoesGCCheck::isSpecial): Deleted.
+        (JSC::DFG::DoesGCCheck::specialIndex): Deleted.
+        (JSC::DFG::DoesGCCheck::bits): Deleted.
+        * dfg/DFGNodeType.h:
+        * dfg/DFGOSRExit.cpp:
+        (JSC::DFG::OSRExit::compileExit):
+        * dfg/DFGSpeculativeJIT32_64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileNode):
+        * ftl/FTLOSRExitCompiler.cpp:
+        (JSC::FTL::compileStub):
+        * heap/Heap.h:
+
 2020-06-04  Tim Horton  <timothy_hor...@apple.com>
 
         Work around broken system version macro

Modified: trunk/Source/_javascript_Core/dfg/DFGDoesGCCheck.cpp (262561 => 262562)


--- trunk/Source/_javascript_Core/dfg/DFGDoesGCCheck.cpp	2020-06-04 19:52:52 UTC (rev 262561)
+++ trunk/Source/_javascript_Core/dfg/DFGDoesGCCheck.cpp	2020-06-04 19:54:17 UTC (rev 262562)
@@ -28,6 +28,7 @@
 
 #include "CallFrame.h"
 #include "CodeBlock.h"
+#include "DFGNodeType.h"
 #include "Heap.h"
 #include "VMInspector.h"
 #include <wtf/DataLog.h>
@@ -41,6 +42,10 @@
 
 void DoesGCCheck::verifyCanGC(VM& vm)
 {
+    // We do this check here just so we don't have to #include DFGNodeType.h
+    // in the header file.
+    static_assert(numberOfNodeTypes <= (1 << nodeOpBits));
+
     if (!expectDoesGC()) {
         dataLog("Error: DoesGC failed");
         if (isSpecial()) {
@@ -53,7 +58,6 @@
             case Special::FTLOSRExit:
                 dataLog(" @ FTL osr exit");
                 break;
-            case Special::ReservedUnused:
             case Special::NumberOfSpecials:
                 RELEASE_ASSERT_NOT_REACHED();
             }

Modified: trunk/Source/_javascript_Core/dfg/DFGDoesGCCheck.h (262561 => 262562)


--- trunk/Source/_javascript_Core/dfg/DFGDoesGCCheck.h	2020-06-04 19:52:52 UTC (rev 262561)
+++ trunk/Source/_javascript_Core/dfg/DFGDoesGCCheck.h	2020-06-04 19:54:17 UTC (rev 262562)
@@ -36,7 +36,6 @@
 
 struct DoesGCCheck {
     enum class Special {
-        ReservedUnused, // so that specials start with 1. isSpecial() relies on this.
         Uninitialized,
         DFGOSRExit,
         FTLOSRExit,
@@ -47,20 +46,16 @@
         : m_value(encode(true, Special::Uninitialized))
     { }
 
-    static uint64_t encode(bool expectDoesGC, unsigned nodeIndex, unsigned nodeOp)
+    static uint32_t encode(bool expectDoesGC, unsigned nodeIndex, unsigned nodeOp)
     {
-        uint64_t result = bits(nodeIndex) << nodeIndexShift;
-        result |= bits(nodeOp) << nodeOpShift;
-        result |= bits(expectDoesGC) << expectDoesGCShift;
-        return result;
+        // We know nodeOp always fits because of the static_assert in DFGDoesGCCheck.cpp.
+        ASSERT((nodeIndex << nodeIndexShift) >> nodeIndexShift == nodeIndex);
+        return nodeIndex << nodeIndexShift | nodeOp << nodeOpShift | bits(expectDoesGC);
     }
 
-    static uint64_t encode(bool expectDoesGC, Special special)
+    static uint32_t encode(bool expectDoesGC, Special special)
     {
-        ASSERT(bits(special) < numberOfSpecials);
-        uint64_t result = bits(special) << specialShift;
-        result |= bits(expectDoesGC) << expectDoesGCShift;
-        return result;
+        return bits(special) << specialShift | isSpecialBit | bits(expectDoesGC);
     }
 
     void set(bool expectDoesGC, unsigned nodeIndex, unsigned nodeOp)
@@ -73,32 +68,39 @@
         m_value = encode(expectDoesGC, special);
     }
 
-    bool expectDoesGC() { return m_value & expectDoesGCMask; }
-    Special special() { return static_cast<Special>(specialIndex()); }
+    bool expectDoesGC() const { return m_value & expectDoesGCBit; }
+    bool isSpecial() const { return m_value & isSpecialBit; }
+
+    Special special() { return static_cast<Special>(m_value >> specialShift); }
+
+    unsigned nodeOp() { return (m_value >> nodeOpShift) & nodeOpMask; }
     unsigned nodeIndex() { return m_value >> nodeIndexShift; }
-    unsigned nodeOp() { return (m_value >> nodeOpShift) & nodeOpMask; }
 
-    bool isSpecial() { return specialIndex(); }
-
     JS_EXPORT_PRIVATE void verifyCanGC(VM&);
 
 private:
-    unsigned specialIndex() { return (m_value >> specialShift) & specialMask; }
+    template<typename T> static uint32_t bits(T value) { return static_cast<uint32_t>(value); }
 
-    template<typename T> static uint64_t bits(T value) { return static_cast<uint64_t>(value); }
+    // The value cannot be both a Special and contain node information at the
+    // time. Hence, the 2 can have separate encodings. The isSpecial bit
+    // determines which encoding is in use.
 
-    static constexpr uint64_t numberOfSpecials = static_cast<uint64_t>(Special::NumberOfSpecials);
+    static constexpr unsigned expectDoesGCBit = 1 << 0;
+    static constexpr unsigned isSpecialBit = 1 << 1;
+    static constexpr unsigned commonBits = 2;
+    static_assert((expectDoesGCBit | isSpecialBit) == (1 << commonBits) - 1);
 
-    static constexpr unsigned expectDoesGCShift = 0;
-    static constexpr unsigned specialShift = 1;
-    static constexpr unsigned nodeOpShift = 16;
-    static constexpr unsigned nodeIndexShift = 32;
+    static constexpr unsigned specialShift = commonBits;
 
-    static constexpr unsigned expectDoesGCMask = 0x1;
-    static constexpr unsigned specialMask = 0x7fff;
-    static constexpr unsigned nodeOpMask = 0xffff;
+    static constexpr unsigned nodeOpBits = 9;
+    static constexpr unsigned nodeOpMask = (1 << nodeOpBits) - 1;
+    static constexpr unsigned nodeOpShift = commonBits;
 
-    uint64_t m_value { 0 };
+    static constexpr unsigned nodeIndexBits = 21;
+    static constexpr unsigned nodeIndexShift = nodeOpShift + nodeOpBits;
+    static_assert(nodeIndexShift + nodeIndexBits == 32);
+
+    uint32_t m_value { 0 };
 };
 
 } // namespace DFG

Modified: trunk/Source/_javascript_Core/dfg/DFGNodeType.h (262561 => 262562)


--- trunk/Source/_javascript_Core/dfg/DFGNodeType.h	2020-06-04 19:52:52 UTC (rev 262561)
+++ trunk/Source/_javascript_Core/dfg/DFGNodeType.h	2020-06-04 19:54:17 UTC (rev 262562)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012-2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2012-2020 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -552,6 +552,10 @@
     LastNodeType
 };
 
+#define DFG_OP_COUNT(opcode, flags) + 1
+constexpr unsigned numberOfNodeTypes = FOR_EACH_DFG_OP(DFG_OP_COUNT);
+#undef DFG_OP_COUNT
+
 // Specifies the default flags for each node.
 inline NodeFlags defaultFlags(NodeType op)
 {

Modified: trunk/Source/_javascript_Core/dfg/DFGOSRExit.cpp (262561 => 262562)


--- trunk/Source/_javascript_Core/dfg/DFGOSRExit.cpp	2020-06-04 19:52:52 UTC (rev 262561)
+++ trunk/Source/_javascript_Core/dfg/DFGOSRExit.cpp	2020-06-04 19:54:17 UTC (rev 262562)
@@ -560,7 +560,7 @@
         // to set it here because compileOSRExit() is only called on the first time
         // we exit from this site, but all subsequent exits will take this compiled
         // ramp without calling compileOSRExit() first.
-        jit.store64(CCallHelpers::TrustedImm64(DoesGCCheck::encode(true, DoesGCCheck::Special::DFGOSRExit)), vm.heap.addressOfDoesGC());
+        jit.store32(CCallHelpers::TrustedImm32(DoesGCCheck::encode(true, DoesGCCheck::Special::DFGOSRExit)), vm.heap.addressOfDoesGC());
     }
 #endif

Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp (262561 => 262562)


--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp	2020-06-04 19:52:52 UTC (rev 262561)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT32_64.cpp	2020-06-04 19:54:17 UTC (rev 262562)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011-2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2011-2020 Apple Inc. All rights reserved.
  * Copyright (C) 2011 Intel Corporation. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -33,6 +33,7 @@
 #include "CallFrameShuffler.h"
 #include "DFGAbstractInterpreterInlines.h"
 #include "DFGCallArrayAllocatorSlowPathGenerator.h"
+#include "DFGDoesGC.h"
 #include "DFGOperations.h"
 #include "DFGSlowPathGenerator.h"
 #include "DirectArguments.h"
@@ -1806,6 +1807,11 @@
 {
     NodeType op = node->op();
 
+    if (validateDFGDoesGC) {
+        bool expectDoesGC = doesGC(m_jit.graph(), node);
+        m_jit.store32(TrustedImm32(DoesGCCheck::encode(expectDoesGC, node->index(), node->op())), vm().heap.addressOfDoesGC());
+    }
+
 #if ENABLE(DFG_REGISTER_ALLOCATION_VALIDATION)
     m_jit.clearRegisterAllocationOffsets();
 #endif

Modified: trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp (262561 => 262562)


--- trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp	2020-06-04 19:52:52 UTC (rev 262561)
+++ trunk/Source/_javascript_Core/dfg/DFGSpeculativeJIT64.cpp	2020-06-04 19:54:17 UTC (rev 262562)
@@ -2138,7 +2138,7 @@
 
     if (validateDFGDoesGC) {
         bool expectDoesGC = doesGC(m_jit.graph(), node);
-        m_jit.store64(TrustedImm64(DoesGCCheck::encode(expectDoesGC, node->index(), node->op())), vm().heap.addressOfDoesGC());
+        m_jit.store32(TrustedImm32(DoesGCCheck::encode(expectDoesGC, node->index(), node->op())), vm().heap.addressOfDoesGC());
     }
 
 #if ENABLE(DFG_REGISTER_ALLOCATION_VALIDATION)

Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (262561 => 262562)


--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp	2020-06-04 19:52:52 UTC (rev 262561)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp	2020-06-04 19:54:17 UTC (rev 262562)
@@ -704,7 +704,7 @@
 
         if (validateDFGDoesGC) {
             bool expectDoesGC = doesGC(m_graph, m_node);
-            m_out.store(m_out.constInt64(DoesGCCheck::encode(expectDoesGC, m_node->index(), m_node->op())), m_out.absolute(vm().heap.addressOfDoesGC()));
+            m_out.store(m_out.constInt32(DoesGCCheck::encode(expectDoesGC, m_node->index(), m_node->op())), m_out.absolute(vm().heap.addressOfDoesGC()));
         }
 
         switch (m_node->op()) {

Modified: trunk/Source/_javascript_Core/ftl/FTLOSRExitCompiler.cpp (262561 => 262562)


--- trunk/Source/_javascript_Core/ftl/FTLOSRExitCompiler.cpp	2020-06-04 19:52:52 UTC (rev 262561)
+++ trunk/Source/_javascript_Core/ftl/FTLOSRExitCompiler.cpp	2020-06-04 19:54:17 UTC (rev 262562)
@@ -215,7 +215,7 @@
         // to set it here because compileFTLOSRExit() is only called on the first time
         // we exit from this site, but all subsequent exits will take this compiled
         // ramp without calling compileFTLOSRExit() first.
-        jit.store64(CCallHelpers::TrustedImm64(DoesGCCheck::encode(true, DoesGCCheck::Special::FTLOSRExit)), vm.heap.addressOfDoesGC());
+        jit.store32(CCallHelpers::TrustedImm32(DoesGCCheck::encode(true, DoesGCCheck::Special::FTLOSRExit)), vm.heap.addressOfDoesGC());
     }
 
     // Bring the stack back into a sane form and assert that it's sane.

Modified: trunk/Source/_javascript_Core/heap/Heap.h (262561 => 262562)


--- trunk/Source/_javascript_Core/heap/Heap.h	2020-06-04 19:52:52 UTC (rev 262561)
+++ trunk/Source/_javascript_Core/heap/Heap.h	2020-06-04 19:54:17 UTC (rev 262562)
@@ -98,7 +98,12 @@
 class Worklist;
 }
 
-#define ENABLE_DFG_DOES_GC_VALIDATION ASSERT_ENABLED
+#if ENABLE(DFG_JIT) && ASSERT_ENABLED
+#define ENABLE_DFG_DOES_GC_VALIDATION 1
+#else
+#define ENABLE_DFG_DOES_GC_VALIDATION 0
+#endif
+
 constexpr bool validateDFGDoesGC = ENABLE_DFG_DOES_GC_VALIDATION;
 
 typedef HashCountedSet<JSCell*> ProtectCountSet;

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to