Eliminate Marker instructions

This diff removes the Marker opcode, replacing it with a BCMarker
struct in each IRInstruction. This gives us fewer redundant lines in IRTrace
dumps and allows for more straightforward control of which IRInstructions are
associated with which bytecodes. I took this opportunity to do some more
cleanup of ir dumps as well, and it's now possible to interpOne every codegen
punt.

hphp/doc/ir.specification

@@ -1273,12 +1273,6 @@ DecRefNZ S0:Gen
 13. Misc
 
 
-Marker<bcOff,stackOff,func>
-
-  Record HHBC virtual position information at the position of this
-  instruction.  Marker has no executable semantics.  The JIT compiler
-  uses this to generate bytecode<->machine code maps.
-
 D:FramePtr = DefFP
 
   Creates a temporary D representing the current vm frame pointer.

hphp/runtime/base/memory/smart_containers.h

@@ -167,6 +167,9 @@ public:
   explicit vector(A&&... args) : Base(std::forward<A>(args)...) {}
 };
 
+template <class T, class Container = deque<T>>
+class stack : public std::stack<T, Container> {};
+
 template <class T>
 class list : public std::list<T,Allocator<T>> {
   typedef std::list<T,Allocator<T>> Base;

hphp/runtime/vm/jit/block.h

@@ -119,16 +119,13 @@ struct Block : boost::noncopyable {
   void setPostId(unsigned id) { m_postid = id; }
 
   /*
-   * Insert inst after this block's optional DefLabel, optional
-   * BeginCatch, and Marker, return an iterator to the newly inserted
-   * instruction.
-   *
-   * Pre: the block contains a Marker after the optional DefLabel.
+   * Insert inst after this block's optional DefLabel and BeginCatch,
+   * then return an iterator to the newly inserted instruction.
    */
   iterator prepend(IRInstruction* inst) {
+    assert(inst->marker().valid());
     auto it = skipHeader();
-    assert(it->op() == Marker);
-    return insert(++it, inst);
+    return insert(it, inst);
   }
 
   // return iterator to first instruction after the DefLabel (if
@@ -146,7 +143,11 @@ struct Block : boost::noncopyable {
   }
 
   // return iterator to last instruction
-  iterator backIter() { auto it = end(); return --it; }
+  iterator backIter() {
+    assert(!empty());
+    auto it = end();
+    return --it;
+  }
 
   // return an iterator to a specific instruction
   iterator iteratorTo(IRInstruction* inst) {
@@ -156,8 +157,12 @@ struct Block : boost::noncopyable {
 
   // Accessors of list of predecessor edges.  Each edge has a from() property
   // which is the predecessor block.
+  EdgeList& preds()             { return m_preds; }
   const EdgeList& preds() const { return m_preds; }
   size_t numPreds() const { return m_preds.size(); }
+
+  // Remove edge from its destination's predecessor list and insert it in
+  // new_to's predecessor list.
   static Block* updatePreds(Edge* edge, Block* new_to) {
     if (Block* old_to = edge->to()) {
       auto &preds = old_to->m_preds;
@@ -210,15 +215,21 @@ struct Block : boost::noncopyable {
   const_iterator   end()   const { return m_instrs.end(); }
 
   iterator insert(iterator pos, IRInstruction* inst) {
+    assert(inst->marker().valid());
     inst->setBlock(this);
     return m_instrs.insert(pos, *inst);
   }
-  void splice(iterator pos, Block* from, iterator begin, iterator end) {
+  void splice(iterator pos, Block* from, iterator begin, iterator end,
+              BCMarker newMarker) {
     assert(from != this);
-    for (auto i = begin; i != end; ++i) (*i).setBlock(this);
+    for (auto i = begin; i != end; ++i) {
+      i->setBlock(this);
+      i->setMarker(newMarker);
+    }
     m_instrs.splice(pos, from->instrs(), begin, end);
   }
   void push_back(IRInstruction* inst) {
+    assert(inst->marker().valid());
     inst->setBlock(this);
     return m_instrs.push_back(*inst);
   }

hphp/runtime/vm/jit/check.cpp

@@ -64,7 +64,7 @@ DEBUG_ONLY static int numBlockParams(Block* b) {
 /*
  * Check one block for being well formed. Invariants verified:
  * 1. The block begins with an optional DefLabel, followed by an optional
- *    BeginCatch, followed by either a Marker or no more instructions.
+ *    BeginCatch.
  * 2. DefLabel and BeginCatch may not appear anywhere in a block other than
  *    where specified in #1.
  * 3. If the optional BeginCatch is present, the block must belong to an exit
@@ -75,6 +75,7 @@ DEBUG_ONLY static int numBlockParams(Block* b) {
  *    blocks listed in the block list for this block's Trace.
  * 7. Any path from this block to a Block that expects values must be
  *    from a Jmp_ instruciton.
+ * 8. Every instruction's BCMarker must point to a valid bytecode instruction.
  */
 bool checkBlock(Block* b) {
   auto it = b->begin();
@@ -91,20 +92,17 @@ bool checkBlock(Block* b) {
     assert(b == b->trace()->front());
   }
 
-  // Invariant #3
-  if (it == end) return true;
-  assert(it->op() == Marker);
-
   // Invariants #2, #4
-  if (++it == end) return true;
+  if (it == end) return true;
   if (b->back()->isBlockEnd()) --end;
-  while (it != end && it->op() == Marker) ++it;
   for (DEBUG_ONLY IRInstruction& inst : folly::makeRange(it, end)) {
     assert(inst.op() != DefLabel);
     assert(inst.op() != BeginCatch);
     assert(!inst.isBlockEnd());
   }
   for (DEBUG_ONLY IRInstruction& inst : *b) {
+    // Invariant #8
+    assert(inst.marker().valid());
     assert(inst.block() == b);
   }
 

hphp/runtime/vm/jit/codegen.cpp

@@ -96,7 +96,8 @@ void cgPunt(const char* file, int line, const char* func, uint32_t bcOff,
   throw FailedCodeGen(file, line, func, bcOff, vmFunc);
 }
 
-#define CG_PUNT(instr) cgPunt(__FILE__, __LINE__, #instr, m_curBcOff, curFunc())
+#define CG_PUNT(instr) \
+  cgPunt(__FILE__, __LINE__, #instr, m_curInst->marker().bcOff, curFunc())
 
 struct CycleInfo {
   int node;
@@ -283,9 +284,8 @@ ArgDesc::ArgDesc(SSATmp* tmp, const RegisterInfo& info, bool val)
 }
 
 const Func* CodeGenerator::curFunc() const {
-  always_assert(m_state.lastMarker &&
-                "We shouldn't be looking for a func when we have no marker");
-  return m_state.lastMarker->func;
+  assert(m_curInst->marker().valid());
+  return m_curInst->marker().func;
 }
 
 /*
@@ -1182,14 +1182,6 @@ void CodeGenerator::cgCallHelper(Asm& a,
   }
 }
 
-/*
- * This doesn't really produce any code; it just keeps track of the current
- * bytecode offset.
- */
-void CodeGenerator::cgMarker(IRInstruction* inst) {
-  m_curBcOff = inst->extra<MarkerData>()->bcOff;
-}
-
 void CodeGenerator::cgMov(IRInstruction* inst) {
   assert(!m_regs[inst->src(0)].hasReg(1));//TODO: t2082361: handle Gen & Cell
   SSATmp* dst   = inst->dst();
@@ -3543,8 +3535,8 @@ void CodeGenerator::cgCall(IRInstruction* inst) {
     m_as.add_imm32_reg64(adjustment, spReg);
   }
 
-  assert(m_state.lastMarker);
-  SrcKey srcKey = SrcKey(m_state.lastMarker->func, m_state.lastMarker->bcOff);
+  assert(m_curInst->marker().valid());
+  SrcKey srcKey = SrcKey(m_curInst->marker().func, m_curInst->marker().bcOff);
   bool isImmutable = (func->isConst() && !func->type().isNull());
   const Func* funcd = isImmutable ? func->getValFunc() : nullptr;
   assert(&m_as == &m_tx64->getAsm());
@@ -3634,7 +3626,7 @@ void CodeGenerator::cgCallBuiltin(IRInstruction* inst) {
   int returnOffset = HHIR_MISOFF(tvBuiltinReturn);
 
   if (TranslatorX64::eagerRecord(func)) {
-    const uchar* pc = curUnit()->entry() + m_state.lastMarker->bcOff;
+    const uchar* pc = curUnit()->entry() + m_curInst->marker().bcOff;
     // we have spilled all args to stack, so spDiff is 0
     m_tx64->emitEagerSyncPoint(m_as, pc, 0);
   }
@@ -4103,9 +4095,9 @@ void CodeGenerator::cgLdRef(IRInstruction* inst) {
 
 void CodeGenerator::recordSyncPoint(Asm& as,
                                     SyncOptions sync /* = kSyncPoint */) {
-  assert(m_state.lastMarker);
+  assert(m_curInst->marker().valid());
 
-  Offset stackOff = m_state.lastMarker->stackOff;
+  Offset stackOff = m_curInst->marker().spOff;
   switch (sync) {
   case SyncOptions::kSyncPointAdjustOne:
     stackOff -= 1;
@@ -4116,7 +4108,7 @@ void CodeGenerator::recordSyncPoint(Asm& as,
     assert(0);
   }
 
-  Offset pcOff = m_state.lastMarker->bcOff - m_state.lastMarker->func->base();
+  Offset pcOff = m_curInst->marker().bcOff - m_curInst->marker().func->base();
 
   FTRACE(5, "IR recordSyncPoint: {} {} {}\n", as.code.frontier, pcOff,
          stackOff);
@@ -5410,16 +5402,17 @@ static void patchJumps(Asm& as, CodegenState& state, Block* block) {
 void CodeGenerator::cgBlock(Block* block, vector<TransBCMapping>* bcMap) {
   FTRACE(6, "cgBlock: {}\n", block->id());
 
+  BCMarker prevMarker;
   for (IRInstruction& instr : *block) {
     IRInstruction* inst = &instr;
-    if (inst->op() == Marker) {
-      m_state.lastMarker = inst->extra<Marker>();
-      FTRACE(7, "lastMarker is now {}\n", inst->extra<Marker>()->show());
-      if (m_tx64 && m_tx64->isTransDBEnabled() && bcMap) {
-        bcMap->push_back((TransBCMapping){Offset(m_state.lastMarker->bcOff),
-              m_as.code.frontier,
-              m_astubs.code.frontier});
-      }
+    // If we're on the first instruction of the block or we have a new
+    // marker since the last instruction, update the bc mapping.
+    if ((!prevMarker.valid() || inst->marker() != prevMarker) &&
+        m_tx64->isTransDBEnabled() && bcMap) {
+      bcMap->push_back(TransBCMapping{inst->marker().bcOff,
+                                      m_as.code.frontier,
+                                      m_astubs.code.frontier});
+      prevMarker = inst->marker();
     }
     m_curInst = inst;
     auto nuller = folly::makeGuard([&]{ m_curInst = nullptr; });
@@ -5508,7 +5501,6 @@ void genCodeForTrace(IRTrace* trace,
     }
 
     cg.cgBlock(block, bcMap);
-    state.lastMarker = nullptr;
     if (auto next = block->next()) {
       if (next != nextBlock) {
         // If there's a fallthrough block and it's not the next thing

hphp/runtime/vm/jit/codegen.h

@@ -68,7 +68,6 @@ struct CodegenState {
                AsmInfo* asmInfo)
     : patches(factory, nullptr)
     , addresses(factory, nullptr)
-    , lastMarker(nullptr)
     , regs(regs)
     , liveRegs(liveRegs)
     , lifetime(lifetime)
@@ -82,10 +81,6 @@ struct CodegenState {
   StateVector<Block,void*> patches;
   StateVector<Block,TCA> addresses;
 
-  // Keep track of the most recent Marker instruction we've seen in the
-  // current trace (even across blocks).
-  const MarkerData* lastMarker;
-
   // True if this block's terminal Jmp_ has a desination equal to the
   // next block in the same assmbler.
   bool noTerminalJmp_;
@@ -131,7 +126,7 @@ struct CodeGenerator {
     , m_rScratch(InvalidReg)
     , m_curInst(nullptr)
     , m_curTrace(trace)
-    , m_curBcOff(-1) {
+  {
   }
 
   void cgBlock(Block* block, vector<TransBCMapping>* bcMap);
@@ -376,8 +371,6 @@ private:
   Reg64               m_rScratch; // currently selected GP scratch reg
   IRInstruction*      m_curInst;  // current instruction being generated
   IRTrace*            m_curTrace;
-  uint32_t            m_curBcOff; // offset of bytecode instr that produced
-                                  // m_curInst
 };
 
 class ArgDesc {

hphp/runtime/vm/jit/dce.cpp

@@ -126,8 +126,16 @@ void removeDeadInstructions(IRTrace* trace, const DceState& state) {
               });
       return state[inst].isDead();
     });
-    // Marker and DefLabel instructions are marked live in reachable blocks
-    assert(!block->empty());
+    if (block->empty()) {
+      // Update any predecessors to point to the empty block's next block.
+      auto next = block->next();
+      for (auto it = block->preds().begin(); it != block->preds().end(); ) {
+        auto cur = it;
+        ++it;
+        cur->setTo(next);
+      }
+      trace->erase(cur);
+    }
   }
 }
 
@@ -325,7 +333,7 @@ void sinkIncRefs(IRTrace* trace, IRFactory* irFactory, DceState& state) {
     for (auto* inst : boost::adaptors::reverse(toSink)) {
       // prepend inserts an instruction to the beginning of a block, after
       // the label. Therefore, we iterate through toSink in the reversed order.
-      IRInstruction* sunkInst = irFactory->gen(IncRef, inst->src(0));
+      auto* sunkInst = irFactory->gen(IncRef, inst->marker(), inst->src(0));
       state[sunkInst].setLive();
       exit->front()->prepend(sunkInst);
 

hphp/runtime/vm/jit/extradata.h

@@ -108,19 +108,6 @@ struct JmpSwitchData : IRExtraData {
   Offset* targets;     // offsets for all targets
 };
 
-struct MarkerData : IRExtraData {
-  uint32_t    bcOff;    // the bytecode offset in unit
-  int32_t     stackOff; // stack off from start of trace
-  const Func* func;     // which func are we in
-
-  std::string show() const {
-    return folly::format("--- bc {}, spOff {} ({})",
-                         bcOff,
-                         stackOff,
-                         func->fullName()->data()).str();
-  }
-};
-
 struct LocalId : IRExtraData {
   explicit LocalId(uint32_t id)
     : locId(id)
@@ -345,7 +332,6 @@ struct InterpOneData : IRExtraData {
 X(JmpSwitchDest,                JmpSwitchData);
 X(LdSSwitchDestFast,            LdSSwitchData);
 X(LdSSwitchDestSlow,            LdSSwitchData);
-X(Marker,                       MarkerData);
 X(RaiseUninitLoc,               LocalId);
 X(GuardLoc,                     LocalId);
 X(CheckLoc,                     LocalId);

hphp/runtime/vm/jit/hhbctranslator.cpp

@@ -76,7 +76,7 @@ HhbcTranslator::HhbcTranslator(IRFactory& irFactory,
   , m_hasExit(false)
   , m_stackDeficit(0)
 {
-  emitMarker();
+  updateMarker();
   auto const fp = gen(DefFP);
   gen(DefSP, StackOffset(initialSpOffsetFromFp), fp);
 }
@@ -303,29 +303,25 @@ void HhbcTranslator::beginInlining(unsigned numParams,
     gen(StLoc, LocalId(i), calleeFP, m_tb->genDefUninit());
   }
 
-  emitMarker();
+  updateMarker();
 }
 
 bool HhbcTranslator::isInlining() const {
   return m_bcStateStack.size() > 1;
 }
 
-IRInstruction* HhbcTranslator::makeMarker(Offset bcOff) {
+BCMarker HhbcTranslator::makeMarker(Offset bcOff) {
   int32_t stackOff = m_tb->spOffset() +
     m_evalStack.numCells() - m_stackDeficit;
 
   FTRACE(2, "makeMarker: bc {} sp {} fn {}\n",
          bcOff, stackOff, curFunc()->fullName()->data());
 
-  MarkerData marker;
-  marker.bcOff     = bcOff;
-  marker.func      = curFunc();
-  marker.stackOff  = stackOff;
-  return m_irFactory.gen(Marker, marker);
+  return BCMarker{ curFunc(), bcOff, stackOff };
 }
 
-void HhbcTranslator::emitMarker() {
-  m_tb->add(makeMarker(bcOff()));
+void HhbcTranslator::updateMarker() {
+  m_tb->setMarker(makeMarker(bcOff()));
 }
 
 void HhbcTranslator::profileFunctionEntry(const char* category) {
@@ -373,7 +369,7 @@ void HhbcTranslator::setBcOff(Offset newOff, bool lastBcOff) {
 
   if (newOff != bcOff()) {
     m_bcStateStack.back().bcOff = newOff;
-    emitMarker();
+    updateMarker();
   }
   m_lastBcOff = lastBcOff;
 }
@@ -1540,8 +1536,8 @@ void HhbcTranslator::emitClsCnsD(int32_t cnsNameId, int32_t clsNameId,
   // chaining to another Tracelet so forward progress still happens.
   auto const sideExit = makeSideExit(
     nextBcOff(),
-    [&] (IRTrace* t) {
-      return genFor(t, LookupClsCns, Type::Cell, clsCnsName);
+    [&] {
+      return gen(LookupClsCns, Type::Cell, clsCnsName);
     }
   );
 
@@ -1940,7 +1936,7 @@ void HhbcTranslator::emitFPushObjMethodD(int32_t numParams,
 
     // This is special. We need to move the stackpointer incase LdObjMethod
     // calls a destructor. Otherwise it would clobber the ActRec we just pushed.
-    emitMarker();
+    updateMarker();
 
     gen(LdObjMethod,
               objCls,
@@ -2182,7 +2178,7 @@ void HhbcTranslator::emitRetFromInlined(Type type) {
   gen(InlineReturn, m_tb->fp());
 
   // Return to the caller function.  Careful between here and the
-  // emitMarker() below, where the caller state isn't entirely set up.
+  // updateMarker() below, where the caller state isn't entirely set up.
   m_bcStateStack.pop_back();
   m_fpiStack.pop();
 
@@ -2207,7 +2203,7 @@ void HhbcTranslator::emitRetFromInlined(Type type) {
   FTRACE(1, "]]] end inlining: {}\n", curFunc()->fullName()->data());
   push(retVal);
 
-  emitMarker();
+  updateMarker();
 }
 
 SSATmp* HhbcTranslator::emitDecRefLocalsInline(SSATmp* retVal) {
@@ -2960,7 +2956,8 @@ void HhbcTranslator::emitBindMem(SSATmp* ptr, SSATmp* src) {
   gen(StMem, ptr, cns(0), src);
   if (isRefCounted(src) && src->type().canRunDtor()) {
     Block* exitBlock = getExitTrace(nextBcOff())->front();
-    exitBlock->prepend(m_irFactory.gen(DecRef, prevValue));
+    exitBlock->prepend(m_irFactory.gen(DecRef, makeMarker(nextBcOff()),
+                                       prevValue));
     gen(DecRefNZOrBranch, exitBlock, prevValue);
   } else {
     gen(DecRef, prevValue);
@@ -3533,8 +3530,11 @@ std::string HhbcTranslator::showStack() const {
     out << folly::format("+{:-^62}+\n", str);
   };
 
-  const int32_t stackDepth = m_tb->spOffset() - curFunc()->numLocals() +
-    m_evalStack.size() - m_stackDeficit;
+  const int32_t frameCells = curFunc()->numLocals() +
+    curFunc()->numIterators() * kNumIterCells;
+  const int32_t stackDepth =
+    m_tb->spOffset() + m_evalStack.size() - m_stackDeficit -
+    (curFunc()->isGenerator() ? 0 : frameCells);
   auto spOffset = stackDepth;
   auto elem = [&](const std::string& str) {
     out << folly::format("| {:<60} |\n",
@@ -3606,19 +3606,19 @@ IRTrace* HhbcTranslator::getExitTrace(Offset targetBcOff /* = -1 */) {
 }
 
 IRTrace* HhbcTranslator::getExitTrace(Offset targetBcOff,
-                                    std::vector<SSATmp*>& spillValues) {
+                                      std::vector<SSATmp*>& spillValues) {
   if (targetBcOff == -1) targetBcOff = bcOff();
   return getExitTraceImpl(targetBcOff, ExitFlag::None, spillValues,
     CustomExit{});
 }
 
 IRTrace* HhbcTranslator::getExitTraceWarn(Offset targetBcOff,
-                                        std::vector<SSATmp*>& spillValues,
-                                        const StringData* warning) {
+                                          std::vector<SSATmp*>& spillValues,
+                                          const StringData* warning) {
   assert(targetBcOff != -1);
   return getExitTraceImpl(targetBcOff, ExitFlag::None, spillValues,
-    [&](IRTrace* t) -> SSATmp* {
-      genFor(t, RaiseWarning, cns(warning));
+    [&]() -> SSATmp* {
+      gen(RaiseWarning, cns(warning));
       return nullptr;
     }
   );
@@ -3643,22 +3643,16 @@ IRTrace* HhbcTranslator::getExitTraceImpl(Offset targetBcOff,
                                         ExitFlag flag,
                                         std::vector<SSATmp*>& stackValues,
                                         const CustomExit& customFn) {
+  BCMarker exitMarker;
+  exitMarker.bcOff = targetBcOff;
+  exitMarker.spOff = m_tb->spOffset() + stackValues.size() - m_stackDeficit;
+  exitMarker.func  = curFunc();
+
+  BCMarker currentMarker = makeMarker(bcOff());
+
   auto const exit = m_tb->makeExitTrace(targetBcOff);
-
-  MarkerData exitMarker;
-  exitMarker.bcOff    = targetBcOff;
-  exitMarker.stackOff = m_tb->spOffset() +
-                          stackValues.size() - m_stackDeficit;
-  exitMarker.func     = curFunc();
-
-  MarkerData currentMarker;
-  currentMarker.bcOff     = bcOff();
-  currentMarker.func      = curFunc();
-  currentMarker.stackOff  = m_tb->spOffset() +
-                              m_evalStack.numCells() - m_stackDeficit;
-
-  genFor(exit, Marker,
-         flag == ExitFlag::DelayedMarker ? currentMarker : exitMarker);
+  TracePusher tp(*m_tb, exit,
+                 flag == ExitFlag::DelayedMarker ? currentMarker : exitMarker);
 
   // The value we use for stack is going to depend on whether we have
   // to spillstack or what.
@@ -3670,41 +3664,38 @@ IRTrace* HhbcTranslator::getExitTraceImpl(Offset targetBcOff,
       stackValues.begin(),
       { m_tb->sp(), cns(int64_t(m_stackDeficit)) }
     );
-    stack = genFor(exit,
-      SpillStack, std::make_pair(stackValues.size(), &stackValues[0])
+    stack = gen(SpillStack, std::make_pair(stackValues.size(), &stackValues[0])
     );
   }
 
   if (customFn) {
-    stack = genFor(exit, ExceptionBarrier, stack);
-    auto const customTmp = customFn(exit);
+    stack = gen(ExceptionBarrier, stack);
+    auto const customTmp = customFn();
     if (customTmp) {
       SSATmp* spill2[] = { stack, cns(0), customTmp };
-      stack = genFor(exit,
-        SpillStack, std::make_pair(sizeof spill2 / sizeof spill2[0], spill2)
+      stack = gen(SpillStack,
+                  std::make_pair(sizeof spill2 / sizeof spill2[0], spill2)
       );
-      exitMarker.stackOff += 1;
+      exitMarker.spOff += 1;
     }
   }
 
   if (flag == ExitFlag::DelayedMarker) {
-    genFor(exit, Marker, exitMarker);
+    m_tb->setMarker(exitMarker);
   }
 
-  genFor(exit, SyncABIRegs, m_tb->fp(), stack);
+  gen(SyncABIRegs, m_tb->fp(), stack);
 
   if (flag == ExitFlag::NoIR) {
-    genFor(exit,
-      targetBcOff == m_startBcOff ? ReqRetranslateNoIR : ReqBindJmpNoIR,
-      BCOffset(targetBcOff)
-    );
+    gen(targetBcOff == m_startBcOff ? ReqRetranslateNoIR : ReqBindJmpNoIR,
+        BCOffset(targetBcOff));
     return exit;
   }
 
   if (bcOff() == m_startBcOff && targetBcOff == m_startBcOff) {
-    genFor(exit, ReqRetranslate);
+    gen(ReqRetranslate);
   } else {
-    genFor(exit, ReqBindJmp, BCOffset(targetBcOff));
+    gen(ReqBindJmp, BCOffset(targetBcOff));
   }
 
   return exit;
@@ -3721,31 +3712,26 @@ IRTrace* HhbcTranslator::getCatchTrace() {
   auto exit = m_tb->makeExitTrace(bcOff());
   assert(exit->blocks().size() == 1);
 
-  genFor(exit, BeginCatch);
-  exit->front()->push_back(makeMarker(bcOff()));
-  auto sp = emitSpillStack(exit, m_tb->sp(), peekSpillValues());
-  genFor(exit, EndCatch, sp);
+  TracePusher tp(*m_tb, exit, makeMarker(bcOff()));
+  gen(BeginCatch);
+  auto sp = emitSpillStack(m_tb->sp(), peekSpillValues());
+  gen(EndCatch, sp);
 
   assert(exit->blocks().size() == 1);
   return exit;
 }
 
-SSATmp* HhbcTranslator::emitSpillStack(IRTrace* t, SSATmp* sp,
+SSATmp* HhbcTranslator::emitSpillStack(SSATmp* sp,
                                        const std::vector<SSATmp*>& spillVals) {
   std::vector<SSATmp*> ssaArgs{ sp, cns(int64_t(m_stackDeficit)) };
   ssaArgs.insert(ssaArgs.end(), spillVals.begin(), spillVals.end());
 
   auto args = std::make_pair(ssaArgs.size(), &ssaArgs[0]);
-  if (t->isMain()) {
-    return gen(SpillStack, args);
-  } else {
-    return genFor(t, SpillStack, args);
-  }
+  return gen(SpillStack, args);
 }
 
 SSATmp* HhbcTranslator::spillStack() {
-  auto newSp =
-    emitSpillStack(m_tb->trace(), m_tb->sp(), peekSpillValues());
+  auto newSp = emitSpillStack(m_tb->sp(), peekSpillValues());
   m_evalStack.clear();
   m_stackDeficit = 0;
   return newSp;

hphp/runtime/vm/jit/hhbctranslator.h

@@ -538,7 +538,7 @@ private:
 
     template<class... Args>
     SSATmp* cns(Args&&... args) {
-      return m_tb.cns(std::forward<Args>(args)...);
+      return m_irf.cns(std::forward<Args>(args)...);
     }
 
     template<class... Args>
@@ -551,6 +551,7 @@ private:
     TraceBuilder& m_tb;
     IRFactory& m_irf;
     const MInstrInfo& m_mii;
+    const BCMarker m_marker;
     hphp_hash_map<unsigned, unsigned> m_stackInputs;
 
     unsigned m_mInd;
@@ -592,7 +593,7 @@ private:
 private: // tracebuilder forwarding utilities
   template<class... Args>
   SSATmp* cns(Args&&... args) {
-    return m_tb->cns(std::forward<Args>(args)...);
+    return m_irFactory.cns(std::forward<Args>(args)...);
   }
 
   template<class... Args>
@@ -600,11 +601,6 @@ private: // tracebuilder forwarding utilities
     return m_tb->gen(std::forward<Args>(args)...);
   }
 
-  template<class... Args>
-  SSATmp* genFor(IRTrace* trace, Args&&... args) {
-    return m_tb->genFor(trace, std::forward<Args>(args)...);
-  }
-
 private:
   /*
    * Emit helpers.
@@ -663,8 +659,8 @@ private:
   void emitBinaryArith(Opcode);
   template<class Lambda>
   SSATmp* emitIterInitCommon(int offset, Lambda genFunc);
-  IRInstruction* makeMarker(Offset bcOff);
-  void emitMarker();
+  BCMarker makeMarker(Offset bcOff);
+  void updateMarker();
   SSATmp* staticTVCns(const TypedValue*);
 
   Type interpOutputType(const NormalizedInstruction&) const;
@@ -673,22 +669,21 @@ private:
 private: // Exit trace creation routines.
   IRTrace* getExitTrace(Offset targetBcOff = -1);
   IRTrace* getExitTrace(Offset targetBcOff,
-                      std::vector<SSATmp*>& spillValues);
+                        std::vector<SSATmp*>& spillValues);
   IRTrace* getExitTraceWarn(Offset targetBcOff,
-                          std::vector<SSATmp*>& spillValues,
-                          const StringData* warning);
+                            std::vector<SSATmp*>& spillValues,
+                            const StringData* warning);
 
   /*
    * Create a custom side exit---that is, an exit that does some
    * amount work before leaving the trace.
    *
-   * The exit trace will spill things with a Marker for the current bytecode.
+   * The exit trace will spill things with for the current bytecode instruction.
    *
-   * Then it will do an ExceptionBarrier, followed by whatever is done
-   * by the CustomExit(IRTrace*) function.  The custom exit may add
-   * instructions to the exit trace, and optionally may return an
-   * additional SSATmp* to spill on the stack.  If there is no
-   * additional SSATmp*, it should return nullptr.
+   * Then it will do an ExceptionBarrier, followed by whatever is done by the
+   * CustomExit() function. Any instructions emitted by the custom exit will go
+   * to the exit trace, and it may return an additional SSATmp* to spill on the
+   * stack. If there is no additional SSATmp*, it should return nullptr.
    *
    * TODO(#2447661): this should be way better than this, should allow
    * using gen/push/spillStack/etc.
@@ -708,9 +703,9 @@ private: // Exit trace creation routines.
    * Implementation for the above.  Takes spillValues, target offset,
    * and a flag for whether to make a no-IR exit.
    *
-   * Also takes a CustomExit(IRTrace*) function that may perform more
-   * operations and optionally return a single additional SSATmp*
-   * (otherwise nullptr) to spill on the stack before exiting.
+   * Also takes a CustomExit() function that may perform more operations and
+   * optionally return a single additional SSATmp* (otherwise nullptr) to spill
+   * on the stack before exiting.
    */
   enum class ExitFlag {
     None,
@@ -720,11 +715,11 @@ private: // Exit trace creation routines.
     // instead of one for targetBcOff.
     DelayedMarker,
   };
-  typedef std::function<SSATmp* (IRTrace*)> CustomExit;
+  typedef std::function<SSATmp* ()> CustomExit;
   IRTrace* getExitTraceImpl(Offset targetBcOff,
-                          ExitFlag noIRExit,
-                          std::vector<SSATmp*>& spillValues,
-                          const CustomExit&);
+                            ExitFlag noIRExit,
+                            std::vector<SSATmp*>& spillValues,
+                            const CustomExit&);
 
 private:
   /*
@@ -787,7 +782,7 @@ private:
   SSATmp* topV(uint32_t i = 0) { return top(Type::BoxedCell, i); }
   Type    topType(uint32_t i) const;
   std::vector<SSATmp*> peekSpillValues() const;
-  SSATmp* emitSpillStack(IRTrace* t, SSATmp* sp,
+  SSATmp* emitSpillStack(SSATmp* sp,
                          const std::vector<SSATmp*>& spillVals);
   SSATmp* spillStack();
   void    exceptionBarrier();

hphp/runtime/vm/jit/ir.cpp

@@ -295,6 +295,24 @@ std::string showExtra(Opcode opc, const IRExtraData* data) {
 
 //////////////////////////////////////////////////////////////////////
 
+std::string BCMarker::show() const {
+  assert(valid());
+  return folly::format("--- bc {}, spOff {} ({})",
+                       bcOff,
+                       spOff,
+                       func->fullName()->data()).str();
+}
+
+bool BCMarker::valid() const {
+  return
+    func != nullptr &&
+    bcOff >= func->base() && bcOff < func->past() &&
+    spOff <= func->numLocals() +
+             func->numIterators() * kNumIterCells +
+             func->maxStackCells();
+}
+
+
 IRInstruction::IRInstruction(Arena& arena, const IRInstruction* inst, Id id)
   : m_op(inst->m_op)
   , m_typeParam(inst->m_typeParam)
@@ -303,6 +321,7 @@ IRInstruction::IRInstruction(Arena& arena, const IRInstruction* inst, Id id)
   , m_id(id)
   , m_srcs(m_numSrcs ? new (arena) SSATmp*[m_numSrcs] : nullptr)
   , m_dst(nullptr)
+  , m_marker(inst->m_marker)
   , m_extra(inst->m_extra ? cloneExtra(op(), inst->m_extra, arena)
                           : nullptr)
 {
@@ -773,7 +792,7 @@ bool isRefCounted(SSATmp* tmp) {
 }
 
 void IRInstruction::convertToNop() {
-  IRInstruction nop(Nop);
+  IRInstruction nop(Nop, marker());
   // copy all but m_id, m_taken, m_listNode
   m_op = nop.m_op;
   m_typeParam = nop.m_typeParam;
@@ -810,7 +829,7 @@ void IRInstruction::become(IRFactory* factory, IRInstruction* other) {
   assert(other->isTransient() || m_numDsts == other->m_numDsts);
   auto& arena = factory->arena();
 
-  // Copy all but m_id, m_taken.from, m_listNode, and don't clone
+  // Copy all but m_id, m_taken.from, m_listNode, m_marker, and don't clone
   // dests---the whole point of become() is things still point to us.
   m_op = other->m_op;
   m_typeParam = other->m_typeParam;

hphp/runtime/vm/jit/ir.h

@@ -432,7 +432,6 @@ O(DecRefMem,                        ND, S(PtrToGen)                           \
 O(DecRefNZ,                         ND, S(Gen),                      Mem|CRc) \
 O(DecRefNZOrBranch,                 ND, S(Gen),                      Mem|CRc) \
 O(DefLabel,                     DMulti, NA,                                E) \
-O(Marker,                           ND, NA,                                E) \
 O(DefInlineFP,             D(FramePtr), S(StkPtr) S(StkPtr),              NF) \
 O(InlineReturn,                     ND, S(FramePtr),                       E) \
 O(DefFP,                   D(FramePtr), NA,                                E) \

hphp/runtime/vm/jit/irfactory.cpp

@@ -20,8 +20,8 @@
 
 namespace HPHP {  namespace JIT {
 
-IRInstruction* IRFactory::defLabel(unsigned numDst) {
-  IRInstruction inst(DefLabel);
+IRInstruction* IRFactory::defLabel(unsigned numDst, BCMarker marker) {
+  IRInstruction inst(DefLabel, marker);
   IRInstruction* label = cloneInstruction(&inst);
   if (numDst > 0) {
     SSATmp* dsts = (SSATmp*) m_arena.alloc(numDst * sizeof(SSATmp));
@@ -37,15 +37,15 @@ Block* IRFactory::defBlock(const Func* func) {
   return new (m_arena) Block(m_nextBlockId++, func);
 }
 
-IRInstruction* IRFactory::mov(SSATmp* dst, SSATmp* src) {
-  IRInstruction* inst = gen(Mov, dst->type(), src);
+IRInstruction* IRFactory::mov(SSATmp* dst, SSATmp* src, BCMarker marker) {
+  IRInstruction* inst = gen(Mov, marker, dst->type(), src);
   dst->setInstruction(inst);
   inst->setDst(dst);
   return inst;
 }
 
 SSATmp* IRFactory::findConst(ConstData& cdata, Type ctype) {
-  IRInstruction inst(DefConst);
+  IRInstruction inst(DefConst, BCMarker());
   inst.setExtra(&cdata);
   inst.setTypeParam(ctype);
   if (SSATmp* tmp = m_constTable.lookup(&inst)) {

hphp/runtime/vm/jit/irfactory.h

@@ -49,20 +49,21 @@ struct InstructionBuilder {
 
   /*
    * Create an IRInstruction, and then recursively chew on the Args
-   * list to populate its fields.
+   * list to populate its fields. Every instruction must have at least
+   * an Opcode and a BCMarker.
    *
    * The IRInstruction is stack allocated, and should not escape the
    * lambda, so we fill it with 0xc0 in debug builds after we're done.
    */
   template<class... Args>
-  Ret go(Opcode op, Args&&... args) {
+  Ret go(Opcode op, BCMarker marker, Args&&... args) {
     std::aligned_storage<
       sizeof(IRInstruction)
     >::type buffer;
     void* const vpBuffer = &buffer;
     SCOPE_EXIT { if (debug) memset(&buffer, 0xc0, sizeof buffer); };
 
-    new (vpBuffer) IRInstruction(op);
+    new (vpBuffer) IRInstruction(op, marker);
     auto const inst = static_cast<IRInstruction*>(vpBuffer);
 
     SCOPE_EXIT { inst->clearExtra(); };
@@ -188,10 +189,12 @@ public:
    * This function takes arguments in the same format as gen().
    */
   template<class... Args>
-  void replace(IRInstruction* old, Args... args) {
+  void replace(IRInstruction* old, Opcode op, Args... args) {
     makeInstruction(
       [&] (IRInstruction* replacement) { old->become(this, replacement); },
-      args...
+      op,
+      old->marker(),
+      std::forward<Args>(args)...
     );
   }
 
@@ -220,22 +223,31 @@ public:
   /*
    * Some helpers for creating specific instruction patterns.
    */
-  IRInstruction* defLabel(unsigned numDst);
+  IRInstruction* defLabel(unsigned numDst, BCMarker marker);
   Block* defBlock(const Func* f);
+
   template<typename T> SSATmp* cns(T val) {
-    Type type = typeForConst(val);
+    return cns(val, typeForConst(val));
+  }
+
+  template<typename T> SSATmp* cns(T val, Type type) {
     // Normalize bool values to 0 or 1
     if (type.equals(Type::Bool)) val = (T)(val != 0);
     ConstData cdata(val);
     return findConst(cdata, type);
   }
 
+  SSATmp* cns(Type type) {
+    ConstData cdata(0);
+    return findConst(cdata, type);
+  }
+
   /*
    * Creates move instrution that moves from src to dst. We can't use gen
    * to create such a move because gen assigns a newly allocated destination
    * SSATmp whereas we want to use the given dst SSATmp.
    */
-  IRInstruction* mov(SSATmp* dst, SSATmp* src);
+  IRInstruction* mov(SSATmp* dst, SSATmp* src, BCMarker marker);
 
   Arena&   arena()               { return m_arena; }
   uint32_t numTmps() const       { return m_nextOpndId; }

hphp/runtime/vm/jit/irinstruction.h

@@ -23,6 +23,42 @@
 
 namespace HPHP { namespace JIT {
 
+/*
+ * BCMarker holds the location of a specific bytecode instruction, along with
+ * the offset from vmfp to vmsp at the beginning of the instruction. Every
+ * IRInstruction has one to keep track of which bytecode instruction it came
+ * from.
+ */
+struct BCMarker {
+  const Func* func;
+  Offset      bcOff;
+  int32_t     spOff;
+
+  BCMarker()
+    : func(nullptr)
+    , bcOff(0)
+    , spOff(0)
+  {}
+
+  BCMarker(const Func* f, Offset o, int32_t sp)
+    : func(f)
+    , bcOff(o)
+    , spOff(sp)
+  {
+    assert(valid());
+  }
+
+  bool operator==(BCMarker b) const {
+    return b.func == func &&
+      b.bcOff == bcOff &&
+      b.spOff == spOff;
+  }
+  bool operator!=(BCMarker b) const { return !operator==(b); }
+
+  std::string show() const;
+  bool valid() const;
+};
+
 /*
  * IRInstructions must be arena-allocatable.
  * (Destructors are not called when they come from IRFactory.)
@@ -37,6 +73,7 @@ struct IRInstruction {
    * TraceBuilder rather than directly.
    */
   explicit IRInstruction(Opcode op,
+                         BCMarker marker,
                          uint32_t numSrcs = 0,
                          SSATmp** srcs = nullptr)
     : m_op(op)
@@ -46,8 +83,15 @@ struct IRInstruction {
     , m_id(kTransient)
     , m_srcs(srcs)
     , m_dst(nullptr)
+    , m_marker(marker)
     , m_extra(nullptr)
-  {}
+  {
+    if (op != DefConst) {
+      // DefConst is the only opcode that's allowed to not have a marker, since
+      // it's not part of the instruction stream.
+      assert(m_marker.valid());
+    }
+  }
 
   IRInstruction(const IRInstruction&) = delete;
   IRInstruction& operator=(const IRInstruction&) = delete;
@@ -260,6 +304,14 @@ struct IRInstruction {
   bool cseEquals(IRInstruction* inst) const;
   size_t cseHash() const;
 
+  void setMarker(BCMarker marker) {
+    assert(marker.valid());
+    m_marker = marker;
+  }
+  const BCMarker& marker() const {
+    return m_marker;
+  }
+
   std::string toString() const;
 
   /*
@@ -307,6 +359,7 @@ private:
   SSATmp**          m_srcs;
   SSATmp*           m_dst;     // if HasDest or NaryDest
   Edge              m_taken;   // for branches, guards, and jmp
+  BCMarker          m_marker;
   IRExtraData*      m_extra;
 public:
   boost::intrusive::list_member_hook<> m_listNode; // for InstructionList

hphp/runtime/vm/jit/irtranslator.cpp

@@ -1628,13 +1628,9 @@ TranslatorX64::irTranslateTracelet(Tracelet& t) {
         m_curNI = ni;
         translateInstr(*ni);
       } catch (JIT::FailedIRGen& fcg) {
-        // If we haven't tried interpreting ni yet, flag it to be interpreted
-        // and retry
-        if (!ni->interp) {
-          ni->interp = true;
-          return Retry;
-        }
-        throw fcg;
+        always_assert(!ni->interp);
+        ni->interp = true;
+        return Retry;
       }
       assert(ni->source.offset() >= curFunc()->base());
       // We sometimes leave the tail of a truncated tracelet in place to aid
@@ -1653,14 +1649,12 @@ TranslatorX64::irTranslateTracelet(Tracelet& t) {
       // problem, flag it to be interpreted, and retranslate the tracelet.
       for (auto ni = t.m_instrStream.first; ni; ni = ni->next) {
         if (ni->source.offset() == fcg.bcOff) {
-          if (!ni->interp) {
-            ni->interp = true;
-            TRACE(1, "HHIR: RETRY Translation %d: will interpOne BC instr %s "
-                  "after failing to code-gen \n\n",
-                  getCurrentTransID(), ni->toString().c_str());
-            return Retry;
-          }
-          break;
+          always_assert(!ni->interp);
+          ni->interp = true;
+          TRACE(1, "HHIR: RETRY Translation %d: will interpOne BC instr %s "
+                "after failing to code-gen \n\n",
+                getCurrentTransID(), ni->toString().c_str());
+          return Retry;
         }
       }
       throw fcg;

hphp/runtime/vm/jit/jumpopts.cpp

@@ -34,26 +34,14 @@ namespace {
 // reached by any other branch, then copy the target of the jump to the
 // end of the trace
 void elimUnconditionalJump(IRTrace* trace, IRFactory* irFactory) {
-  boost::dynamic_bitset<> isJoin(irFactory->numBlocks());
-  boost::dynamic_bitset<> havePred(irFactory->numBlocks());
-  for (Block* block : trace->blocks()) {
-    if (block->taken()) {
-      auto id = block->taken()->id();
-      isJoin[id] = havePred[id];
-      havePred[id] = 1;
-    }
-    if (block->next()) {
-      auto id = block->next()->id();
-      isJoin[id] = havePred[id];
-      havePred[id] = 1;
-    }
-  }
   Block* lastBlock = trace->back();
   auto lastInst = lastBlock->backIter(); // iterator to last instruction
   IRInstruction& jmp = *lastInst;
-  if (jmp.op() == Jmp_ && !isJoin[jmp.taken()->id()]) {
+  if (jmp.op() == Jmp_ && jmp.taken()->numPreds() == 1) {
     Block* target = jmp.taken();
-    lastBlock->splice(lastInst, target, target->skipHeader(), target->end());
+    lastBlock->splice(lastInst, target, target->skipHeader(), target->end(),
+                      lastInst->marker());
+    jmp.convertToNop();         // unlink it from its Edge
     lastBlock->erase(lastInst); // delete the jmp
   }
 }
@@ -99,7 +87,6 @@ struct BlockMatcher {
 
   template<class... Opcodes>
   bool match(Opcode op, Opcodes... opcs) {
-    while (m_it != m_block->end() && m_it->op() == Marker) ++m_it;
     if (m_it == m_block->end()) return false;
     auto const cur = m_it->op();
     ++m_it;
@@ -156,19 +143,26 @@ void optimizeCondTraceExit(IRTrace* trace, IRFactory* irFactory) {
   if (!isNormalExit(jccExitTrace)) return;
   FTRACE(5, "exit trace is side-effect free\n");
 
+  auto it = mainExit->backIter();
+  auto& reqBindJmp = *(it--);
+  auto& syncAbi = *it;
+  assert(syncAbi.op() == SyncABIRegs);
+
   auto const newOpcode = jmpToReqBindJmp(jccBlock->back()->op());
   ReqBindJccData data;
   data.taken = jccExitTrace->back()->extra<ReqBindJmp>()->offset;
-  data.notTaken = mainExit->back()->extra<ReqBindJmp>()->offset;
+  data.notTaken = reqBindJmp.extra<ReqBindJmp>()->offset;
 
   FTRACE(5, "replacing {} with {}\n", jccInst->id(), opcodeName(newOpcode));
   irFactory->replace(
-    mainExit->back(),
+    &reqBindJmp,
     newOpcode,
     data,
     std::make_pair(jccInst->numSrcs(), jccInst->srcs().begin())
   );
 
+  syncAbi.setMarker(jccInst->marker());
+  reqBindJmp.setMarker(jccInst->marker());
   jccInst->convertToNop();
 }
 

hphp/runtime/vm/jit/linearscan.cpp

@@ -396,7 +396,8 @@ void LinearScan::allocRegToInstruction(InstructionList::iterator it) {
 
       // Insert the Reload instruction.
       SSATmp* spillTmp = m_slots[slotId].spillTmp;
-      IRInstruction* reload = m_irFactory->gen(Reload, spillTmp);
+      IRInstruction* reload = m_irFactory->gen(Reload, inst->marker(),
+                                               spillTmp);
       inst->block()->insert(it, reload);
 
       // Create <reloadTmp> which inherits <tmp>'s slot ID and
@@ -979,7 +980,6 @@ void LinearScan::numberInstructions(const BlockList& blocks) {
   uint32_t nextId = 1;
   for (auto* block : blocks) {
     for (auto& inst : *block) {
-      if (inst.op() == Marker) continue; // don't number markers
       uint32_t id = nextId++;
       m_linear[inst] = id;
       for (SSATmp* tmp : inst.srcs()) {
@@ -1029,27 +1029,18 @@ void LinearScan::genSpillStats(IRTrace* trace, int numSpillLocs) {
   static StringData* exitSpills = StringData::GetStaticString("ExitSpills");
   static StringData* exitReloads = StringData::GetStaticString("ExitReloads");
   static StringData* spillSpace = StringData::GetStaticString("SpillSpace");
-  trace->front()->prepend(m_irFactory->gen(
-                            IncStatGrouped,
-                            cns(spillStats),
-                            cns(mainSpills), cns(numMainSpills)));
-  trace->front()->prepend(m_irFactory->gen(
-                            IncStatGrouped,
-                            cns(spillStats),
-                            cns(mainReloads), cns(numMainReloads)));
-  trace->front()->prepend(m_irFactory->gen(
-                            IncStatGrouped,
-                            cns(spillStats),
-                            cns(exitSpills), cns(numExitSpills)));
-  trace->front()->prepend(m_irFactory->gen(
-                            IncStatGrouped,
-                            cns(spillStats),
-                            cns(exitReloads), cns(numExitReloads)));
-  trace->front()->prepend(m_irFactory->gen(
-                            IncStatGrouped,
-                            cns(spillStats),
-                            cns(spillSpace), cns(numSpillLocs)));
 
+  auto const marker = trace->front()->front()->marker();
+  auto addStat = [&](const StringData* key, int value) {
+    trace->front()->prepend(m_irFactory->gen(IncStatGrouped, marker,
+                                             cns(spillStats), cns(key),
+                                             cns(value)));
+  };
+  addStat(mainSpills, numMainSpills);
+  addStat(mainReloads, numMainReloads);
+  addStat(exitSpills, numExitSpills);
+  addStat(exitReloads, numExitReloads);
+  addStat(spillSpace, numSpillLocs);
 }
 
 /*
@@ -1205,10 +1196,6 @@ void LinearScan::allocRegsOneTrace(BlockList::iterator& blockIt,
       block->next()->prepend(spill);
     } else {
       auto pos = block->iteratorTo(inst);
-      if (inst->op() == DefLabel) {
-        ++pos;
-        assert(pos != block->end() && pos->op() == Marker);
-      }
       block->insert(++pos, spill);
     }
   }
@@ -1390,7 +1377,7 @@ void LinearScan::spill(SSATmp* tmp) {
 uint32_t LinearScan::createSpillSlot(SSATmp* tmp) {
   uint32_t slotId = m_slots.size();
   m_spillSlots[tmp] = slotId;
-  IRInstruction* spillInst = m_irFactory->gen(Spill, tmp);
+  auto* spillInst = m_irFactory->gen(Spill, tmp->inst()->marker(), tmp);
   SSATmp* spillTmp = spillInst->dst();
   SlotInfo si;
   si.spillTmp = spillTmp;

hphp/runtime/vm/jit/opt.cpp

@@ -30,10 +30,8 @@ static void insertAfter(IRInstruction* definer, IRInstruction* inst) {
   auto pos = block->iteratorTo(definer);
   if (pos->op() == DefLabel) {
     ++pos;
-    assert(pos != block->end() && pos->op() == Marker);
   }
-  ++pos;
-  block->insert(pos, inst);
+  block->insert(++pos, inst);
 }
 
 /*
@@ -45,7 +43,7 @@ static void insertRefCountAsserts(IRInstruction& inst, IRFactory* factory) {
   for (SSATmp& dst : inst.dsts()) {
     Type t = dst.type();
     if (t.subtypeOf(Type::Counted | Type::StaticStr | Type::StaticArr)) {
-      insertAfter(&inst, factory->gen(DbgAssertRefCount, &dst));
+      insertAfter(&inst, factory->gen(DbgAssertRefCount, inst.marker(), &dst));
     }
   }
 }
@@ -63,11 +61,13 @@ static void insertSpillStackAsserts(IRInstruction& inst, IRFactory* factory) {
     Type t = vals[i]->type();
     if (t.subtypeOf(Type::Gen)) {
       IRInstruction* addr = factory->gen(LdStackAddr,
+                                         inst.marker(),
                                          Type::PtrToGen,
                                          StackOffset(i),
                                          sp);
       block->insert(pos, addr);
-      IRInstruction* check = factory->gen(DbgAssertPtr, addr->dst());
+      IRInstruction* check = factory->gen(DbgAssertPtr, inst.marker(),
+                                          addr->dst());
       block->insert(pos, check);
     }
   }
@@ -89,11 +89,13 @@ static void insertAsserts(IRTrace* trace, IRFactory* factory) {
       if (inst.op() == Call) {
         SSATmp* sp = inst.dst();
         IRInstruction* addr = factory->gen(LdStackAddr,
+                                           inst.marker(),
                                            Type::PtrToGen,
                                            StackOffset(0),
                                            sp);
         insertAfter(&inst, addr);
-        insertAfter(addr, factory->gen(DbgAssertPtr, addr->dst()));
+        insertAfter(addr, factory->gen(DbgAssertPtr, inst.marker(),
+                                       addr->dst()));
         continue;
       }
       if (!inst.isBlockEnd()) insertRefCountAsserts(inst, factory);

hphp/runtime/vm/jit/predictionopts.cpp

@@ -41,7 +41,6 @@ bool instructionsAreSinkable(InputIterator first, InputIterator last) {
     case ReDefGeneratorSP:
     case DecRef:
     case DecRefNZ:
-    case Marker:
     case IncRef:
     case LdMem:
       return true;
@@ -81,8 +80,7 @@ void optimizePredictions(IRTrace* const trace, IRFactory* const irFactory) {
    * generic LdMem/IncRef on the exit block, otherwise we do
    * type-specialized versions.
    */
-  auto optLdMem = [&] (IRInstruction* checkType,
-                       IRInstruction* lastMarker) -> bool {
+  auto optLdMem = [&] (IRInstruction* checkType) -> bool {
     auto const incRef = checkType->src(0)->inst();
     if (incRef->op() != IncRef) return false;
     auto const ldMem = incRef->src(0)->inst();
@@ -116,6 +114,7 @@ void optimizePredictions(IRTrace* const trace, IRFactory* const irFactory) {
      */
     auto const newCheckType = irFactory->gen(
       CheckTypeMem,
+      checkType->marker(),
       checkType->typeParam(),
       checkType->taken(),
       ldMem->src(0)
@@ -124,7 +123,6 @@ void optimizePredictions(IRTrace* const trace, IRFactory* const irFactory) {
 
     // Clone the instructions to the exit before specializing.
     cloneToBlock(rpoSort, irFactory, sinkFirst, sinkLast, exit);
-    exit->insert(exit->skipHeader(), irFactory->cloneInstruction(lastMarker));
 
     /*
      * Specialize the LdMem left on the main trace after cloning the
@@ -146,8 +144,6 @@ void optimizePredictions(IRTrace* const trace, IRFactory* const irFactory) {
 
     // Move the fallthrough case to specialized.
     moveToBlock(sinkFirst, boost::next(sinkLast), specialized);
-    specialized->insert(specialized->skipHeader(),
-                        irFactory->cloneInstruction(lastMarker));
 
     return true;
   };
@@ -163,17 +159,10 @@ void optimizePredictions(IRTrace* const trace, IRFactory* const irFactory) {
   if (!trace->isMain()) return;
   bool needsReflow = false;
   for (Block* b : trace->blocks()) {
-    IRInstruction* lastMarker = nullptr;
     for (auto& inst : *b) {
-      if (inst.op() == Marker) {
-        lastMarker = &inst;
-        continue;
-      }
-
       if (inst.op() == CheckType &&
           inst.src(0)->type().equals(Type::Cell)) {
-        assert(lastMarker);
-        if (optLdMem(&inst, lastMarker)) {
+        if (optLdMem(&inst)) {
           needsReflow = true;
           break;
         }

hphp/runtime/vm/jit/print.cpp

@@ -129,14 +129,6 @@ void printLabel(std::ostream& os, const Block* block) {
 
 void print(std::ostream& ostream, const IRInstruction* inst,
            const RegAllocInfo* regs, const LifetimeInfo* lifetime) {
-  if (inst->op() == Marker) {
-    auto* marker = inst->extra<Marker>();
-    ostream << color(ANSI_COLOR_BLUE)
-            << marker->show()
-            << color(ANSI_COLOR_END);
-    return;
-  }
-
   if (!inst->isTransient()) {
     ostream << color(ANSI_COLOR_YELLOW);
     if (!lifetime || !lifetime->linear[inst]) {
@@ -313,47 +305,62 @@ void print(std::ostream& os, const IRTrace* trace, const RegAllocInfo* regs,
                                  .printEncoding(dumpIREnabled(kExtraLevel))
                                  .color(color(ANSI_COLOR_BROWN)));
 
+  BCMarker curMarker;
   for (Block* block : blocks(trace, asmInfo)) {
     if (!block->isMain()) {
       os << "\n" << color(ANSI_COLOR_GREEN)
          << "    -------  Exit Trace  -------"
          << color(ANSI_COLOR_END) << '\n';
+      curMarker = BCMarker();
     }
 
     TcaRange blockRange = asmInfo ? asmInfo->asmRanges[block] :
                           TcaRange(nullptr, nullptr);
 
-    os << std::string(kIndent - 2, ' ');
+    os << '\n' << std::string(kIndent - 3, ' ');
     printLabel(os, block);
     os << punc(":") << "\n";
 
+    const char* markerEndl = "";
     for (auto it = block->begin(); it != block->end();) {
       auto& inst = *it; ++it;
 
-      if (inst.op() == Marker) {
-        os << std::string(kIndent, ' ');
-        JIT::print(os, &inst, regs, lifetime);
-        os << '\n';
+      if (inst.marker() != curMarker) {
+        std::ostringstream mStr;
+        auto const& newMarker = inst.marker();
+        auto func = newMarker.func;
+        if (!func) {
+          os << color(ANSI_COLOR_BLUE)
+             << std::string(kIndent, ' ')
+             << "--- invalid marker"
+             << color(ANSI_COLOR_END)
+             << '\n';
+        } else {
+          if (func != curMarker.func) {
+            func->prettyPrint(mStr);
+          }
+          mStr << std::string(kIndent, ' ')
+               << newMarker.show()
+               << '\n';
 
-        // Don't print bytecode in a non-main trace.
-        if (!trace->isMain()) continue;
-
-        auto* marker = inst.extra<Marker>();
-        uint32_t bcOffset = marker->bcOff;
-        if (const auto* func = marker->func) {
-          std::ostringstream uStr;
+          auto bcOffset = newMarker.bcOff;
           func->unit()->prettyPrint(
-            uStr, Unit::PrintOpts()
-                  .range(bcOffset, bcOffset+1)
-                  .noLineNumbers()
-                  .indent(0));
+            mStr, Unit::PrintOpts()
+                       .range(bcOffset, bcOffset+1)
+                       .noLineNumbers()
+                       .noFuncs()
+                       .indent(0));
           std::vector<std::string> vec;
-          folly::split('\n', uStr.str(), vec);
+          folly::split('\n', mStr.str(), vec);
+          os << markerEndl;
+          markerEndl = "\n";
           for (auto& s : vec) {
+            if (s.empty()) continue;
             os << color(ANSI_COLOR_BLUE) << s << color(ANSI_COLOR_END) << '\n';
           }
-          continue;
         }
+
+        curMarker = newMarker;
       }
 
       if (inst.op() == DefLabel) {
@@ -410,6 +417,15 @@ void print(std::ostream& os, const IRTrace* trace, const RegAllocInfo* regs,
         os << '\n';
       }
     }
+
+    os << std::string(kIndent - 2, ' ');
+    if (auto next = block->next()) {
+      os << punc("-> ");
+      printLabel(os, next);
+      os << '\n';
+    } else {
+      os << "no fallthrough\n";
+    }
   }
 }
 

hphp/runtime/vm/jit/simplifier.cpp

@@ -251,13 +251,25 @@ template<class... Args> SSATmp* Simplifier::cns(Args&&... cns) {
   return m_tb->cns(std::forward<Args>(cns)...);
 }
 
-template<class... Args> SSATmp* Simplifier::gen(Args&&... args) {
-  return m_tb->gen(std::forward<Args>(args)...);
+template<class... Args> SSATmp* Simplifier::gen(Opcode op, Args&&... args) {
+  assert(!m_insts.empty());
+  return m_tb->gen(op, m_insts.top()->marker(), std::forward<Args>(args)...);
+}
+
+template<class... Args> SSATmp* Simplifier::gen(Opcode op, BCMarker marker,
+                                                Args&&... args) {
+  return m_tb->gen(op, marker, std::forward<Args>(args)...);
 }
 
 //////////////////////////////////////////////////////////////////////
 
 SSATmp* Simplifier::simplify(IRInstruction* inst) {
+  m_insts.push(inst);
+  SCOPE_EXIT {
+    assert(m_insts.top() == inst);
+    m_insts.pop();
+  };
+
   SSATmp* src1 = inst->src(0);
   SSATmp* src2 = inst->src(1);
 
@@ -561,6 +573,7 @@ SSATmp* Simplifier::simplifyQueryJmp(IRInstruction* inst) {
       return nullptr;
     },
     JmpNZero,
+    inst->marker(),
     inst->taken(),
     newCmp);
   if (!newQueryJmp) return nullptr;
@@ -1617,6 +1630,7 @@ SSATmp* Simplifier::simplifyCondJmp(IRInstruction* inst) {
         inst->op() == JmpZero
           ? negateQueryOp(srcOpcode)
           : srcOpcode),
+      srcInst->marker(),
       srcInst->typeParam(), // if it had a type param
       inst->taken(),
       std::make_pair(ssas.size(), ssas.begin())

hphp/runtime/vm/jit/simplifier.h

@@ -136,10 +136,16 @@ private:
 
 private: // tracebuilder forwarders
   template<class... Args> SSATmp* cns(Args&&...);
-  template<class... Args> SSATmp* gen(Args&&...);
+  template<class... Args> SSATmp* gen(Opcode op, Args&&...);
+  template<class... Args> SSATmp* gen(Opcode op, BCMarker marker, Args&&...);
 
 private:
   TraceBuilder* const m_tb;
+
+  // The current instruction being simplified is always at
+  // m_insts.top(). This has to be a stack instead of just a pointer
+  // because simplify is reentrant.
+  smart::stack<const IRInstruction*> m_insts;
 };
 
 //////////////////////////////////////////////////////////////////////

hphp/runtime/vm/jit/trace.h

@@ -59,11 +59,12 @@ struct IRTrace : private boost::noncopyable {
         iterator end()          { return blocks().end(); }
 
   /*
-   * Unlink a block from a trace.  Updates any successor blocks that
+   * Unlink a block from a trace. Updates any successor blocks that
    * have a DefLabel with a dest depending on this block.
    */
   iterator erase(iterator it) {
     Block* b = *it;
+    assert(b->preds().empty());
     it = m_blocks.erase(it);
     b->setTrace(nullptr);
     if (!b->empty()) b->back()->setTaken(nullptr);
@@ -95,11 +96,13 @@ struct IRTrace : private boost::noncopyable {
     return exit;
   }
   bool isMain() const { return m_main == nullptr; }
+  /*
+   * Catch traces always start with DefLabel; BeginCatch.
+   */
   bool isCatch() const {
-    auto it = front()->skipHeader();
-    if (it == front()->end()) return false;
+    if (front()->empty()) return false;
 
-    assert(it->op() == Marker);
+    auto it = front()->skipHeader();
     if (it == front()->begin()) return false;
 
     return (--it)->op() == BeginCatch;

hphp/runtime/vm/jit/tracebuilder.cpp

@@ -32,7 +32,9 @@ TraceBuilder::TraceBuilder(Offset initialBcOffset,
                            const Func* func)
   : m_irFactory(irFactory)
   , m_simplifier(this)
-  , m_trace(makeTrace(func, initialBcOffset))
+  , m_mainTrace(makeTrace(func, initialBcOffset))
+  , m_curTrace(m_mainTrace.get())
+  , m_curBlock(nullptr)
   , m_enableCse(false)
   , m_enableSimplification(false)
   , m_snapshots(&irFactory, nullptr)
@@ -44,7 +46,7 @@ TraceBuilder::TraceBuilder(Offset initialBcOffset,
   , m_localValues(func->numLocals(), nullptr)
   , m_localTypes(func->numLocals(), Type::None)
 {
-  m_curFunc = cns(func);
+  m_curFunc = m_irFactory.cns(func);
   if (RuntimeOption::EvalHHIRGenOpts) {
     m_enableCse = RuntimeOption::EvalHHIRCse;
     m_enableSimplification = RuntimeOption::EvalHHIRSimplification;
@@ -158,6 +160,9 @@ void TraceBuilder::trackInlineReturn(IRInstruction* inst) {
 }
 
 void TraceBuilder::updateTrackedState(IRInstruction* inst) {
+  // We don't track state for any trace other than the main trace.
+  if (m_savedTraces.size() > 0) return;
+
   Opcode opc = inst->op();
   // Update tracked state of local values/types, stack/frame pointer, CSE, etc.
 
@@ -170,10 +175,6 @@ void TraceBuilder::updateTrackedState(IRInstruction* inst) {
   case DefInlineFP:    trackDefInlineFP(inst);  break;
   case InlineReturn:   trackInlineReturn(inst); break;
 
-  case Marker:
-    m_lastMarker = *inst->extra<Marker>();
-    break;
-
   case Call:
     m_spValue = inst->dst();
     // A call pops the ActRec and pushes a return value.
@@ -372,7 +373,8 @@ std::unique_ptr<TraceBuilder::State> TraceBuilder::createState() const {
   state->localTypes = m_localTypes;
   state->callerAvailableValues = m_callerAvailableValues;
   state->refCountedMemValue = m_refCountedMemValue;
-  state->lastMarker = *m_lastMarker;
+  state->curMarker = m_curMarker;
+  assert(state->curMarker.valid());
   return state;
 }
 
@@ -436,10 +438,7 @@ void TraceBuilder::mergeState(State* state) {
 
   // We should not be merging states that have different hhbc bytecode
   // boundaries.
-  assert(m_lastMarker &&
-    state->lastMarker.bcOff    == m_lastMarker->bcOff &&
-    state->lastMarker.stackOff == m_lastMarker->stackOff &&
-    state->lastMarker.func     == m_lastMarker->func);
+  assert(m_curMarker.valid() && state->curMarker == m_curMarker);
 }
 
 void TraceBuilder::useState(std::unique_ptr<State> state) {
@@ -452,7 +451,7 @@ void TraceBuilder::useState(std::unique_ptr<State> state) {
   m_localValues = std::move(state->localValues);
   m_localTypes = std::move(state->localTypes);
   m_callerAvailableValues = std::move(state->callerAvailableValues);
-  m_lastMarker = state->lastMarker;
+  m_curMarker = state->curMarker;
   // If spValue is null, we merged two different but equivalent values.
   // Define a new sp using the known-good spOffset.
   if (!m_spValue) {
@@ -488,10 +487,11 @@ void TraceBuilder::clearTrackedState() {
     delete *i;
     *i = nullptr;
   }
-  m_lastMarker = folly::none;
+  m_curMarker = BCMarker();
 }
 
 void TraceBuilder::appendInstruction(IRInstruction* inst, Block* block) {
+  assert(inst->marker().valid());
   Opcode opc = inst->op();
   if (opc != Nop && opc != DefConst) {
     block->push_back(inst);
@@ -499,20 +499,27 @@ void TraceBuilder::appendInstruction(IRInstruction* inst, Block* block) {
 }
 
 void TraceBuilder::appendInstruction(IRInstruction* inst) {
-  Block* block = m_trace->back();
+  if (m_curWhere) {
+    // We have a specific position to insert instructions.
+    assert(!inst->isBlockEnd());
+    auto& it = m_curWhere.get();
+    it = m_curBlock->insert(it, inst);
+    ++it;
+    return;
+  }
+
+  Block* block = m_curTrace->back();
   if (!block->empty()) {
     IRInstruction* prev = block->back();
     if (prev->isBlockEnd()) {
       // start a new block
       Block* next = m_irFactory.defBlock(m_curFunc->getValFunc());
-      m_trace->push_back(next);
+      m_curTrace->push_back(next);
       if (!prev->isTerminal()) {
         // new block is reachable from old block so link it.
         block->setNext(next);
       }
       block = next;
-      assert(m_lastMarker.hasValue());
-      gen(Marker, *m_lastMarker);
     }
   }
   appendInstruction(inst, block);
@@ -520,14 +527,12 @@ void TraceBuilder::appendInstruction(IRInstruction* inst) {
 }
 
 void TraceBuilder::appendBlock(Block* block) {
-  if (!m_trace->back()->back()->isTerminal()) {
+  if (!m_curTrace->back()->back()->isTerminal()) {
     // previous instruction falls through; merge current state with block.
     saveState(block);
   }
-  m_trace->push_back(block);
+  m_curTrace->push_back(block);
   useState(block);
-  assert(m_lastMarker.hasValue());
-  gen(Marker, *m_lastMarker);
 }
 
 CSEHash* TraceBuilder::cseHashTable(IRInstruction* inst) {
@@ -761,6 +766,10 @@ SSATmp* TraceBuilder::preOptimize(IRInstruction* inst) {
 
 SSATmp* TraceBuilder::optimizeWork(IRInstruction* inst,
                                    const folly::Optional<IdomVector>& idoms) {
+  // Since some of these optimizations inspect tracked state, we don't
+  // perform any of them on non-main traces.
+  if (m_savedTraces.size() > 0) return nullptr;
+
   static DEBUG_ONLY __thread int instNest = 0;
   if (debug) ++instNest;
   SCOPE_EXIT { if (debug) --instNest; };
@@ -852,30 +861,32 @@ SSATmp* TraceBuilder::optimizeInst(IRInstruction* inst, CloneFlag doClone) {
 void TraceBuilder::reoptimize() {
   FTRACE(5, "ReOptimize:vvvvvvvvvvvvvvvvvvvv\n");
   SCOPE_EXIT { FTRACE(5, "ReOptimize:^^^^^^^^^^^^^^^^^^^^\n"); };
+  assert(m_curTrace == m_mainTrace.get());
+  assert(m_savedTraces.size() == 0);
 
   m_enableCse = RuntimeOption::EvalHHIRCse;
   m_enableSimplification = RuntimeOption::EvalHHIRSimplification;
   if (!m_enableCse && !m_enableSimplification) return;
-  if (m_trace->blocks().size() >
+  if (m_mainTrace->blocks().size() >
       RuntimeOption::EvalHHIRSimplificationMaxBlocks) {
     // TODO CSEHash::filter is very slow for large block sizes
     // t2135219 should address that
     return;
   }
 
-  BlockList sortedBlocks = rpoSortCfg(m_trace.get(), m_irFactory);
+  BlockList sortedBlocks = rpoSortCfg(m_mainTrace.get(), m_irFactory);
   auto const idoms = findDominators(sortedBlocks);
   clearTrackedState();
 
-  auto blocks = std::move(m_trace->blocks());
-  assert(m_trace->blocks().empty());
+  auto blocks = std::move(m_mainTrace->blocks());
+  assert(m_mainTrace->blocks().empty());
   while (!blocks.empty()) {
     Block* block = blocks.front();
     blocks.pop_front();
-    assert(block->trace() == m_trace.get());
+    assert(block->trace() == m_mainTrace.get());
     FTRACE(5, "Block: {}\n", block->id());
 
-    m_trace->push_back(block);
+    m_mainTrace->push_back(block);
     if (m_snapshots[block]) {
       useState(block);
     }
@@ -886,6 +897,12 @@ void TraceBuilder::reoptimize() {
       auto *inst = &instructions.front();
       instructions.pop_front();
 
+      // merging state looks at the current marker, and optimizeWork
+      // below may create new instructions. Use the marker from this
+      // instruction.
+      assert(inst->marker().valid());
+      setMarker(inst->marker());
+
       auto const tmp = optimizeWork(inst, idoms); // Can generate new instrs!
       if (!tmp) {
         // Could not optimize; keep the old instruction
@@ -899,8 +916,8 @@ void TraceBuilder::reoptimize() {
         // Generate a mov(tmp->dst) to get result into dst. If we get here then
         // assume the last instruction in the block isn't a guard. If it was,
         // we would have to insert the mov on the fall-through edge.
-        assert(!block->back()->isBlockEnd());
-        IRInstruction* mov = m_irFactory.mov(dst, tmp);
+        assert(block->empty() || !block->back()->isBlockEnd());
+        IRInstruction* mov = m_irFactory.mov(dst, tmp, inst->marker());
         appendInstruction(mov, block);
         updateTrackedState(mov);
       }
@@ -1020,4 +1037,41 @@ void TraceBuilder::killLocals() {
   }
 }
 
+void TraceBuilder::setMarker(BCMarker marker) {
+  if (m_curMarker == marker) return;
+  FTRACE(2, "TraceBuilder changing current marker from {} to {}\n",
+         m_curMarker.func ? m_curMarker.show() : "<invalid>", marker.show());
+  assert(marker.valid());
+  m_curMarker = marker;
+}
+
+void TraceBuilder::pushTrace(IRTrace* t, BCMarker marker, Block* b,
+                             const boost::optional<Block::iterator>& where) {
+  FTRACE(2, "TraceBuilder saving {}@{} and using {}@{}\n",
+         m_curTrace, m_curMarker.show(), t, marker.show());
+  assert(t);
+  assert(bool(b) == bool(where));
+  assert(IMPLIES(b, b->trace() == t));
+
+  m_savedTraces.push(
+    TraceState{ m_curTrace, m_curBlock, m_curMarker, m_curWhere });
+  m_curTrace = t;
+  m_curBlock = b;
+  setMarker(marker);
+  m_curWhere = where;
+}
+
+void TraceBuilder::popTrace() {
+  assert(!m_savedTraces.empty());
+
+  auto const& top = m_savedTraces.top();
+  FTRACE(2, "TraceBuilder popping {}@{} to restore {}@{}\n",
+         m_curTrace, m_curMarker.show(), top.trace, top.marker.show());
+  m_curTrace = top.trace;
+  m_curBlock = top.block;
+  setMarker(top.marker);
+  m_curWhere = top.where;
+  m_savedTraces.pop();
+}
+
 }}

hphp/runtime/vm/jit/tracebuilder.h

@@ -98,7 +98,7 @@ struct TraceBuilder {
   void setEnableCse(bool val)            { m_enableCse = val; }
   void setEnableSimplification(bool val) { m_enableSimplification = val; }
 
-  IRTrace* trace() const { return m_trace.get(); }
+  IRTrace* trace() const { return m_curTrace; }
   IRFactory* factory() { return &m_irFactory; }
   int32_t spOffset() { return m_spOffset; }
   SSATmp* sp() const { return m_spValue; }
@@ -115,6 +115,33 @@ struct TraceBuilder {
   SSATmp* getLocalValue(unsigned id) const;
   void setLocalValue(unsigned id, SSATmp* value);
 
+  /*
+   * Updates the marker used for instructions generated without one
+   * supplied.
+   */
+  void setMarker(BCMarker marker);
+
+  /*
+   * To emit code to a trace other than the main trace, call pushTrace(), emit
+   * instructions as usual with gen(...), then, call popTrace(). This is best
+   * done using the TracePusher struct:
+   *
+   * gen(CodeForMainTrace, ...);
+   * {
+   *   TracePusher tp(m_tb, exitTrace, marker);
+   *   gen(CodeForExitTrace, ...);
+   * }
+   * gen(CodeForMainTrace, ...);
+   *
+   * b and where may be supplied to emit code to a specific location in the
+   * trace. b must be nullptr iff where is boost::none, and where (if present)
+   * must be an iterator to somewhere in b's InstructionList. Instructions will
+   * be inserted at where, before the instruction it currently points to.
+   */
+  void pushTrace(IRTrace* t, BCMarker marker, Block* b,
+                 const boost::optional<Block::iterator>& where);
+  void popTrace();
+
   /*
    * Run another pass of TraceBuilder-managed optimizations on this
    * trace.
@@ -122,35 +149,27 @@ struct TraceBuilder {
   void reoptimize();
 
   /*
-   * Create an IRInstruction attached to the current main Trace, and
+   * Create an IRInstruction attached to the current IRTrace, and
    * allocate a destination SSATmp for it.  Uses the same argument
    * list format as IRFactory::gen.
    */
   template<class... Args>
-  SSATmp* gen(Args&&... args) {
+  SSATmp* gen(Opcode op, Args&&... args) {
+    return gen(op, m_curMarker, std::forward<Args>(args)...);
+  }
+
+  template<class... Args>
+  SSATmp* gen(Opcode op, BCMarker marker, Args&&... args) {
     return makeInstruction(
       [this] (IRInstruction* inst) {
         return optimizeInst(inst, CloneFlag::Yes);
       },
+      op,
+      marker,
       std::forward<Args>(args)...
     );
   }
 
-  /*
-   * Create an IRInstruction, similar to gen(), except link it into
-   * the Trace t instead of the current main trace.
-   *
-   * Does not run optimization passes.
-   *
-   * TODO(#2404447): run simplifier?
-   */
-  template<class... Args>
-  SSATmp* genFor(IRTrace* t, Args&&... args) {
-    auto instr = m_irFactory.gen(std::forward<Args>(args)...);
-    t->back()->push_back(instr);
-    return instr->dst();
-  }
-
   /*
    * Add an already created instruction, running it through the normal
    * optimization passes and updating tracked state.
@@ -169,18 +188,9 @@ struct TraceBuilder {
   SSATmp* genPtrToUninit();
   SSATmp* genDefNone();
 
-  template<typename T>
-  SSATmp* cns(T val) {
-    return gen(DefConst, typeForConst(val), ConstData(val));
-  }
-
-  template<typename T>
-  SSATmp* cns(T val, Type type) {
-    return gen(DefConst, type, ConstData(val));
-  }
-
-  SSATmp* cns(Type type) {
-    return gen(DefConst, type, ConstData(0));
+  template<class... Args>
+  SSATmp* cns(Args&&... args) {
+    return m_irFactory.cns(std::forward<Args>(args)...);
   }
 
   template<typename T>
@@ -193,7 +203,7 @@ struct TraceBuilder {
 
   // hint the execution frequency of the current block
   void hint(Block::Hint h) const {
-    m_trace->back()->setHint(h);
+    m_curTrace->back()->setHint(h);
   }
 
   /*
@@ -206,7 +216,7 @@ struct TraceBuilder {
   SSATmp* cond(const Func* func, Branch branch, Next next, Taken taken) {
     Block* taken_block = m_irFactory.defBlock(func);
     Block* done_block = m_irFactory.defBlock(func);
-    IRInstruction* label = m_irFactory.defLabel(1);
+    IRInstruction* label = m_irFactory.defLabel(1, m_curMarker);
     done_block->push_back(label);
     DisableCseGuard guard(*this);
     branch(taken_block);
@@ -232,8 +242,8 @@ struct TraceBuilder {
     Block* done_block = m_irFactory.defBlock(func);
     DisableCseGuard guard(*this);
     branch(taken_block);
-    assert(!m_trace->back()->next());
-    m_trace->back()->setNext(done_block);
+    assert(!m_curTrace->back()->next());
+    m_curTrace->back()->setNext(done_block);
     appendBlock(taken_block);
     taken();
     taken_block->setNext(done_block);
@@ -252,7 +262,7 @@ struct TraceBuilder {
     DisableCseGuard guard(*this);
     branch(done_block);
     next();
-    m_trace->back()->setNext(done_block);
+    m_curTrace->back()->setNext(done_block);
     appendBlock(done_block);
   }
 
@@ -262,8 +272,8 @@ struct TraceBuilder {
    * rejoining the main line.
    */
   IRTrace* makeExitTrace(uint32_t bcOff) {
-    return m_trace->addExitTrace(makeTrace(m_curFunc->getValFunc(),
-                                           bcOff));
+    return m_mainTrace->addExitTrace(makeTrace(m_curFunc->getValFunc(),
+                                               bcOff));
   }
 
 private:
@@ -297,7 +307,7 @@ private:
     std::vector<Type> localTypes;
     SSATmp* refCountedMemValue;
     std::vector<SSATmp*> callerAvailableValues; // unordered list
-    MarkerData lastMarker;
+    BCMarker curMarker;
   };
 
 private:
@@ -358,7 +368,24 @@ private:
   IRFactory& m_irFactory;
   Simplifier m_simplifier;
 
-  boost::scoped_ptr<IRTrace> const m_trace; // generated trace
+  boost::scoped_ptr<IRTrace> const m_mainTrace; // generated trace
+
+  /*
+   * m_savedTraces will be nonempty iff we're emitting code to a trace other
+   * than the main trace. m_curTrace, m_curMarker, m_curBlock, m_curWhere are
+   * all set from the most recent call to pushTrace() or popTrace().
+   */
+  struct TraceState {
+    IRTrace* trace;
+    Block* block;
+    BCMarker marker;
+    boost::optional<Block::iterator> where;
+  };
+  smart::stack<TraceState> m_savedTraces;
+  IRTrace* m_curTrace;
+  Block* m_curBlock;
+  boost::optional<Block::iterator> m_curWhere;
+  BCMarker m_curMarker;
 
   // Flags that enable optimizations
   bool       m_enableCse;
@@ -418,9 +445,6 @@ private:
   // inlined call.
   std::vector<SSATmp*> m_callerAvailableValues;
 
-  // The data for the last Marker instruction we've seen.
-  folly::Optional<MarkerData> m_lastMarker;
-
   // When we're building traces for an inlined callee, the state of
   // the caller needs to be preserved here.
   std::vector<std::unique_ptr<State>> m_inlineSavedStates;
@@ -428,6 +452,29 @@ private:
 
 //////////////////////////////////////////////////////////////////////
 
+/*
+ * RAII helper for emitting code to exit traces. See TraceBuilder::pushTrace
+ * for usage.
+ */
+struct TracePusher {
+  template<typename... Args>
+  TracePusher(TraceBuilder& tb, IRTrace* trace, BCMarker marker,
+              Block* block = nullptr,
+              const boost::optional<Block::iterator>& where =
+                boost::optional<Block::iterator>())
+    : m_tb(tb)
+  {
+    tb.pushTrace(trace, marker, block, where);
+  }
+
+  ~TracePusher() {
+    m_tb.popTrace();
+  }
+
+ private:
+  TraceBuilder& m_tb;
+};
+
 }}
 
 #endif

hphp/runtime/vm/jit/translator.cpp

@@ -3924,12 +3924,7 @@ Translator::translateRegion(const RegionDesc& region,
   } catch (const JIT::FailedCodeGen& exn) {
     FTRACE(1, "code generation failed with {}\n", exn.what());
     SrcKey sk{exn.vmFunc, exn.bcOff};
-
-    // Until we can trust the placement of Marker instructions, we can't assert
-    // that this sk isn't already in the interp set. t2424830
-    if (toInterp.count(sk)) {
-      return Failure;
-    }
+    always_assert(!toInterp.count(sk));
     toInterp.insert(sk);
     return Retry;
   }

hphp/runtime/vm/jit/vectortranslator.cpp

@@ -256,6 +256,7 @@ HhbcTranslator::VectorTranslator::VectorTranslator(
     , m_tb(*m_ht.m_tb)
     , m_irf(m_ht.m_irFactory)
     , m_mii(getMInstrInfo(ni.mInstrOp()))
+    , m_marker(ht.makeMarker(ht.bcOff()))
     , m_needMIS(true)
     , m_misBase(nullptr)
     , m_base(nullptr)
@@ -2412,8 +2413,8 @@ void HhbcTranslator::VectorTranslator::emitMPost() {
       if (input->isA(Type::Gen)) {
         gen(DecRef, input);
         if (m_failedSetTrace) {
-          m_ht.genFor(m_failedSetTrace, DecRefStack,
-                      StackOffset(m_stackInputs[i]), Type::Cell, catchSp);
+          TracePusher tp(m_tb, m_failedSetTrace, m_marker);
+          gen(DecRefStack, StackOffset(m_stackInputs[i]), Type::Cell, catchSp);
         }
       }
       break;
@@ -2451,8 +2452,8 @@ void HhbcTranslator::VectorTranslator::emitMPost() {
   // it will be stored in tvRef.
   static const size_t refOffs[] = { HHIR_MISOFF(tvRef), HHIR_MISOFF(tvRef2) };
   for (unsigned i = 0; i < std::min(nLogicalRatchets(), 2U); ++i) {
-    IRInstruction* inst = m_irf.gen(DecRefMem, Type::Gen, m_misBase,
-                                  cns(refOffs[m_failedSetTrace ? 1 - i : i]));
+    IRInstruction* inst = m_irf.gen(DecRefMem, m_marker, Type::Gen, m_misBase,
+                                    cns(refOffs[m_failedSetTrace ? 1 - i : i]));
     m_tb.add(inst);
     prependToTraces(inst);
   }
@@ -2481,18 +2482,16 @@ void HhbcTranslator::VectorTranslator::emitSideExits(SSATmp* catchSp,
         cns(nStack), // cells popped since the last SpillStack
     };
 
+    TracePusher tp(m_tb, m_failedSetTrace, m_marker);
     if (!isSetWithRef) {
-      m_ht.genFor(m_failedSetTrace, DecRefStack, StackOffset(0),
-                  Type::Cell, catchSp);
-      args.push_back(m_ht.genFor(m_failedSetTrace, LdUnwinderValue,
-                                 Type::Cell));
+      gen(DecRefStack, StackOffset(0), Type::Cell, catchSp);
+      args.push_back(m_ht.gen(LdUnwinderValue, Type::Cell));
     }
 
-    SSATmp* sp = m_ht.genFor(m_failedSetTrace, SpillStack,
-                             std::make_pair(args.size(), &args[0]));
-    m_ht.genFor(m_failedSetTrace, DeleteUnwinderException);
-    m_ht.genFor(m_failedSetTrace, SyncABIRegs, m_tb.fp(), sp);
-    m_ht.genFor(m_failedSetTrace, ReqBindJmp, BCOffset(nextOff));
+    SSATmp* sp = gen(SpillStack, std::make_pair(args.size(), &args[0]));
+    gen(DeleteUnwinderException);
+    gen(SyncABIRegs, m_tb.fp(), sp);
+    gen(ReqBindJmp, BCOffset(nextOff));
   }
 
   if (m_strTestResult) {
@@ -2504,17 +2503,18 @@ void HhbcTranslator::VectorTranslator::emitSideExits(SSATmp* catchSp,
     auto toSpill = m_ht.peekSpillValues();
     assert(toSpill.size());
     assert(toSpill[0] == m_result);
-    SSATmp* str = m_irf.gen(AssertNonNull, m_strTestResult)->dst();
+    SSATmp* str = m_irf.gen(AssertNonNull, m_marker, m_strTestResult)->dst();
     toSpill[0] = str;
-    IRTrace* exit = m_ht.getExitTrace(nextOff, toSpill);
-    exit->front()->prepend(str->inst());
-    exit->front()->prepend(m_irf.gen(DecRef, m_result));
-    exit->front()->prepend(
-      m_irf.gen(IncStat, cns(Stats::TC_SetMStrGuess_Miss),
-                           cns(1), cns(false)));
-    exit->front()->prepend(m_ht.makeMarker(m_ht.bcOff()));
 
-    gen(CheckType, Type::Nullptr, exit, m_strTestResult);
+    auto exitTrace = m_ht.getExitTrace(nextOff, toSpill);
+    {
+      TracePusher tp(m_tb, exitTrace, m_marker, exitTrace->back(),
+                     exitTrace->back()->skipHeader());
+      gen(IncStat, cns(Stats::TC_SetMStrGuess_Miss), cns(1), cns(false));
+      gen(DecRef, m_result);
+      m_tb.add(str->inst());
+    }
+    gen(CheckType, Type::Nullptr, exitTrace, m_strTestResult);
     gen(IncStat, cns(Stats::TC_SetMStrGuess_Hit), cns(1), cns(false));
   }
 }

hphp/runtime/vm/unit.cpp

@@ -1556,10 +1556,12 @@ void Unit::prettyPrint(std::ostream& out, PrintOpts opts) const {
   MetaHandle metaHand;
   while (it < &m_bc[stopOffset]) {
     assert(funcIt == funcMap.end() || funcIt->first >= offsetOf(it));
-    if (funcIt != funcMap.end() && funcIt->first == offsetOf(it)) {
-      out.put('\n');
-      funcIt->second->prettyPrint(out);
-      ++funcIt;
+    if (opts.showFuncs) {
+      if (funcIt != funcMap.end() && funcIt->first == offsetOf(it)) {
+        out.put('\n');
+        funcIt->second->prettyPrint(out);
+        ++funcIt;
+      }
     }
 
     if (opts.showLines) {

hphp/runtime/vm/unit.h

@@ -655,6 +655,7 @@ public:
       : startOffset(kInvalidOffset)
       , stopOffset(kInvalidOffset)
       , showLines(true)
+      , showFuncs(true)
       , indentSize(1)
     {}
 
@@ -669,6 +670,11 @@ public:
       return *this;
     }
 
+    PrintOpts& noFuncs() {
+      showFuncs = false;
+      return *this;
+    }
+
     PrintOpts& indent(int i) {
       indentSize = i;
       return *this;
@@ -677,6 +683,7 @@ public:
     Offset startOffset;
     Offset stopOffset;
     bool showLines;
+    bool showFuncs;
     int indentSize;
   };