Changeset 183497 in webkit for trunk/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp

Timestamp:

Apr 28, 2015, 12:27:23 PM (10 years ago)

Author:

[email protected]

Message:

DFG should not use or preserve Phantoms during transformations
​https://bugs.webkit.org/show_bug.cgi?id=143736

Reviewed by Geoffrey Garen.

Since ​http://trac.webkit.org/changeset/183207 and ​http://trac.webkit.org/changeset/183406, it is
no longer necessary to preserve Phantoms during transformations. They are still useful just
before FixupPhase to support backwards propagation analyses. They are still inserted late in the
game in the DFG backend. But transformations don't need to worry about them. Inside a basic
block, we can be sure that so long as the IR pinpoints the place where the value becomes
available in a bytecode register (using MovHint) and so long as there is a SetLocal anytime some
other block would need the value (either for OSR or for DFG execution), then we don't need any
liveness markers.

So, this removes any places where we inserted Phantoms just for liveness during transformation
and it replaces convertToPhantom() with remove(), which just converts the node to a Check. A
Check node only keeps its children so long as those children have checks.

The fact that we no longer convertToPhantom() means that we have to be more careful when
constant-folding GetLocal. Previously we would convertToPhantom() and use the fact that
Phantom(Phi) was a valid construct. It's not valid anymore. So, when constant folding encounters
a GetLocal it needs to insert a PhantomLocal directly. This allows us to simplify
Graph::convertToConstant() a bit. Luckily, none of the other users of this method would see
GetLocals.

The only Phantom-like cruft left over after this patch is:

• Phantoms before FixupPhase. I kind of like these. It means that before FixupPhase, we can do backwards analyses and rely on the fact that the users of a node in DFG IR are a superset of the users of the original local's live range in bytecode. This is essential for supporting our BackwardsPropagationPhase, which is an important optimization for things like asm.js.

• PhantomLocals and GetLocals being NodeMustGenerate. See discussion in ​https://bugs.webkit.org/show_bug.cgi?id=144086. It appears that this is not as evil as the alternatives. The best long-term plan is to simply ditch the ThreadedCPS IR entirely and have the DFG use SSA. For now, so long as any new DFG optimizations we add are block-local and treat GetLocal/SetLocal conservatively, this should all be sound.

This change should be perf-neutral although it does reduce the total work that the compiler
does.

• CMakeLists.txt:
• JavaScriptCore.vcxproj/JavaScriptCore.vcxproj:
• JavaScriptCore.xcodeproj/project.pbxproj:
• dfg/DFGAdjacencyList.h:

(JSC::DFG::AdjacencyList::justChecks):

• dfg/DFGArgumentsEliminationPhase.cpp:
• dfg/DFGBasicBlock.cpp:

(JSC::DFG::BasicBlock::replaceTerminal):

• dfg/DFGBasicBlock.h:

(JSC::DFG::BasicBlock::findTerminal):

• dfg/DFGCFGSimplificationPhase.cpp:

(JSC::DFG::CFGSimplificationPhase::keepOperandAlive):
(JSC::DFG::CFGSimplificationPhase::mergeBlocks):

• dfg/DFGCPSRethreadingPhase.cpp:

(JSC::DFG::CPSRethreadingPhase::CPSRethreadingPhase):
(JSC::DFG::CPSRethreadingPhase::clearVariables):
(JSC::DFG::CPSRethreadingPhase::canonicalizeFlushOrPhantomLocalFor):
(JSC::DFG::CPSRethreadingPhase::canonicalizeLocalsInBlock):

• dfg/DFGCSEPhase.cpp:
• dfg/DFGCleanUpPhase.cpp: Copied from Source/JavaScriptCore/dfg/DFGPhantomRemovalPhase.cpp.

(JSC::DFG::CleanUpPhase::CleanUpPhase):
(JSC::DFG::CleanUpPhase::run):
(JSC::DFG::performCleanUp):
(JSC::DFG::PhantomRemovalPhase::PhantomRemovalPhase): Deleted.
(JSC::DFG::PhantomRemovalPhase::run): Deleted.
(JSC::DFG::performPhantomRemoval): Deleted.

• dfg/DFGCleanUpPhase.h: Copied from Source/JavaScriptCore/dfg/DFGPhantomRemovalPhase.h.
• dfg/DFGConstantFoldingPhase.cpp:

(JSC::DFG::ConstantFoldingPhase::foldConstants):
(JSC::DFG::ConstantFoldingPhase::addBaseCheck):
(JSC::DFG::ConstantFoldingPhase::fixUpsilons):

• dfg/DFGDCEPhase.cpp:

(JSC::DFG::DCEPhase::run):
(JSC::DFG::DCEPhase::fixupBlock):
(JSC::DFG::DCEPhase::cleanVariables):

