New Block invariant

It's a little hard to reason about how lastMarker works in
codegen wrt blocks that don't have Marker.  In a optimization pass for
hoisting CheckType instructions I'm running into cases where I'd have
to track arbitrarily far backward in the main trace to find a Marker
that needs to go into the exit trace; it seemed simpler to just always
have a Marker in the front of each block.

hphp/runtime/vm/translator/hopt/block.h

@@ -94,11 +94,15 @@ struct Block : boost::noncopyable {
   unsigned postId() const { return m_postid; }
   void setPostId(unsigned id) { m_postid = id; }
 
-  // insert inst after this block's label, return an iterator to the
-  // newly inserted instruction.
+  /*
+   * Insert inst after this block's DefLabel/Marker, return an
+   * iterator to the newly inserted instruction.
+   *
+   * Pre: the block contains a Marker after the DefLabel.
+   */
   iterator prepend(IRInstruction* inst) {
     assert(front()->op() == DefLabel);
-    auto it = begin();
+    auto it = skipLabel();
     return insert(++it, inst);
   }
 

hphp/runtime/vm/translator/hopt/check.cpp

@@ -13,8 +13,10 @@
    | license@php.net so we can mail you a copy immediately.               |
    +----------------------------------------------------------------------+
 */
-
 #include "hphp/runtime/vm/translator/hopt/check.h"
+
+#include <boost/next_prior.hpp>
+
 #include "hphp/runtime/vm/translator/hopt/ir.h"
 #include "hphp/runtime/vm/translator/hopt/irfactory.h"
 #include "hphp/runtime/vm/translator/hopt/linearscan.h"
@@ -55,12 +57,17 @@ struct RegState {
 /*
  * Check one block for being well formed.  It must:
  * 1. have exactly one DefLabel as the first instruction
- * 2. if any instruction is isBlockEnd(), it must be last
- * 3. if the last instruction isTerminal(), block->next must be null.
+ * 2. have either no other instructions, or else at least one Marker
+ *    following the DefLabel before any other instructions.
+ * 3. if any instruction is isBlockEnd(), it must be last
+ * 4. if the last instruction isTerminal(), block->next must be null.
  */
 bool checkBlock(Block* b) {
   assert(!b->empty());
   assert(b->front()->op() == DefLabel);
+  if (b->skipLabel() != b->end()) {
+    assert(b->skipLabel()->op() == Marker);
+  }
   if (b->back()->isTerminal()) assert(!b->getNext());
   if (b->getTaken()) {
     // only Jmp_ can branch to a join block expecting values.

hphp/runtime/vm/translator/hopt/hhbctranslator.cpp

@@ -36,6 +36,28 @@ TRACE_SET_MOD(hhir);
 
 using namespace HPHP::Transl;
 
+HhbcTranslator::HhbcTranslator(IRFactory& irFactory,
+                               Offset startOffset,
+                               Offset nextTraceOffset,
+                               uint32_t initialSpOffsetFromFp,
+                               const Func* func)
+  : m_irFactory(irFactory)
+  , m_tb(new TraceBuilder(startOffset,
+                          initialSpOffsetFromFp,
+                          m_irFactory,
+                          func))
+  , m_bcStateStack {BcState(startOffset, func)}
+  , m_startBcOff(startOffset)
+  , m_nextTraceBcOff(nextTraceOffset)
+  , m_lastBcOff(false)
+  , m_hasExit(false)
+  , m_stackDeficit(0)
+{
+  emitMarker();
+  auto const fp = gen(DefFP);
+  gen(DefSP, StackOffset(initialSpOffsetFromFp), fp);
+}
+
 ArrayData* HhbcTranslator::lookupArrayId(int arrId) {
   return getCurUnit()->lookupArrayId(arrId);
 }

hphp/runtime/vm/translator/hopt/hhbctranslator.h

@@ -104,21 +104,7 @@ struct HhbcTranslator {
                  Offset startOffset,
                  Offset nextTraceOffset,
                  uint32_t initialSpOffsetFromFp,
-                 const Func* func)
-    : m_irFactory(irFactory)
-    , m_tb(new TraceBuilder(startOffset,
-                            initialSpOffsetFromFp,
-                            m_irFactory,
-                            func))
-    , m_bcStateStack {BcState(startOffset, func)}
-    , m_startBcOff(startOffset)
-    , m_nextTraceBcOff(nextTraceOffset)
-    , m_lastBcOff(false)
-    , m_hasExit(false)
-    , m_stackDeficit(0)
-  {
-    emitMarker();
-  }
+                 const Func* func);
 
   // Accessors.
   Trace* getTrace() const { return m_tb->getTrace(); }

hphp/runtime/vm/translator/hopt/linearscan.cpp

@@ -1030,6 +1030,10 @@ void LinearScan::allocRegsOneTrace(BlockList::iterator& blockIt,
       block->getNext()->prepend(spill);
     } else {
       auto pos = block->iteratorTo(inst);
+      if (inst->op() == DefLabel) {
+        ++pos;
+        assert(pos != block->end() && pos->op() == Marker);
+      }
       block->insert(++pos, spill);
     }
   }

hphp/runtime/vm/translator/hopt/opt.cpp

@@ -27,7 +27,12 @@ namespace JIT {
 static void insertAfter(IRInstruction* definer, IRInstruction* inst) {
   assert(!definer->isBlockEnd());
   Block* block = definer->getBlock();
-  auto pos = block->iteratorTo(definer); ++pos;
+  auto pos = block->iteratorTo(definer);
+  if (pos->op() == DefLabel) {
+    ++pos;
+    assert(pos != block->end() && pos->op() == Marker);
+  }
+  ++pos;
   block->insert(pos, inst);
 }
 

hphp/runtime/vm/translator/hopt/tracebuilder.cpp

@@ -28,12 +28,11 @@ namespace JIT {
 static const HPHP::Trace::Module TRACEMOD = HPHP::Trace::hhir;
 
 TraceBuilder::TraceBuilder(Offset initialBcOffset,
-                           uint32_t initialSpOffsetFromFp,
+                           Offset initialSpOffsetFromFp,
                            IRFactory& irFactory,
                            const Func* func)
   : m_irFactory(irFactory)
   , m_simplifier(this)
-  , m_initialBcOff(initialBcOffset)
   , m_trace(makeTrace(func, initialBcOffset))
   , m_enableCse(false)
   , m_enableSimplification(false)
@@ -46,19 +45,11 @@ TraceBuilder::TraceBuilder(Offset initialBcOffset,
   , m_localValues(func->numLocals(), nullptr)
   , m_localTypes(func->numLocals(), Type::None)
 {
-  FTRACE(2, "TraceBuilder: initial sp offset {}\n", initialSpOffsetFromFp);
-
-  // put a function marker at the start of trace
   m_curFunc = cns(func);
   if (RuntimeOption::EvalHHIRGenOpts) {
     m_enableCse = RuntimeOption::EvalHHIRCse;
     m_enableSimplification = RuntimeOption::EvalHHIRSimplification;
   }
-
-  gen(DefFP);
-  gen(DefSP, StackOffset(initialSpOffsetFromFp), m_fpValue);
-
-  assert(m_spOffset >= 0);
 }
 
 TraceBuilder::~TraceBuilder() {
@@ -180,6 +171,10 @@ void TraceBuilder::updateTrackedState(IRInstruction* inst) {
   case DefInlineFP:    trackDefInlineFP(inst);  break;
   case InlineReturn:   trackInlineReturn(inst); break;
 
+  case Marker:
+    m_lastMarker = *inst->getExtra<Marker>();
+    break;
+
   case Call:
     m_spValue = inst->getDst();
     // A call pops the ActRec and pushes a return value.
@@ -367,6 +362,7 @@ std::unique_ptr<TraceBuilder::State> TraceBuilder::createState() const {
   state->localTypes = m_localTypes;
   state->callerAvailableValues = m_callerAvailableValues;
   state->refCountedMemValue = m_refCountedMemValue;
+  state->lastMarker = *m_lastMarker;
   return state;
 }
 
@@ -427,6 +423,13 @@ void TraceBuilder::mergeState(State* state) {
 
   // Don't attempt to continue tracking caller's available values.
   state->callerAvailableValues.clear();
+
+  // 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);
 }
 
 void TraceBuilder::useState(std::unique_ptr<State> state) {
@@ -439,6 +442,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;
   // If spValue is null, we merged two different but equivalent values.
   // Define a new sp using the known-good spOffset.
   if (!m_spValue) {
@@ -470,6 +474,7 @@ void TraceBuilder::clearTrackedState() {
     delete *i;
     *i = nullptr;
   }
+  m_lastMarker = folly::none;
 }
 
 void TraceBuilder::appendInstruction(IRInstruction* inst, Block* block) {
@@ -491,6 +496,8 @@ void TraceBuilder::appendInstruction(IRInstruction* inst) {
       block->setNext(next);
     }
     block = next;
+    assert(m_lastMarker.hasValue());
+    gen(Marker, *m_lastMarker);
   }
   appendInstruction(inst, block);
   updateTrackedState(inst);
@@ -503,6 +510,8 @@ void TraceBuilder::appendBlock(Block* block) {
   }
   m_trace->push_back(block);
   useState(block);
+  assert(m_lastMarker.hasValue());
+  gen(Marker, *m_lastMarker);
 }
 
 CSEHash* TraceBuilder::getCSEHashTable(IRInstruction* inst) {

hphp/runtime/vm/translator/hopt/tracebuilder.h

@@ -19,14 +19,15 @@
 
 #include <boost/scoped_ptr.hpp>
 
+#include "folly/ScopeGuard.h"
+#include "folly/Optional.h"
+
 #include "hphp/runtime/vm/translator/hopt/ir.h"
 #include "hphp/runtime/vm/translator/hopt/irfactory.h"
 #include "hphp/runtime/vm/translator/hopt/cse.h"
 #include "hphp/runtime/vm/translator/hopt/simplifier.h"
 #include "hphp/runtime/vm/translator/hopt/state_vector.h"
 
-#include "folly/ScopeGuard.h"
-
 namespace HPHP {  namespace JIT {
 
 //////////////////////////////////////////////////////////////////////
@@ -89,7 +90,7 @@ namespace HPHP {  namespace JIT {
  */
 struct TraceBuilder {
   TraceBuilder(Offset initialBcOffset,
-               uint32_t initialSpOffsetFromFp,
+               Offset initialSpOffsetFromFp,
                IRFactory&,
                const Func* func);
   ~TraceBuilder();
@@ -280,6 +281,7 @@ private:
     std::vector<Type> localTypes;
     SSATmp* refCountedMemValue;
     std::vector<SSATmp*> callerAvailableValues; // unordered list
+    MarkerData lastMarker;
   };
 
 private:
@@ -339,7 +341,6 @@ private:
   IRFactory& m_irFactory;
   Simplifier m_simplifier;
 
-  Offset const m_initialBcOff; // offset of initial bytecode in trace
   boost::scoped_ptr<Trace> const m_trace; // generated trace
 
   // Flags that enable optimizations
@@ -400,6 +401,9 @@ 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;