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

@@ -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<BlockPtrList> children = findDomChildren(blocks);
 
   // visit dom tree in preorder, checking all tmps

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<Block, bool> 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;
 }
 

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 <boost/noncopyable.hpp>
@@ -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;
       }
     }

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

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<Block*>::const_iterator const_iterator;
+  typedef std::list<Block*>::iterator iterator;
+
   explicit Trace(Block* first, uint32_t bcOff)
     : m_bcOff(bcOff)
     , m_main(nullptr)
@@ -43,11 +45,37 @@ public:
 
   std::list<Block*>& getBlocks() { return m_blocks; }
   const std::list<Block*>& 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);