Bug 905091 part 2 - Ensure no instructions are inserted between an instruction and its OsiPoint. r=bhackett
authorJan de Mooij <jdemooij@mozilla.com>
Fri, 16 Aug 2013 10:40:30 +0200
changeset 155767 1b16c2c729b9d62e103846aa9c9276fe5600dfc7
parent 155732 6bcbba81872755a5093d768294bbf0ab3901dee0
child 155768 c51c5ea1758de1d55920db4a5cf6d7cab8e1f3f1
push id2961
push userlsblakk@mozilla.com
push dateMon, 28 Oct 2013 21:59:28 +0000
treeherdermozilla-beta@73ef4f13486f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbhackett
bugs905091
milestone26.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 905091 part 2 - Ensure no instructions are inserted between an instruction and its OsiPoint. r=bhackett
js/src/jit/CodeGenerator.cpp
js/src/jit/LinearScan.cpp
js/src/jit/RegisterAllocator.h
--- a/js/src/jit/CodeGenerator.cpp
+++ b/js/src/jit/CodeGenerator.cpp
@@ -813,16 +813,30 @@ CodeGenerator::visitOsiPoint(LOsiPoint *
 
     uint32_t osiCallPointOffset;
     if (!markOsiPoint(lir, &osiCallPointOffset))
         return false;
 
     LSafepoint *safepoint = lir->associatedSafepoint();
     JS_ASSERT(!safepoint->osiCallPointOffset());
     safepoint->setOsiCallPointOffset(osiCallPointOffset);
+
+#ifdef DEBUG
+    // There should be no movegroups or other instructions between
+    // an instruction and its OsiPoint. This is necessary because
+    // we use the OsiPoint's snapshot from within VM calls.
+    for (LInstructionReverseIterator iter(current->rbegin(lir)); iter != current->rend(); iter++) {
+        if (*iter == lir || iter->isNop())
+            continue;
+        JS_ASSERT(!iter->isMoveGroup());
+        JS_ASSERT(iter->safepoint() == safepoint);
+        break;
+    }
+#endif
+
     return true;
 }
 
 bool
 CodeGenerator::visitGoto(LGoto *lir)
 {
     jumpToBlock(lir->target());
     return true;
--- a/js/src/jit/LinearScan.cpp
+++ b/js/src/jit/LinearScan.cpp
@@ -304,16 +304,35 @@ bool
 LinearScanAllocator::moveInputAlloc(CodePosition pos, LAllocation *from, LAllocation *to)
 {
     if (*from == *to)
         return true;
     LMoveGroup *moves = getInputMoveGroup(pos);
     return moves->add(from, to);
 }
 
+static inline void
+SetOsiPointUses(LiveInterval *interval, CodePosition defEnd, const LAllocation &allocation)
+{
+    // Moves are inserted after OsiPoint instructions. This function sets
+    // any OsiPoint uses of this interval to the allocation of the value
+    // before the move.
+
+    JS_ASSERT(interval->index() == 0);
+
+    for (UsePositionIterator usePos(interval->usesBegin());
+         usePos != interval->usesEnd();
+         usePos++)
+    {
+        if (usePos->pos > defEnd)
+            break;
+        *static_cast<LAllocation *>(usePos->use) = allocation;
+    }
+}
+
 /*
  * This function takes the allocations assigned to the live intervals and
  * erases all virtual registers in the function with the allocations
  * corresponding to the appropriate interval.
  */
 bool
 LinearScanAllocator::reifyAllocations()
 {
@@ -344,34 +363,27 @@ LinearScanAllocator::reifyAllocations()
         }
 
         // Erase the def of this interval if it's the first one
         if (interval->index() == 0)
         {
             LDefinition *def = reg->def();
             LAllocation *spillFrom;
 
+            // Insert the moves after any OsiPoint or Nop instructions
+            // following this one. See minimalDefEnd for more info.
+            CodePosition defEnd = minimalDefEnd(reg->ins());
+
             if (def->policy() == LDefinition::PRESET && def->output()->isRegister()) {
                 AnyRegister fixedReg = def->output()->toRegister();
                 LiveInterval *from = fixedIntervals[fixedReg.code()];
 
-                // Insert the move after any OsiPoint or Nop instructions
-                // following this one. See minimalDefEnd for more info.
-                CodePosition defEnd = minimalDefEnd(reg->ins());
-
-                // If we just skipped an OsiPoint, and it uses this vreg, it
-                // should use the fixed register instead.
-                for (UsePositionIterator usePos(interval->usesBegin());
-                     usePos != interval->usesEnd();
-                     usePos++)
-                {
-                    if (usePos->pos > defEnd)
-                        break;
-                    *static_cast<LAllocation *>(usePos->use) = LAllocation(fixedReg);
-                }
+                // If we insert the move after an OsiPoint that uses this vreg,
+                // it should use the fixed register instead.
+                SetOsiPointUses(interval, defEnd, LAllocation(fixedReg));
 
                 if (!moveAfter(defEnd, from, interval))
                     return false;
                 spillFrom = from->getAllocation();
             } else {
                 if (def->policy() == LDefinition::MUST_REUSE_INPUT) {
                     LAllocation *alloc = reg->ins()->getOperand(def->getReusedInput());
                     LAllocation *origAlloc = LAllocation::New(*alloc);
@@ -396,21 +408,24 @@ LinearScanAllocator::reifyAllocations()
                     if (entry->isUse() && entry->toUse()->policy() == LUse::RECOVERED_INPUT)
                         *entry = *def->output();
                 }
             }
 
             if (reg->mustSpillAtDefinition() && !reg->ins()->isPhi() &&
                 (*reg->canonicalSpill() != *spillFrom))
             {
-                // Insert a spill at the input of the next instruction. Control
-                // instructions never have outputs, so the next instruction is
-                // always valid. Note that we explicitly ignore phis, which
-                // should have been handled in resolveControlFlow().
-                LMoveGroup *moves = getMoveGroupAfter(outputOf(reg->ins()));
+                // If we move the spill after an OsiPoint, the OsiPoint should
+                // use the original location instead.
+                SetOsiPointUses(interval, defEnd, *spillFrom);
+
+                // Insert a spill after this instruction (or after any OsiPoint
+                // or Nop instructions). Note that we explicitly ignore phis,
+                // which should have been handled in resolveControlFlow().
+                LMoveGroup *moves = getMoveGroupAfter(defEnd);
                 if (!moves->add(spillFrom, reg->canonicalSpill()))
                     return false;
             }
         }
         else if (interval->start() > inputOf(insData[interval->start()].block()->firstId()) &&
                  (!reg->canonicalSpill() ||
                   (reg->canonicalSpill() == interval->getAllocation() &&
                    !reg->mustSpillAtDefinition()) ||
--- a/js/src/jit/RegisterAllocator.h
+++ b/js/src/jit/RegisterAllocator.h
@@ -352,18 +352,17 @@ class RegisterAllocator
         }
         return i;
     }
 
     CodePosition minimalDefEnd(LInstruction *ins) {
         // Compute the shortest interval that captures vregs defined by ins.
         // Watch for instructions that are followed by an OSI point and/or Nop.
         // If moves are introduced between the instruction and the OSI point then
-        // safepoint information for the instruction may be incorrect. This is
-        // pretty disgusting and should be fixed somewhere else in the compiler.
+        // safepoint information for the instruction may be incorrect.
         while (true) {
             LInstruction *next = insData[outputOf(ins).next()].ins();
             if (!next->isNop() && !next->isOsiPoint())
                 break;
             ins = next;
         }
 
         return outputOf(ins);