From 362f69b6fe99021588ddbf25ec5bfe7c3cb4586c Mon Sep 17 00:00:00 2001
From: Jordan DeLong <jdelong@fb.com>
Date: Mon, 20 May 2013 06:48:22 -0700
Subject: [PATCH] Fix an issue in the register allocator

An optimization I'm working on sent the register allocator a
CFG that looked like this:

  M0
  | \
  |  M1
  M3 | \
  |  |  E4
  |  M8
  | /
  M2
  |
  M5

The RPO was 0 1 8 4 3 2 5.

It assert-fails here because it visited E4 before M3, and it appears
this would incorrectly mark everything as free.

This diff changes the register allocator to skip all non-main-trace
blocks during the first pass over the CFG.  Then each exit block is
visited.
---
 hphp/runtime/vm/translator/hopt/cfg.h         |  2 +-
 hphp/runtime/vm/translator/hopt/codegen.cpp   |  4 +-
 .../runtime/vm/translator/hopt/linearscan.cpp | 42 ++++++++++++++++---
 3 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/hphp/runtime/vm/translator/hopt/cfg.h b/hphp/runtime/vm/translator/hopt/cfg.h
index 07328afca..c04ef5449 100644
--- a/hphp/runtime/vm/translator/hopt/cfg.h
+++ b/hphp/runtime/vm/translator/hopt/cfg.h
@@ -32,7 +32,7 @@ namespace HPHP { namespace JIT {
  */
 template <class Visitor>
 struct PostorderSort {
-  PostorderSort(Visitor &visitor, unsigned num_blocks)
+  PostorderSort(Visitor& visitor, unsigned num_blocks)
     : m_visited(num_blocks), m_visitor(visitor)
   {}
 
diff --git a/hphp/runtime/vm/translator/hopt/codegen.cpp b/hphp/runtime/vm/translator/hopt/codegen.cpp
index 15d89b606..00827e84f 100644
--- a/hphp/runtime/vm/translator/hopt/codegen.cpp
+++ b/hphp/runtime/vm/translator/hopt/codegen.cpp
@@ -3563,7 +3563,7 @@ void CodeGenerator::cgSpillStack(IRInstruction* inst) {
     // we don't need to spill it.
     if (inst->op() == LdStack && inst->getSrc(0) == sp &&
         inst->getExtra<LdStack>()->offset * sizeof(Cell) == offset) {
-      FTRACE(1, "{}: Not spilling spill value {} from {}\n",
+      FTRACE(6, "{}: Not spilling spill value {} from {}\n",
              __func__, i, inst->toString());
     } else {
       cgStore(spReg, offset, val);
@@ -3942,7 +3942,7 @@ void CodeGenerator::recordSyncPoint(Asm& as,
 
   Offset pcOff = m_state.lastMarker->bcOff - m_state.lastMarker->func->base();
 
-  FTRACE(3, "IR recordSyncPoint: {} {} {}\n", as.code.frontier, pcOff,
+  FTRACE(5, "IR recordSyncPoint: {} {} {}\n", as.code.frontier, pcOff,
          stackOff);
   m_tx64->recordSyncPoint(as, pcOff, stackOff);
 }
diff --git a/hphp/runtime/vm/translator/hopt/linearscan.cpp b/hphp/runtime/vm/translator/hopt/linearscan.cpp
index 2da6878fe..56f06ba7a 100644
--- a/hphp/runtime/vm/translator/hopt/linearscan.cpp
+++ b/hphp/runtime/vm/translator/hopt/linearscan.cpp
@@ -33,7 +33,7 @@ namespace JIT{
 
 using namespace Transl::reg;
 
-static const HPHP::Trace::Module TRACEMOD = HPHP::Trace::hhir;
+TRACE_SET_MOD(hhir);
 
 int RegisterInfo::numAllocatedRegs() const {
   // Return the number of register slots that actually have an allocated
@@ -142,7 +142,8 @@ private:
   void initFreeList();
   void coalesce(Trace* trace);
   void genSpillStats(Trace* trace, int numSpillLocs);
-  void allocRegsOneTrace(BlockList::iterator& blockIt, ExitTraceMap& etm);
+  void allocRegsOneTrace(BlockList::iterator& blockIt,
+                         ExitTraceMap& etm);
   void allocRegsToTrace();
   uint32_t createSpillSlot(SSATmp* tmp);
   static SSATmp* getSpilledTmp(SSATmp* tmp);
@@ -168,7 +169,7 @@ private:
 
   template<typename Inner, int DumpVal=4>
   void dumpIR(const Inner* in, const char* msg) {
-    if (dumpIREnabled(DumpVal)) {
+    if (HPHP::Trace::moduleEnabled(HPHP::Trace::hhir, DumpVal)) {
       std::ostringstream str;
       print(str, in, &m_allocInfo, &m_lifetime);
       HPHP::Trace::traceRelease("--- %s: %s\n", msg, str.str().c_str());
@@ -1125,7 +1126,8 @@ RegAllocInfo LinearScan::allocRegs(Trace* trace, LifetimeInfo* lifetime) {
 
 void LinearScan::allocRegsOneTrace(BlockList::iterator& blockIt,
                                    ExitTraceMap& etm) {
-  Trace* trace = (*blockIt)->getTrace();
+  auto const trace = (*blockIt)->getTrace();
+
   collectInfo(blockIt, trace);
   computePreColoringHint();
 
@@ -1145,8 +1147,16 @@ void LinearScan::allocRegsOneTrace(BlockList::iterator& blockIt,
   while (blockIt != m_blocks.end()) {
     Block* block = *blockIt;
     if (block->getTrace() != trace) {
-      break;
+      if (!isMain) {
+        break;
+      } else {
+        ++blockIt;
+        continue;
+      }
     }
+    FTRACE(5, "Block{}: {} ({})\n",
+           trace->isMain() ? "" : " (exit trace)",
+           (*blockIt)->getId(), (*blockIt)->postId());
 
     // clear remembered reloads that don't dominate this block
     for (SlotInfo& slot : m_slots) {
@@ -1158,7 +1168,7 @@ void LinearScan::allocRegsOneTrace(BlockList::iterator& blockIt,
     }
     for (auto it = block->begin(), end = block->end(); it != end; ++it) {
       allocRegToInstruction(it);
-      dumpIR<IRInstruction, kExtraLevel>(&*it, "allocated to instruction");
+      dumpIR<IRInstruction, kExtraLevel>(&*it, "allocated to instruction ");
     }
     if (isMain) {
       assert(block->getTrace()->isMain());
@@ -1213,10 +1223,30 @@ void LinearScan::allocRegsToTrace() {
 
   numberInstructions(m_blocks);
 
+  if (HPHP::Trace::moduleEnabled(HPHP::Trace::hhir, 5)) {
+    std::stringstream s;
+    s << "RPO: ";
+    for (auto& b : m_blocks) {
+      s << folly::format("{}{} ",
+                         b->isMain() ? "M" : "E",
+                         b->getId());
+    }
+    s << "\n";
+    HPHP::Trace::traceRelease("%s\n", s.str().c_str());
+  }
+
   BlockList::iterator it = m_blocks.begin();
   while (it != m_blocks.end()) {
     allocRegsOneTrace(it, etm);
   }
+
+  for (it = m_blocks.begin(); it != m_blocks.end();) {
+    if ((*it)->isMain()) {
+      ++it;
+      continue;
+    }
+    allocRegsOneTrace(it, etm);
+  }
 }
 
 void LinearScan::rematerialize() {