Bug 1601599 part 3 - Simplify loop phi code in IonBuilder. r=tcampbell
authorJan de Mooij <jdemooij@mozilla.com>
Sun, 15 Dec 2019 11:33:36 +0000
changeset 507040 93842c3971f8e612537cba5d6d25b482c0d67a66
parent 507039 4903b1b93526423b2a52251081252fcfe100d8f5
child 507041 d9c1e44eb0addfc3e44ae299c86381128aa08e7f
push id103228
push userjdemooij@mozilla.com
push dateSun, 15 Dec 2019 11:41:22 +0000
treeherderautoland@ba6bf1f3c391 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstcampbell
bugs1601599, 980263
milestone73.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 1601599 part 3 - Simplify loop phi code in IonBuilder. r=tcampbell Before this bug: * We did not add phis for expression stack slots that aren't part of the loop. * We disallowed OSR into Ion at loops that have such non-loop stack slots. This goes back to bug 980263 to fix the following problem: JSOP_INITPROP in IonBuilder assumed it found MNewObject on the stack and got confused when it found a loop phi instead. There was a similar issue with MNewArray. Since then the affected ops have been fixed and supporting OSR at these loops (and inserting phis for all stack slots) simplifies the code and fixes a potential perf cliff. Depends on D56699 Differential Revision: https://phabricator.services.mozilla.com/D56700
js/src/jit/IonBuilder.cpp
js/src/jit/IonBuilder.h
js/src/jit/MIRGraph.cpp
js/src/jit/MIRGraph.h
--- a/js/src/jit/IonBuilder.cpp
+++ b/js/src/jit/IonBuilder.cpp
@@ -1797,58 +1797,38 @@ AbortReasonOr<Ok> IonBuilder::visitGoto(
   return Ok();
 }
 
 AbortReasonOr<Ok> IonBuilder::jsop_loophead() {
   // All loops have the following bytecode structure:
   //
   //    LOOPHEAD
   //    ...
-  //    IFNE/IFEQ/GOTO to LOOPHEAD
+  //    IFNE/GOTO to LOOPHEAD
 
   MOZ_ASSERT(*pc == JSOP_LOOPHEAD);
 
   if (hasTerminatedBlock()) {
     // The whole loop is unreachable.
     return Ok();
   }
 
-  jssrcnote* sn = GetSrcNote(gsn, script(), pc);
-  MOZ_ASSERT(sn);
-
-  uint32_t stackPhiCount;
-  switch (SN_TYPE(sn)) {
-    case SRC_FOR_OF:
-      stackPhiCount = 3;
-      break;
-    case SRC_FOR_IN:
-    case SRC_FOR:
-    case SRC_WHILE:
-    case SRC_DO_WHILE:
-      stackPhiCount = 0;
-      break;
-    default:
-      MOZ_CRASH("Unexpected source note");
-  }
-
-  bool canOsr = LoopHeadCanIonOsr(pc);
   bool osr = pc == info().osrPc();
   if (osr) {
-    MOZ_ASSERT(canOsr);
+    MOZ_ASSERT(LoopHeadCanIonOsr(pc));
 
     MBasicBlock* preheader;
     MOZ_TRY_VAR(preheader, newOsrPreheader(current, pc));
     current->end(MGoto::New(alloc(), preheader));
     MOZ_TRY(setCurrentAndSpecializePhis(preheader));
   }
 
   loopDepth_++;
   MBasicBlock* header;
-  MOZ_TRY_VAR(header,
-              newPendingLoopHeader(current, pc, osr, canOsr, stackPhiCount));
+  MOZ_TRY_VAR(header, newPendingLoopHeader(current, pc, osr));
   current->end(MGoto::New(alloc(), header));
 
   if (!loopStack_.emplaceBack(header)) {
     return abort(AbortReason::Alloc);
   }
 
   MOZ_TRY(analyzeNewLoopTypes(header));
 
@@ -7815,26 +7795,19 @@ AbortReasonOr<MBasicBlock*> IonBuilder::
     return abort(AbortReason::Alloc);
   }
   graph().setOsrBlock(osrBlock);
 
   return preheader;
 }
 
 AbortReasonOr<MBasicBlock*> IonBuilder::newPendingLoopHeader(
-    MBasicBlock* predecessor, jsbytecode* pc, bool osr, bool canOsr,
-    unsigned stackPhiCount) {
-  // If this site can OSR, all values on the expression stack are part of the
-  // loop.
-  if (canOsr) {
-    stackPhiCount = predecessor->stackDepth() - info().firstStackSlot();
-  }
-
+    MBasicBlock* predecessor, jsbytecode* pc, bool osr) {
   MBasicBlock* block = MBasicBlock::NewPendingLoopHeader(
-      graph(), info(), predecessor, bytecodeSite(pc), stackPhiCount);
+      graph(), info(), predecessor, bytecodeSite(pc));
   if (!block) {
     return abort(AbortReason::Alloc);
   }
 
   if (osr) {
     // Incorporate type information from the OSR frame into the loop
     // header. The OSR frame may have unexpected types due to type changes
     // within the loop body or due to incomplete profiling information,
--- a/js/src/jit/IonBuilder.h
+++ b/js/src/jit/IonBuilder.h
@@ -229,19 +229,17 @@ class IonBuilder : public MIRGenerator,
   AbortReasonOr<MBasicBlock*> newBlockPopN(MBasicBlock* predecessor,
                                            jsbytecode* pc, uint32_t popped);
   AbortReasonOr<MBasicBlock*> newBlockAfter(
       MBasicBlock* at, size_t stackDepth, jsbytecode* pc,
       MBasicBlock* maybePredecessor = nullptr);
   AbortReasonOr<MBasicBlock*> newOsrPreheader(MBasicBlock* header,
                                               jsbytecode* loopHead);
   AbortReasonOr<MBasicBlock*> newPendingLoopHeader(MBasicBlock* predecessor,
-                                                   jsbytecode* pc, bool osr,
-                                                   bool canOsr,
-                                                   unsigned stackPhiCount);
+                                                   jsbytecode* pc, bool osr);
 
   AbortReasonOr<MBasicBlock*> newBlock(MBasicBlock* predecessor,
                                        jsbytecode* pc) {
     return newBlock(predecessor->stackDepth(), pc, predecessor);
   }
 
   AbortReasonOr<Ok> addPendingEdge(const PendingEdge& edge, jsbytecode* target);
 
