From 0b763991b66f663b2aa290742f9eda219dd5f8a5 Mon Sep 17 00:00:00 2001 From: Jordan DeLong Date: Sun, 19 May 2013 12:42:02 -0700 Subject: [PATCH] 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 | 10 +++++-- hphp/runtime/vm/translator/hopt/check.cpp | 13 +++++++-- .../vm/translator/hopt/hhbctranslator.cpp | 22 ++++++++++++++ .../vm/translator/hopt/hhbctranslator.h | 16 +--------- .../runtime/vm/translator/hopt/linearscan.cpp | 4 +++ hphp/runtime/vm/translator/hopt/opt.cpp | 7 ++++- .../vm/translator/hopt/tracebuilder.cpp | 29 ++++++++++++------- .../runtime/vm/translator/hopt/tracebuilder.h | 12 +++++--- 8 files changed, 77 insertions(+), 36 deletions(-) diff --git a/hphp/runtime/vm/translator/hopt/block.h b/hphp/runtime/vm/translator/hopt/block.h index 1d6e56475..b7f6ac438 100644 --- a/hphp/runtime/vm/translator/hopt/block.h +++ b/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); } diff --git a/hphp/runtime/vm/translator/hopt/check.cpp b/hphp/runtime/vm/translator/hopt/check.cpp index ea2a034f1..48c482ba8 100644 --- a/hphp/runtime/vm/translator/hopt/check.cpp +++ b/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 + #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. diff --git a/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp b/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp index 867dc3470..c16d88bc7 100644 --- a/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp +++ b/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); } diff --git a/hphp/runtime/vm/translator/hopt/hhbctranslator.h b/hphp/runtime/vm/translator/hopt/hhbctranslator.h index 9016622c9..4bc5c6bc7 100644 --- a/hphp/runtime/vm/translator/hopt/hhbctranslator.h +++ b/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(); } diff --git a/hphp/runtime/vm/translator/hopt/linearscan.cpp b/hphp/runtime/vm/translator/hopt/linearscan.cpp index dfbaa0f7b..4b2110c69 100644 --- a/hphp/runtime/vm/translator/hopt/linearscan.cpp +++ b/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); } } diff --git a/hphp/runtime/vm/translator/hopt/opt.cpp b/hphp/runtime/vm/translator/hopt/opt.cpp index b61fe3c37..922422e5d 100644 --- a/hphp/runtime/vm/translator/hopt/opt.cpp +++ b/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); } diff --git a/hphp/runtime/vm/translator/hopt/tracebuilder.cpp b/hphp/runtime/vm/translator/hopt/tracebuilder.cpp index 9f29be0fb..dcbfa29e7 100644 --- a/hphp/runtime/vm/translator/hopt/tracebuilder.cpp +++ b/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(); + 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::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) { @@ -439,6 +442,7 @@ void TraceBuilder::useState(std::unique_ptr 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) { diff --git a/hphp/runtime/vm/translator/hopt/tracebuilder.h b/hphp/runtime/vm/translator/hopt/tracebuilder.h index cba93d388..9b2681174 100644 --- a/hphp/runtime/vm/translator/hopt/tracebuilder.h +++ b/hphp/runtime/vm/translator/hopt/tracebuilder.h @@ -19,14 +19,15 @@ #include +#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 localTypes; SSATmp* refCountedMemValue; std::vector 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 const m_trace; // generated trace // Flags that enable optimizations @@ -400,6 +401,9 @@ private: // inlined call. std::vector m_callerAvailableValues; + // The data for the last Marker instruction we've seen. + folly::Optional m_lastMarker; + // When we're building traces for an inlined callee, the state of // the caller needs to be preserved here. std::vector> m_inlineSavedStates;