Changeset 245035 in webkit

Timestamp:

May 7, 2019 2:28:38 PM (5 years ago)

Author:

rmorisset@apple.com

Message:

[B3] Constants should be hoisted to the root block until moveConstants
​https://bugs.webkit.org/show_bug.cgi?id=197265

Reviewed by Saam Barati.

This patch does the following:

• B3ReduceStrength now hoists all constants to the root BB, and de-duplicates them along the way
• B3PureCSE no longer bothers with constants, since they are already de-duplicated by the time it gets to see them
• We now run eliminateDeadCode just after moveConstants, so that the Nops that moveConstants generates are freed instead of staying live throughout Air compilation, reducing memory pressure.
• I also took the opportunity to fix typos in comments in various parts of the code base.

Here are a few numbers to justify this patch:

• In JetStream2, about 27% of values at the beginning of B3 are constants
• In JetStream2, about 11% of values at the end of B3 are Nops
• In JetStream2, this patch increases the number of times that tail duplication happens from a bit less than 24k to a bit more than 25k (hoisting constants makes blocks smaller).

When I tried measuring the total effect on JetStream2 I got a tiny and almost certainly non-significant progression.

• b3/B3Generate.cpp:

(JSC::B3::generateToAir):

• b3/B3MoveConstants.cpp:
• b3/B3PureCSE.cpp:

(JSC::B3::PureCSE::process):

• b3/B3PureCSE.h:
• b3/B3ReduceStrength.cpp:
• bytecode/GetByIdStatus.cpp:

(JSC::GetByIdStatus::computeForStubInfoWithoutExitSiteFeedback):

• dfg/DFGCSEPhase.cpp:
• dfg/DFGOSRAvailabilityAnalysisPhase.h:
• dfg/DFGOSRExit.cpp:

(JSC::DFG::OSRExit::executeOSRExit):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• b3/B3Generate.cpp (modified) (1 diff)
• b3/B3MoveConstants.cpp (modified) (1 diff)
• b3/B3PureCSE.cpp (modified) (1 diff)
• b3/B3PureCSE.h (modified) (1 diff)
• b3/B3ReduceStrength.cpp (modified) (4 diffs)
• bytecode/GetByIdStatus.cpp (modified) (1 diff)
• dfg/DFGCSEPhase.cpp (modified) (1 diff)
• dfg/DFGOSRAvailabilityAnalysisPhase.h (modified) (1 diff)
• dfg/DFGOSRExit.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-05-07  Robin Morisset  <rmorisset@apple.com>
+
+        [B3] Constants should be hoisted to the root block until moveConstants
+        https://bugs.webkit.org/show_bug.cgi?id=197265
+
+        Reviewed by Saam Barati.
+
+        This patch does the following:
+        - B3ReduceStrength now hoists all constants to the root BB, and de-duplicates them along the way
+        - B3PureCSE no longer bothers with constants, since they are already de-duplicated by the time it gets to see them
+        - We now run eliminateDeadCode just after moveConstants, so that the Nops that moveConstants generates are freed instead of staying live throughout Air compilation, reducing memory pressure.
+        - I also took the opportunity to fix typos in comments in various parts of the code base.
+
+        Here are a few numbers to justify this patch:
+        - In JetStream2, about 27% of values at the beginning of B3 are constants
+        - In JetStream2, about 11% of values at the end of B3 are Nops
+        - In JetStream2, this patch increases the number of times that tail duplication happens from a bit less than 24k to a bit more than 25k (hoisting constants makes blocks smaller).
+
+        When I tried measuring the total effect on JetStream2 I got a tiny and almost certainly non-significant progression.
+
+        * b3/B3Generate.cpp:
+        (JSC::B3::generateToAir):
+        * b3/B3MoveConstants.cpp:
+        * b3/B3PureCSE.cpp:
+        (JSC::B3::PureCSE::process):
+        * b3/B3PureCSE.h:
+        * b3/B3ReduceStrength.cpp:
+        * bytecode/GetByIdStatus.cpp:
+        (JSC::GetByIdStatus::computeForStubInfoWithoutExitSiteFeedback):
+        * dfg/DFGCSEPhase.cpp:
+        * dfg/DFGOSRAvailabilityAnalysisPhase.h:
+        * dfg/DFGOSRExit.cpp:
+        (JSC::DFG::OSRExit::executeOSRExit):
+
 2019-05-07  Robin Morisset  <rmorisset@apple.com>
 

trunk/Source/JavaScriptCore/b3/B3Generate.cpp