• dfg/DFGFixupPhase.cpp:

(JSC::DFG::FixupPhase::fixupBlock):
(JSC::DFG::FixupPhase::fixupNode):
(JSC::DFG::FixupPhase::convertStringAddUse):
(JSC::DFG::FixupPhase::attemptToMakeFastStringAdd):
(JSC::DFG::FixupPhase::checkArray):
(JSC::DFG::FixupPhase::fixIntConvertingEdge):
(JSC::DFG::FixupPhase::fixIntOrBooleanEdge):
(JSC::DFG::FixupPhase::fixDoubleOrBooleanEdge):
(JSC::DFG::FixupPhase::injectTypeConversionsInBlock):
(JSC::DFG::FixupPhase::tryToRelaxRepresentation):
(JSC::DFG::FixupPhase::injectTypeConversionsForEdge):
(JSC::DFG::FixupPhase::addRequiredPhantom): Deleted.
(JSC::DFG::FixupPhase::addPhantomsIfNecessary): Deleted.

• dfg/DFGGraph.cpp:

(JSC::DFG::Graph::convertToConstant):
(JSC::DFG::Graph::mergeRelevantToOSR): Deleted.

• dfg/DFGGraph.h:
• dfg/DFGInsertionSet.h:

(JSC::DFG::InsertionSet::insertCheck):

• dfg/DFGIntegerCheckCombiningPhase.cpp:

(JSC::DFG::IntegerCheckCombiningPhase::handleBlock):

• dfg/DFGLICMPhase.cpp:

(JSC::DFG::LICMPhase::attemptHoist):

• dfg/DFGNode.cpp:

(JSC::DFG::Node::remove):

• dfg/DFGNode.h:

(JSC::DFG::Node::replaceWith):
(JSC::DFG::Node::convertToPhantom): Deleted.
(JSC::DFG::Node::convertToCheck): Deleted.
(JSC::DFG::Node::willHaveCodeGenOrOSR): Deleted.

• dfg/DFGNodeFlags.h:
• dfg/DFGNodeType.h:
• dfg/DFGObjectAllocationSinkingPhase.cpp:

(JSC::DFG::ObjectAllocationSinkingPhase::lowerNonReadingOperationsOnPhantomAllocations):
(JSC::DFG::ObjectAllocationSinkingPhase::handleNode):

• dfg/DFGPhantomCanonicalizationPhase.cpp: Removed.
• dfg/DFGPhantomCanonicalizationPhase.h: Removed.
• dfg/DFGPhantomRemovalPhase.cpp: Removed.
• dfg/DFGPhantomRemovalPhase.h: Removed.
• dfg/DFGPlan.cpp:

(JSC::DFG::Plan::compileInThreadImpl):

• dfg/DFGPutStackSinkingPhase.cpp:
• dfg/DFGResurrectionForValidationPhase.cpp: Removed.
• dfg/DFGResurrectionForValidationPhase.h: Removed.
• dfg/DFGSSAConversionPhase.cpp:

(JSC::DFG::SSAConversionPhase::run):

• dfg/DFGSpeculativeJIT64.cpp:

(JSC::DFG::SpeculativeJIT::fillSpeculateDouble):

• dfg/DFGStoreBarrierElisionPhase.cpp:

(JSC::DFG::StoreBarrierElisionPhase::elideBarrier):

• dfg/DFGStrengthReductionPhase.cpp:

(JSC::DFG::StrengthReductionPhase::handleNode):
(JSC::DFG::StrengthReductionPhase::convertToIdentityOverChild):

• dfg/DFGValidate.cpp:

(JSC::DFG::Validate::validate):
(JSC::DFG::Validate::validateCPS):
(JSC::DFG::Validate::validateSSA):

• dfg/DFGVarargsForwardingPhase.cpp:
• ftl/FTLLink.cpp:

(JSC::FTL::link):

• ftl/FTLLowerDFGToLLVM.cpp:

(JSC::FTL::LowerDFGToLLVM::compileNode):
(JSC::FTL::LowerDFGToLLVM::compileNoOp):
(JSC::FTL::LowerDFGToLLVM::compilePhantom): Deleted.

File:

• trunk/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp (modified) (5 diffs)

trunk/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp

@@ -51 +51 @@
         m_graph.computeRefCounts();
         
