Bug 1046183 - Move Scalar Replacement after the EliminatePhis phase. r=jandem
authorNicolas B. Pierron <nicolas.b.pierron@mozilla.com>
Thu, 21 Aug 2014 21:48:23 +0200
changeset 200935 1043b94f196992c9f95e2155d5e36ebcb7e2f905
parent 200934 e4886c5bea1c57934bab47e424894356a2fa8670
child 200936 48f6eb406821fa76e527fc7cec8b371b0422b557
push id27358
push userkwierso@gmail.com
push dateFri, 22 Aug 2014 01:22:28 +0000
treeherdermozilla-central@cd2acc7ab2f8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjandem
bugs1046183
milestone34.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1046183 - Move Scalar Replacement after the EliminatePhis phase. r=jandem
js/src/jit/Ion.cpp
js/src/jit/ScalarReplacement.cpp
--- a/js/src/jit/Ion.cpp
+++ b/js/src/jit/Ion.cpp
@@ -1346,27 +1346,16 @@ OptimizeMIR(MIRGenerator *mir)
         if (!BuildDominatorTree(graph))
             return false;
         // No spew: graph not changed.
 
         if (mir->shouldCancel("Dominator Tree"))
             return false;
     }
 
-    if (mir->optimizationInfo().scalarReplacementEnabled()) {
-        AutoTraceLog log(logger, TraceLogger::ScalarReplacement);
-        if (!ScalarReplacement(mir, graph))
-            return false;
-        IonSpewPass("Scalar Replacement");
-        AssertGraphCoherency(graph);
-
-        if (mir->shouldCancel("Scalar Replacement"))
-            return false;
-    }
-
     {
         AutoTraceLog log(logger, TraceLogger::PhiAnalysis);
         // Aggressive phi elimination must occur before any code elimination. If the
         // script contains a try-statement, we only compiled the try block and not
         // the catch or finally blocks, so in this case it's also invalid to use
         // aggressive phi elimination.
         Observability observability = graph.hasTryBlock()
                                       ? ConservativeObservability
@@ -1383,16 +1372,27 @@ OptimizeMIR(MIRGenerator *mir)
             return false;
         AssertExtendedGraphCoherency(graph);
         // No spew: graph not changed.
 
         if (mir->shouldCancel("Phi reverse mapping"))
             return false;
     }
 