@@ -117 +117 @@
     legalizeMemoryOffsets(procedure);
     moveConstants(procedure);
+    eliminateDeadCode(procedure);
 
     // FIXME: We should run pureCSE here to clean up some platform specific changes from the previous phases.

trunk/Source/JavaScriptCore/b3/B3MoveConstants.cpp

@@ -155 +155 @@
                 
                 // We call this when we have found a constant that we'd like to use. It's possible that
-                // we have computed that the constant should be meterialized in this block, but we
+                // we have computed that the constant should be materialized in this block, but we
                 // haven't inserted it yet. This inserts the constant if necessary.
                 auto materialize = [&] (Value* child) {

trunk/Source/JavaScriptCore/b3/B3PureCSE.cpp

@@ -69 +69 @@
 bool PureCSE::process(Value* value, Dominators& dominators)
 {
-    if (value->opcode() == Identity)
+    if (value->opcode() == Identity || value->isConstant())
         return false;
 

trunk/Source/JavaScriptCore/b3/B3PureCSE.h

@@ -41 +41 @@
 
 // This is a reusable utility for doing pure CSE. You can use it to do pure CSE on a program by just
-// proceeding in order an calling process().
+// proceeding in order and calling process().
 class PureCSE {
 public:

trunk/Source/JavaScriptCore/b3/B3ReduceStrength.cpp

@@ -403 +403 @@
         , m_insertionSet(proc)
         , m_blockInsertionSet(proc)
+        , m_root(proc.at(0))
     {
     }
@@ -443 +444 @@
             // put them. We're not actually smart enough to move things around at random.
             m_changed |= eliminateDeadCodeImpl(m_proc);
+            m_valueForConstant.clear();
             
             simplifySSA();
@@ -2146 +2148 @@
             break;
         }
-            
+
+        case Const32:
+        case Const64:
+        case ConstFloat:
+        case ConstDouble: {
+            ValueKey key = m_value->key();
+            if (Value* constInRoot = m_valueForConstant.get(key)) {
+                if (constInRoot != m_value) {
+                    m_value->replaceWithIdentity(constInRoot);
+                    m_changed = true;
+                }
+            } else if (m_block == m_root)
+                m_valueForConstant.add(key, m_value);
+            else {
+                Value* constInRoot = m_proc.clone(m_value);
+                m_root->appendNonTerminal(constInRoot);
+                m_valueForConstant.add(key, constInRoot);
+                m_value->replaceWithIdentity(constInRoot);
+                m_changed = true;
+            }
+            break;
+        }
+
         default:
             break;
@@ -2795 +2819 @@
     InsertionSet m_insertionSet;
     BlockInsertionSet m_blockInsertionSet;
+    HashMap<ValueKey, Value*> m_valueForConstant;
+    BasicBlock* m_root { nullptr };
     BasicBlock* m_block { nullptr };
     unsigned m_index { 0 };

trunk/Source/JavaScriptCore/bytecode/GetByIdStatus.cpp

@@ -277 +277 @@
 
                 if (domAttribute) {
-                    // Give up when cutom accesses are not merged into one.
+                    // Give up when custom accesses are not merged into one.
                     if (result.numVariants() != 1)
                         return GetByIdStatus(JSC::slowVersion(summary));

trunk/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp

@@ -258 +258 @@
     }
 
-    // The majority of Impure Stack Slotsare unique per value.
+    // The majority of Impure Stack Slots are unique per value.
     // This is very useful for fast clobber(), we can just remove the slot addressed by AbstractHeap
     // in O(1).

trunk/Source/JavaScriptCore/dfg/DFGOSRAvailabilityAnalysisPhase.h

@@ -34 +34 @@
 class Graph;
 
-// Computes BasicBlock::ssa->availabiltiyAtHead/Tail. This is a forward flow type inference
+// Computes BasicBlock::ssa->availabilityAtHead/Tail. This is a forward flow type inference
 // over MovHints and SetLocals. This analysis is run directly by the Plan for preparing for
 // lowering to B3 IR, but it can also be used as a utility. Note that if you run it before

trunk/Source/JavaScriptCore/dfg/DFGOSRExit.cpp

@@ -453 +453 @@
     context.sp() = context.fp<uint8_t*>() + exitState.stackPointerOffset;
 
-    // The only reason for using this do while look is so we can break out midway when appropriate.
+    // The only reason for using this do while loop is so we can break out midway when appropriate.
     do {
         auto extraInitializationLevel = static_cast<ExtraInitializationLevel>(exitState.extraInitializationLevel);