-        if (m_graph.m_form == SSA) {
-            for (BasicBlock* block : m_graph.blocksInPreOrder())
-                fixupBlock(block);
-            
-            // This is like cleanVariables, but has a much simpler approach to GetLocal.
-            for (unsigned i = m_graph.m_arguments.size(); i--;) {
-                Node* node = m_graph.m_arguments[i];
-                if (!node)
-                    continue;
-                if (node->op() != Phantom && node->op() != Check && node->shouldGenerate())
-                    continue;
-                m_graph.m_arguments[i] = nullptr;
-            }
-        } else {
-            RELEASE_ASSERT(m_graph.m_form == ThreadedCPS);
-            
-            for (BlockIndex blockIndex = 0; blockIndex < m_graph.numBlocks(); ++blockIndex)
-                fixupBlock(m_graph.block(blockIndex));
-            cleanVariables(m_graph.m_arguments);
-        }
+        for (BasicBlock* block : m_graph.blocksInPreOrder())
+            fixupBlock(block);
         
+        cleanVariables(m_graph.m_arguments);
+
         // Just do a basic Phantom/Check clean-up.
         for (BlockIndex blockIndex = m_graph.numBlocks(); blockIndex--;) {
@@ -105 +89 @@
         if (!block)
             return;
-        
-        switch (m_graph.m_form) {
-        case SSA:
-            break;
-            
-        case ThreadedCPS: {
-            // Clean up variable links for the block. We need to do this before the actual DCE
-            // because we need to see GetLocals, so we can bypass them in situations where the
-            // vars-at-tail point to a GetLocal, the GetLocal is dead, but the Phi it points
-            // to is alive.
-            
+
+        if (m_graph.m_form == ThreadedCPS) {
             for (unsigned phiIndex = 0; phiIndex < block->phis.size(); ++phiIndex) {
-                if (!block->phis[phiIndex]->shouldGenerate()) {
-                    // FIXME: We could actually free nodes here. Except that it probably
-                    // doesn't matter, since we don't add any nodes after this phase.
-                    // https://bugs.webkit.org/show_bug.cgi?id=126239
+                Node* phi = block->phis[phiIndex];
+                if (!phi->shouldGenerate()) {
+                    m_graph.m_allocator.free(phi);
                     block->phis[phiIndex--] = block->phis.last();
                     block->phis.removeLast();
@@ -128 +102 @@
             cleanVariables(block->variablesAtHead);
             cleanVariables(block->variablesAtTail);
-            break;
-        }
-            
-        default:
-            RELEASE_ASSERT_NOT_REACHED();
-            return;
         }
 
@@ -142 +110 @@
                 continue;
                 
-            switch (node->op()) {
-            case MovHint:
-            case ZombieHint:
-                // These are not killable. (They once were.)
-                RELEASE_ASSERT_NOT_REACHED();
+            if (node->flags() & NodeHasVarArgs) {
+                for (unsigned childIdx = node->firstChild(); childIdx < node->firstChild() + node->numChildren(); childIdx++) {
+                    Edge edge = m_graph.m_varArgChildren[childIdx];
+                    
+                    if (!edge || edge.willNotHaveCheck())
+                        continue;
+                    
+                    m_insertionSet.insertNode(indexInBlock, SpecNone, Check, node->origin, edge);
+                }
                 
-            default: {
-                if (node->flags() & NodeHasVarArgs) {
-                    for (unsigned childIdx = node->firstChild(); childIdx < node->firstChild() + node->numChildren(); childIdx++) {
-                        Edge edge = m_graph.m_varArgChildren[childIdx];
-
-                        if (!edge || edge.isProved() || edge.willNotHaveCheck())
-                            continue;
-
-                        m_insertionSet.insertNode(indexInBlock, SpecNone, Check, node->origin, edge);
-                    }
-
-                    node->convertToPhantom();
-                    node->children.reset();
-                    node->setRefCount(1);
-                    break;
-                }
-
-                node->convertToCheck();
-                for (unsigned i = 0; i < AdjacencyList::Size; ++i) {
-                    Edge edge = node->children.child(i);
-                    if (!edge)
-                        continue;
-                    if (edge.isProved() || edge.willNotHaveCheck())
-                        node->children.removeEdge(i--);
-                }
+                node->setOpAndDefaultFlags(Check);
+                node->children.reset();
                 node->setRefCount(1);
-                break;
-            } }
+                continue;
+            }
+            
+            node->remove();
+            node->setRefCount(1);
         }
 
@@ -188 +140 @@
             if (!node)
                 continue;
-            if (node->op() != Phantom && node->op() != Check && node->shouldGenerate())
+            if (node->op() != Check && node->shouldGenerate())
                 continue;
-            if (node->op() == GetLocal) {
-                node = node->child1().node();
-                
-                ASSERT(node->op() == Phi || node->op() == SetArgument);
-                
-                if (node->shouldGenerate()) {
-                    variables[i] = node;
-                    continue;
-                }
-            }
-            variables[i] = 0;
+            variables[i] = nullptr;
         }
     }