From e3bdb5123faebc3c0eb2717383d3ba4ae92dbf38 Mon Sep 17 00:00:00 2001 From: Jordan DeLong Date: Tue, 21 May 2013 11:27:03 -0700 Subject: [PATCH] Remove edges when dce eliminates a whole block If DCE eliminates a block that ends with a Jmp_, two things can currently go wrong. One is that linearscan will still inspect the incoming edge to try to precolor jump destinations and fail an assert; the other is that the type of the DefLabel destination may be too relaxed. This diff adds a new checkCfg invariant that all incoming edges to a DefLabel are in the block list for the trace, and weakens the assert in linearscan so things still work if DCE is turned off but a block becomes unreachable. Also changes dce's removeUnreachable to reflowTypes if it removes an incoming edge. --- hphp/runtime/vm/translator/hopt/check.cpp | 32 +++++++++++++------ hphp/runtime/vm/translator/hopt/dce.cpp | 24 ++++++++++++-- .../runtime/vm/translator/hopt/linearscan.cpp | 9 ++++-- hphp/runtime/vm/translator/hopt/opt.cpp | 4 +-- hphp/runtime/vm/translator/hopt/trace.h | 32 +++++++++++++++++-- 5 files changed, 82 insertions(+), 19 deletions(-) diff --git a/hphp/runtime/vm/translator/hopt/check.cpp b/hphp/runtime/vm/translator/hopt/check.cpp index c1bb69c82..ca7f03b2b 100644 --- a/hphp/runtime/vm/translator/hopt/check.cpp +++ b/hphp/runtime/vm/translator/hopt/check.cpp @@ -61,6 +61,8 @@ struct RegState { * 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. + * 5. All the incoming edges to the DefLabel must be from blocks listed + * in the block list for this block's Trace. */ bool checkBlock(Block* b) { assert(!b->empty()); @@ -78,13 +80,25 @@ bool checkBlock(Block* b) { // cannot fall-through to join block expecting values assert(b->getNext()->front()->getNumDsts() == 0); } - auto i = b->begin(), e = b->end(); - ++i; - if (b->back()->isBlockEnd()) --e; - for (DEBUG_ONLY IRInstruction& inst : folly::makeRange(i, e)) { - assert(inst.op() != DefLabel); - assert(!inst.isBlockEnd()); + + { + auto i = b->begin(), e = b->end(); + ++i; + if (b->back()->isBlockEnd()) --e; + for (DEBUG_ONLY IRInstruction& inst : folly::makeRange(i, e)) { + assert(inst.op() != DefLabel); + assert(!inst.isBlockEnd()); + } } + + for (int i = 0; i < b->front()->getNumDsts(); ++i) { + auto const traceBlocks = b->getTrace()->getBlocks(); + b->forEachSrc(i, [&](IRInstruction* inst, SSATmp*){ + assert(std::find(traceBlocks.begin(), traceBlocks.end(), + inst->getBlock()) != traceBlocks.end()); + }); + } + return true; } @@ -101,10 +115,8 @@ bool checkBlock(Block* b) { * 4. Treat tmps defined by DefConst as always defined. */ bool checkCfg(Trace* trace, const IRFactory& factory) { - forEachTraceBlock(trace, [&](Block* b) { - checkBlock(b); - }); - BlockList blocks = sortCfg(trace, factory); + auto const blocks = sortCfg(trace, factory); + forEachTraceBlock(trace, checkBlock); std::vector children = findDomChildren(blocks); // visit dom tree in preorder, checking all tmps diff --git a/hphp/runtime/vm/translator/hopt/dce.cpp b/hphp/runtime/vm/translator/hopt/dce.cpp index 445a97172..1c67432c4 100644 --- a/hphp/runtime/vm/translator/hopt/dce.cpp +++ b/hphp/runtime/vm/translator/hopt/dce.cpp @@ -22,6 +22,7 @@ #include "hphp/runtime/vm/translator/hopt/irfactory.h" #include "hphp/runtime/vm/translator/hopt/simplifier.h" #include "hphp/runtime/vm/translator/hopt/state_vector.h" +#include "hphp/runtime/vm/translator/hopt/mutation.h" namespace HPHP { namespace JIT { @@ -143,6 +144,9 @@ bool isUnguardedLoad(IRInstruction* inst) { // removeUnreachable erases unreachable blocks from trace, and returns // a sorted list of the remaining blocks. BlockList removeUnreachable(Trace* trace, IRFactory* factory) { + FTRACE(5, "RemoveUnreachable:vvvvvvvvvvvvvvvvvvvv\n"); + SCOPE_EXIT { FTRACE(5, "RemoveUnreachable:^^^^^^^^^^^^^^^^^^^^\n"); }; + // 1. simplify unguarded loads to remove unnecssary branches, and // perform copy propagation on every instruction. Targets that become // unreachable from this pass will be eliminated in step 2 below. @@ -162,15 +166,29 @@ BlockList removeUnreachable(Trace* trace, IRFactory* factory) { // 2. get a list of reachable blocks by sorting them, and erase any // blocks that are unreachable. + bool needsReflow = false; BlockList blocks = sortCfg(trace, *factory); StateVector reachable(factory, false); for (Block* b : blocks) reachable[b] = true; forEachTrace(trace, [&](Trace* t) { - t->getBlocks().remove_if([&](Block* b) { - return !reachable[b]; - }); + for (auto bit = t->begin(); bit != t->end();) { + if (reachable[*bit]) { + ++bit; + continue; + } + FTRACE(5, "erasing block {}\n", (*bit)->getId()); + if ((*bit)->getTaken() && (*bit)->back()->op() == Jmp_) { + needsReflow = true; + } + bit = t->erase(bit); + } }); + // 3. if we removed any whole blocks that ended in Jmp_ + // instructions, reflow all types in case they change the + // incoming types of DefLabel instructions. + if (needsReflow) reflowTypes(blocks.front(), blocks); + return blocks; } diff --git a/hphp/runtime/vm/translator/hopt/linearscan.cpp b/hphp/runtime/vm/translator/hopt/linearscan.cpp index 9c0f1cc0a..2da6878fe 100644 --- a/hphp/runtime/vm/translator/hopt/linearscan.cpp +++ b/hphp/runtime/vm/translator/hopt/linearscan.cpp @@ -23,6 +23,7 @@ #include "hphp/runtime/vm/translator/hopt/tracebuilder.h" #include "hphp/runtime/vm/translator/hopt/codegen.h" #include "hphp/runtime/vm/translator/hopt/state_vector.h" +#include "hphp/runtime/vm/translator/hopt/check.h" #include "hphp/runtime/vm/translator/physreg.h" #include "hphp/runtime/vm/translator/abi-x64.h" #include @@ -909,8 +910,12 @@ RegNumber LinearScan::getJmpPreColor(SSATmp* tmp, uint32_t regIndex, auto reg = findLabelSrcReg(m_allocInfo, srcInst, i, regIndex); // Until we handle loops, it's a bug to try and allocate a // register to a DefLabel's dest before all of its incoming - // Jmp_s have had their srcs allocated. - always_assert(reg != reg::noreg); + // Jmp_s have had their srcs allocated, unless the incoming + // block is unreachable. + const DEBUG_ONLY bool unreachable = + std::find(m_blocks.begin(), m_blocks.end(), + srcInst->getBlock()) == m_blocks.end(); + always_assert(reg != reg::noreg || unreachable); return reg; } } diff --git a/hphp/runtime/vm/translator/hopt/opt.cpp b/hphp/runtime/vm/translator/hopt/opt.cpp index 6350cc2a7..e4ffb75fc 100644 --- a/hphp/runtime/vm/translator/hopt/opt.cpp +++ b/hphp/runtime/vm/translator/hopt/opt.cpp @@ -120,7 +120,7 @@ void optimizeTrace(Trace* trace, TraceBuilder* traceBuilder) { auto dce = [&](const char* which) { if (!RuntimeOption::EvalHHIRDeadCodeElim) return; eliminateDeadCode(trace, irFactory); - finishPass(folly::format("after {} DCE", which).str().c_str()); + finishPass(folly::format("{} DCE", which).str().c_str()); }; if (false && RuntimeOption::EvalHHIRMemOpt) { @@ -135,7 +135,7 @@ void optimizeTrace(Trace* trace, TraceBuilder* traceBuilder) { && (RuntimeOption::EvalHHIRCse || RuntimeOption::EvalHHIRSimplification)) { traceBuilder->reoptimize(); - finishPass("after reoptimize"); + finishPass("reoptimize"); // Cleanup any dead code left around by CSE/Simplification // Ideally, this would be controlled by a flag returned // by optimzeTrace indicating whether DCE is necessary diff --git a/hphp/runtime/vm/translator/hopt/trace.h b/hphp/runtime/vm/translator/hopt/trace.h index d8b5298cb..9bb5d9d0e 100644 --- a/hphp/runtime/vm/translator/hopt/trace.h +++ b/hphp/runtime/vm/translator/hopt/trace.h @@ -27,8 +27,10 @@ namespace HPHP { namespace JIT { * each block falls through to the next block but this is not guaranteed; * traces may contain internal forward-only control flow. */ -class Trace : boost::noncopyable { -public: +struct Trace : private boost::noncopyable { + typedef std::list::const_iterator const_iterator; + typedef std::list::iterator iterator; + explicit Trace(Block* first, uint32_t bcOff) : m_bcOff(bcOff) , m_main(nullptr) @@ -43,11 +45,37 @@ public: std::list& getBlocks() { return m_blocks; } const std::list& getBlocks() const { return m_blocks; } + Block* front() { return *m_blocks.begin(); } Block* back() { auto it = m_blocks.end(); return *(--it); } const Block* front() const { return *m_blocks.begin(); } const Block* back() const { auto it = m_blocks.end(); return *(--it); } + const_iterator cbegin() const { return getBlocks().cbegin(); } + const_iterator cend() const { return getBlocks().cend(); } + const_iterator begin() const { return getBlocks().begin(); } + const_iterator end() const { return getBlocks().end(); } + iterator begin() { return getBlocks().begin(); } + iterator end() { return getBlocks().end(); } + + /* + * 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; + it = m_blocks.erase(it); + b->setTrace(nullptr); + if (b->back()->op() == Jmp_) { + b->back()->setTaken(nullptr); + } + b->setNext(nullptr); + return it; + } + + /* + * Add a block to the back of this trace's block list. + */ Block* push_back(Block* b) { b->setTrace(this); m_blocks.push_back(b);