Add assertions that no SSATmps span calls, except constants and FramePtrs

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

@@ -76,7 +76,7 @@ IdomVector findDominators(const BlockList& blocks);
 typedef std::vector<BlockPtrList> DomChildren;
 
 /*
- * compute the dominator tree, then populate a list of dominator children
+ * Compute the dominator tree, then populate a list of dominator children
  * for each block.  Note that DomChildren is indexed by block->postId(),
  * not block->id(); that's why we don't use StateVector here.
  */
@@ -107,7 +107,7 @@ bool hasInternalFlow(Trace*);
  * as each block is processed.
  */
 template <class State, class Body>
-void forPreorderDoms(Block* block, const std::vector<BlockPtrList>& children,
+void forPreorderDoms(Block* block, const DomChildren& children,
                      State state, Body body) {
   body(block, state);
   for (Block* child : children[block->postId()]) {

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

@@ -22,6 +22,36 @@
 
 namespace HPHP {  namespace JIT {
 
+namespace {
+
+//////////////////////////////////////////////////////////////////////
+
+TRACE_SET_MOD(hhir);
+
+enum Limits : unsigned {
+  kNumRegisters = Transl::kNumRegs,
+  kNumSlots = NumPreAllocatedSpillLocs
+};
+
+struct RegState {
+  RegState() {
+    memset(regs, 0, sizeof(regs));
+    memset(slots, 0, sizeof(slots));
+  }
+  SSATmp* regs[kNumRegisters];  // which tmp is in each register
+  SSATmp* slots[kNumSlots]; // which tmp is in each spill slot
+  SSATmp*& tmp(const RegisterInfo& info, int i) {
+    if (info.spilled()) {
+      auto slot = info.getSpillInfo(i).slot();
+      assert(unsigned(slot) < kNumSlots);
+      return slots[slot];
+    }
+    auto r = info.getReg(i);
+    assert(r != Transl::InvalidReg && unsigned(int(r)) < kNumRegisters);
+    return regs[int(r)];
+  }
+};
+
 /*
  * Check one block for being well formed.  It must:
  * 1. have exactly one DefLabel as the first instruction
@@ -51,6 +81,10 @@ bool checkBlock(Block* b) {
   return true;
 }
 
+}
+
+//////////////////////////////////////////////////////////////////////
+
 /*
  * Build the CFG, then the dominator tree, then use it to validate SSA.
  * 1. Each src must be defined by some other instruction, and each dst must
@@ -93,29 +127,53 @@ bool checkCfg(Trace* trace, const IRFactory& factory) {
   return true;
 }
 
-enum Limits : unsigned {
-  kNumRegisters = Transl::kNumRegs,
-  kNumSlots = NumPreAllocatedSpillLocs
-};
+bool checkTmpsSpanningCalls(Trace* trace, const IRFactory& irFactory) {
+  auto const blocks   = sortCfg(trace, irFactory);
+  auto const children = findDomChildren(blocks);
 
-struct RegState {
-  RegState() {
-    memset(regs, 0, sizeof(regs));
-    memset(slots, 0, sizeof(slots));
-  }
-  SSATmp* regs[kNumRegisters];  // which tmp is in each register
-  SSATmp* slots[kNumSlots]; // which tmp is in each spill slot
-  SSATmp*& tmp(const RegisterInfo& info, int i) {
-    if (info.spilled()) {
-      auto slot = info.getSpillInfo(i).slot();
-      assert(unsigned(slot) < kNumSlots);
-      return slots[slot];
+  // CallBuiltin is ok because it is not a php-level call.  (It will
+  // call a C++ helper and we can push/pop around it normally.)
+  auto isCall = [&] (Opcode op) {
+    return op == Call || op == CallArray;
+  };
+
+  typedef StateVector<SSATmp,bool> State;
+
+  bool isValid = true;
+  forPreorderDoms(
+    blocks.front(), children, State(&irFactory, false),
+    [&] (Block* b, State& state) {
+      for (auto& inst : *b) {
+        for (auto& src : inst.getSrcs()) {
+          if (src->isA(Type::FramePtr)) continue;
+          if (src->isConst()) continue;
+          if (!state[src]) {
+            FTRACE(1, "checkTmpsSpanningCalls failed\n"
+                      "  instruction: {}\n"
+                      "  src:         {}\n",
+                      inst.toString(),
+                      src->toString());
+            isValid = false;
+          }
+        }
+
+        /*
+         * Php calls kill all live temporaries.  We can't keep them
+         * alive across the call because we currently have no
+         * caller-saved registers in our abi, and all translations
+         * share the same spill slots.
+         */
+        if (isCall(inst.op())) state.reset();
+
+        for (auto& d : inst.getDsts()) {
+          state[d] = true;
+        }
+      }
     }
-    auto r = info.getReg(i);
-    assert(r != Transl::InvalidReg && unsigned(int(r)) < kNumRegisters);
-    return regs[int(r)];
-  }
-};
+  );
+
+  return isValid;
+}
 
 bool checkRegisters(Trace* trace, const IRFactory& factory,
                     const RegAllocInfo& regs) {
@@ -146,6 +204,7 @@ bool checkRegisters(Trace* trace, const IRFactory& factory,
       }
     }
   });
+
   return true;
 }
 

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

@@ -30,6 +30,15 @@ struct RegAllocInfo;
  */
 bool checkCfg(Trace*, const IRFactory&);
 
+/*
+ * We can't have SSATmps spanning php-level calls, except for frame
+ * pointers and constant values.
+ *
+ * We have no caller-saved registers in php, and there'd be nowhere to
+ * spill these because all translations share the spill space.
+ */
+bool checkTmpsSpanningCalls(Trace*, const IRFactory&);
+
 /*
  * Check register and spill slot assignments; registers and spill slots must
  * contain the correct SSATmp value at every point of use.

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

@@ -101,7 +101,8 @@ void optimizeTrace(Trace* trace, TraceBuilder* traceBuilder) {
 
   auto finishPass = [&](const char* msg) {
     dumpTrace(6, trace, msg);
-    assert(JIT::checkCfg(trace, *irFactory));
+    assert(checkCfg(trace, *irFactory));
+    assert(checkTmpsSpanningCalls(trace, *irFactory));
     if (debug) forEachTraceInst(trace, assertOperandTypes);
   };