+    if (mir->optimizationInfo().scalarReplacementEnabled()) {
+        AutoTraceLog log(logger, TraceLogger::ScalarReplacement);
+        if (!ScalarReplacement(mir, graph))
+            return false;
+        IonSpewPass("Scalar Replacement");
+        AssertGraphCoherency(graph);
+
+        if (mir->shouldCancel("Scalar Replacement"))
+            return false;
+    }
+
     if (!mir->compilingAsmJS()) {
         AutoTraceLog log(logger, TraceLogger::ApplyTypes);
         if (!ApplyTypeInformation(mir, graph))
             return false;
         IonSpewPass("Apply types");
         AssertExtendedGraphCoherency(graph);
 
         if (mir->shouldCancel("Apply types"))
--- a/js/src/jit/ScalarReplacement.cpp
+++ b/js/src/jit/ScalarReplacement.cpp
@@ -3,16 +3,17 @@
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "jit/ScalarReplacement.h"
 
 #include "mozilla/Vector.h"
 
+#include "jit/IonAnalysis.h"
 #include "jit/MIR.h"
 #include "jit/MIRGenerator.h"
 #include "jit/MIRGraph.h"
 
 namespace js {
 namespace jit {
 
 // Scan resume point operands in search of a local variable which captures the
@@ -316,18 +317,21 @@ ObjectMemoryView::mergeIntoSuccessorStat
         // of the successor block, after all the phi nodes.  Note that it
         // would be captured by the entry resume point of the successor
         // block.
         succ->insertBefore(*succ->begin(), succState);
         *pSuccState = succState;
     }
 
     if (succ->numPredecessors() > 1) {
+        // We need to re-compute successorWithPhis as the previous EliminatePhis
+        // phase might have removed all the Phis from the successor block.
         size_t currIndex = succ->indexForPredecessor(curr);
         MOZ_ASSERT(succ->getPredecessor(currIndex) == curr);
+        curr->setSuccessorWithPhis(succ, currIndex);
 
         // Copy the current slot states to the index of current block in all the
         // Phi created during the first visit of the successor.
         for (size_t slot = 0; slot < state_->numSlots(); slot++) {
             MPhi *phi = succState->getSlot(slot)->toPhi();
             phi->replaceOperand(currIndex, state_->getSlot(slot));
         }
     }
@@ -812,31 +816,27 @@ ScalarReplacementOfArray(MIRGenerator *m
                         succState->setElement(index, phi);
                     }
                 } else {
                     succState = states[succ->id()] = state;
                 }
             }
 
             if (succ->numPredecessors() > 1) {
-                // The current block might appear multiple times among the
-                // predecessors. As we need to replace all the inputs, we need
-                // to check all predecessors against the current block to
-                // replace the Phi node operands.
-                size_t numPreds = succ->numPredecessors();
-                for (size_t p = 0; p < numPreds; p++) {
-                    if (succ->getPredecessor(p) != *block)
-                        continue;
+                // We need to re-compute successorWithPhis as the previous EliminatePhis
+                // phase might have removed all the Phis from the successor block.
+                size_t currIndex = succ->indexForPredecessor(*block);
+                MOZ_ASSERT(succ->getPredecessor(currIndex) == *block);
+                block->setSuccessorWithPhis(succ, currIndex);
 
-                    // Copy the current slot state to the predecessor index of
-                    // each Phi of the same slot.
-                    for (size_t index = 0; index < state->numElements(); index++) {
-                        MPhi *phi = succState->getElement(index)->toPhi();
-                        phi->replaceOperand(p, state->getElement(index));
-                    }
+                // Copy the current slot state to the predecessor index of
+                // each Phi of the same slot.
+                for (size_t index = 0; index < state->numElements(); index++) {
+                    MPhi *phi = succState->getElement(index)->toPhi();
+                    phi->replaceOperand(currIndex, state->getElement(index));
                 }
             }
         }
     }
 
     MOZ_ASSERT(!arr->hasLiveDefUses());
     arr->setRecoveredOnBailout();
     states.clear();
@@ -844,35 +844,47 @@ ScalarReplacementOfArray(MIRGenerator *m
 }
 
 
 bool
 ScalarReplacement(MIRGenerator *mir, MIRGraph &graph)
 {
     EmulateStateOf<ObjectMemoryView> replaceObject(mir, graph);
     ArrayTrait::GraphState arrayStates;
+    bool addedPhi = false;
 
     for (ReversePostorderIterator block = graph.rpoBegin(); block != graph.rpoEnd(); block++) {
         if (mir->shouldCancel("Scalar Replacement (main loop)"))
             return false;
 
         for (MInstructionIterator ins = block->begin(); ins != block->end(); ins++) {
             if (ins->isNewObject() && !IsObjectEscaped(*ins)) {
                 ObjectMemoryView view(graph.alloc(), *ins);
                 if (!replaceObject.run(view))
                     return false;
                 view.assertSuccess();
+                addedPhi = true;
                 continue;
             }
 
             if (ins->isNewArray() && !IsArrayEscaped(*ins)) {
                 if (!ScalarReplacementOfArray(mir, graph, arrayStates, *ins))
                     return false;
+                addedPhi = true;
                 continue;
             }
         }
     }
 
+    if (addedPhi) {
+        // Phis added by Scalar Replacement are only redundant Phis which are
+        // not directly captured by any resume point but only by the MDefinition
+        // state. The conservative observability only focuses on Phis which are
+        // not used as resume points operands.
+        if (!EliminatePhis(mir, graph, ConservativeObservability))
+            return false;
+    }
+
     return true;
 }
 
 } /* namespace jit */
 } /* namespace js */