--- a/js/src/jit/MIRGraph.cpp
+++ b/js/src/jit/MIRGraph.cpp
@@ -299,28 +299,26 @@ MBasicBlock* MBasicBlock::NewWithResumeP
   }
 
   return block;
 }
 
 MBasicBlock* MBasicBlock::NewPendingLoopHeader(MIRGraph& graph,
                                                const CompileInfo& info,
                                                MBasicBlock* pred,
-                                               BytecodeSite* site,
-                                               unsigned stackPhiCount) {
+                                               BytecodeSite* site) {
   MOZ_ASSERT(site->pc() != nullptr);
 
   MBasicBlock* block =
       new (graph.alloc()) MBasicBlock(graph, info, site, PENDING_LOOP_HEADER);
   if (!block->init()) {
     return nullptr;
   }
 
-  if (!block->inherit(graph.alloc(), pred->stackDepth(), pred, 0,
-                      stackPhiCount)) {
+  if (!block->inherit(graph.alloc(), pred->stackDepth(), pred, 0)) {
     return nullptr;
   }
 
   return block;
 }
 
 MBasicBlock* MBasicBlock::NewSplitEdge(MIRGraph& graph, MBasicBlock* pred,
                                        size_t predEdgeIdx, MBasicBlock* succ) {
@@ -518,18 +516,17 @@ void MBasicBlock::copySlots(MBasicBlock*
   MDefinition** thisSlots = slots_.begin();
   MDefinition** fromSlots = from->slots_.begin();
   for (size_t i = 0, e = stackPosition_; i < e; ++i) {
     thisSlots[i] = fromSlots[i];
   }
 }
 
 bool MBasicBlock::inherit(TempAllocator& alloc, size_t stackDepth,
-                          MBasicBlock* maybePred, uint32_t popped,
-                          unsigned stackPhiCount) {
+                          MBasicBlock* maybePred, uint32_t popped) {
   MOZ_ASSERT_IF(maybePred, maybePred->stackDepth() == stackDepth);
 
   MOZ_ASSERT(stackDepth >= popped);
   stackDepth -= popped;
   stackPosition_ = stackDepth;
 
   if (maybePred && kind_ != PENDING_LOOP_HEADER) {
     copySlots(maybePred);
@@ -549,41 +546,17 @@ bool MBasicBlock::inherit(TempAllocator&
   }
 
   if (maybePred) {
     if (!predecessors_.append(maybePred)) {
       return false;
     }
 
     if (kind_ == PENDING_LOOP_HEADER) {
-      size_t i = 0;
-      for (i = 0; i < info().firstStackSlot(); i++) {
-        MPhi* phi = MPhi::New(alloc.fallible());
-        if (!phi) {
-          return false;
-        }
-        phi->addInlineInput(maybePred->getSlot(i));
-        addPhi(phi);
-        setSlot(i, phi);
-        entryResumePoint()->initOperand(i, phi);
-      }
-
-      MOZ_ASSERT(stackPhiCount <= stackDepth);
-      MOZ_ASSERT(info().firstStackSlot() <= stackDepth - stackPhiCount);
-
-      // Avoid creating new phis for stack values that aren't part of the
-      // loop.  Note that for loop headers that can OSR, all values on the
-      // stack are part of the loop.
-      for (; i < stackDepth - stackPhiCount; i++) {
-        MDefinition* val = maybePred->getSlot(i);
-        setSlot(i, val);
-        entryResumePoint()->initOperand(i, val);
-      }
-
-      for (; i < stackDepth; i++) {
+      for (size_t i = 0; i < stackDepth; i++) {
         MPhi* phi = MPhi::New(alloc.fallible());
         if (!phi) {
           return false;
         }
         phi->addInlineInput(maybePred->getSlot(i));
         addPhi(phi);
         setSlot(i, phi);
         entryResumePoint()->initOperand(i, phi);
--- a/js/src/jit/MIRGraph.h
+++ b/js/src/jit/MIRGraph.h
@@ -39,18 +39,17 @@ class MBasicBlock : public TempObject, p
   enum Kind { NORMAL, PENDING_LOOP_HEADER, LOOP_HEADER, SPLIT_EDGE, DEAD };
 
  private:
   MBasicBlock(MIRGraph& graph, const CompileInfo& info, BytecodeSite* site,
               Kind kind);
   MOZ_MUST_USE bool init();
   void copySlots(MBasicBlock* from);
   MOZ_MUST_USE bool inherit(TempAllocator& alloc, size_t stackDepth,
-                            MBasicBlock* maybePred, uint32_t popped,
-                            unsigned stackPhiCount = 0);
+                            MBasicBlock* maybePred, uint32_t popped);
   MOZ_MUST_USE bool inheritResumePoint(MBasicBlock* pred);
   void assertUsesAreNotWithin(MUseIterator use, MUseIterator end);
 
   // This block cannot be reached by any means.
   bool unreachable_;
 
   // Keeps track if the phis has been type specialized already.
   bool specialized_;
@@ -118,18 +117,17 @@ class MBasicBlock : public TempObject, p
                               uint32_t popn);
   static MBasicBlock* NewWithResumePoint(MIRGraph& graph,
                                          const CompileInfo& info,
                                          MBasicBlock* pred, BytecodeSite* site,
                                          MResumePoint* resumePoint);
   static MBasicBlock* NewPendingLoopHeader(MIRGraph& graph,
                                            const CompileInfo& info,
                                            MBasicBlock* pred,
-                                           BytecodeSite* site,
-                                           unsigned loopStateSlots);
+                                           BytecodeSite* site);
   static MBasicBlock* NewSplitEdge(MIRGraph& graph, MBasicBlock* pred,
                                    size_t predEdgeIdx, MBasicBlock* succ);
 
   bool dominates(const MBasicBlock* other) const {
     return other->domIndex() - domIndex() < numDominated();
   }
 
   void setId(uint32_t id) { id_ = id; }