Changeset 262562 in webkit

Timestamp:

Jun 4, 2020 12:54:17 PM (4 years ago)

Author:

mark.lam@apple.com

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:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGDoesGCCheck.cpp (modified) (3 diffs)
• dfg/DFGDoesGCCheck.h (modified) (3 diffs)
• dfg/DFGNodeType.h (modified) (2 diffs)
• dfg/DFGOSRExit.cpp (modified) (1 diff)
• dfg/DFGSpeculativeJIT32_64.cpp (modified) (3 diffs)
• dfg/DFGSpeculativeJIT64.cpp (modified) (1 diff)
• ftl/FTLLowerDFGToB3.cpp (modified) (1 diff)
• ftl/FTLOSRExitCompiler.cpp (modified) (1 diff)
• heap/Heap.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-06-04  Mark Lam  <mark.lam@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_horton@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGDoesGCCheck.cpp

@@ -29 +29 @@
 #include "CallFrame.h"
 #include "CodeBlock.h"
+#include "DFGNodeType.h"
 #include "Heap.h"
 #include "VMInspector.h"
@@ -42 +43 @@
 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");
@@ -54 +59 @@
                 dataLog(" @ FTL osr exit");
                 break;
-            case Special::ReservedUnused:
             case Special::NumberOfSpecials:
                 RELEASE_ASSERT_NOT_REACHED();

trunk/Source/JavaScriptCore/dfg/DFGDoesGCCheck.h

@@ -37 +37 @@
 struct DoesGCCheck {
     enum class Special {
-        ReservedUnused, // so that specials start with 1. isSpecial() relies on this.
         Uninitialized,
         DFGOSRExit,
@@ -48 +47 @@
     { }
 
-    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);
     }
 
@@ -74 +69 @@
     }
 
-    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 };
 };
 

trunk/Source/JavaScriptCore/dfg/DFGNodeType.h

@@ -1 +1 @@
 /*
- * 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
@@ -553 +553 @@
 };
 
+#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)

trunk/Source/JavaScriptCore/dfg/DFGOSRExit.cpp

@@ -561 +561 @@
         // 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

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp

@@ -1 +1 @@
 /*
- * 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.
  *
@@ -34 +34 @@
 #include "DFGAbstractInterpreterInlines.h"
 #include "DFGCallArrayAllocatorSlowPathGenerator.h"
+#include "DFGDoesGC.h"
 #include "DFGOperations.h"
 #include "DFGSlowPathGenerator.h"
@@ -1806 +1807 @@
 {
     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)

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp

@@ -2139 +2139 @@
     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());
     }
 

trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

@@ -705 +705 @@
         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()));
         }
 

trunk/Source/JavaScriptCore/ftl/FTLOSRExitCompiler.cpp

@@ -216 +216 @@
         // 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());
     }
 

trunk/Source/JavaScriptCore/heap/Heap.h

@@ -99 +99 @@
 }
 
-#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;