Support parameter reffiness in the tracelet region selector

This brings the tracelet region selector to 100% coverage,
though perf is still not at parity. Guard relaxation/specialization
and inlining are the big remaining features that will help perf.

hphp/runtime/vm/hhbc.cpp

@@ -1023,8 +1023,7 @@ int instrSpToArDelta(const Op* opcode) {
   // stack are already accounted for by numPops.
   int numPops = instrNumPops(opcode);
   int numExtra = isFCallStar(*opcode) ? 0 : getImm(opcode, 0).u_IVA;
-  int delta = numPops + numExtra;
-  return delta;
+  return numPops + numExtra;
 }
 
 const MInstrInfo& getMInstrInfo(Op op) {

hphp/runtime/vm/jit/annotation.cpp

@@ -144,12 +144,12 @@ void annotate(NormalizedInstruction* i) {
       const StringData* className = nullptr;
       const StringData* funcName = nullptr;
       if (i->op() == OpFPushFuncD) {
-        funcName = curUnit()->lookupLitstrId(i->imm[1].u_SA);
+        funcName = i->m_unit->lookupLitstrId(i->imm[1].u_SA);
       } else if (i->op() == OpFPushObjMethodD) {
         if (i->inputs[0]->valueType() != KindOfObject) break;
         const Class* cls = i->inputs[0]->rtt.valueClass();
         if (!cls) break;
-        funcName = curUnit()->lookupLitstrId(i->imm[1].u_SA);
+        funcName = i->m_unit->lookupLitstrId(i->imm[1].u_SA);
         className = cls->name();
       } else if (i->op() == OpFPushClsMethodF) {
         if (!i->inputs[1]->isString() ||
@@ -162,10 +162,10 @@ void annotate(NormalizedInstruction* i) {
         funcName = i->inputs[1]->rtt.valueString();
         className = cls->name();
       } else if (i->op() == OpFPushClsMethodD) {
-        funcName = curUnit()->lookupLitstrId(i->imm[1].u_SA);
-        className = curUnit()->lookupLitstrId(i->imm[2].u_SA);
+        funcName = i->m_unit->lookupLitstrId(i->imm[1].u_SA);
+        className = i->m_unit->lookupLitstrId(i->imm[2].u_SA);
       } else if (i->op() == OpFPushCtorD) {
-        className = curUnit()->lookupLitstrId(i->imm[1].u_SA);
+        className = i->m_unit->lookupLitstrId(i->imm[1].u_SA);
         const Class* cls = Unit::lookupUniqueClass(className);
         if (!cls) break;
         funcName = cls->getCtor()->name();
@@ -176,7 +176,7 @@ void annotate(NormalizedInstruction* i) {
         funcName = cls->getCtor()->name();
       }
       assert(funcName->isStatic());
-      recordActRecPush(*i, curUnit(), funcName, className,
+      recordActRecPush(*i, i->m_unit, funcName, className,
                        i->op() == OpFPushClsMethodD ||
                        i->op() == OpFPushClsMethodF);
     } break;

hphp/runtime/vm/jit/hhbc-translator.cpp

@@ -233,6 +233,10 @@ Type HhbcTranslator::topType(uint32_t idx) const {
   }
 }
 
+size_t HhbcTranslator::spOffset() const {
+  return m_tb->spOffset() + m_evalStack.size() - m_stackDeficit;
+}
+
 /*
  * When doing gen-time inlining, we set up a series of IR instructions
  * that looks like this:
@@ -2658,7 +2662,7 @@ void HhbcTranslator::checkTypeStack(uint32_t idx, Type type, Offset dest) {
   } else {
     FTRACE(1, "checkTypeStack({}): no tmp: {}\n", idx, type.toString());
     gen(CheckStk, type, exitTrace,
-        StackOffset(idx - m_evalStack.size () + m_stackDeficit), m_tb->sp());
+        StackOffset(idx - m_evalStack.size() + m_stackDeficit), m_tb->sp());
   }
 }
 

hphp/runtime/vm/jit/hhbc-translator.h

@@ -764,7 +764,7 @@ private: // Exit trace creation routines.
                             std::vector<SSATmp*>& spillValues,
                             const CustomExit&);
 
-private:
+public:
   /*
    * Accessors for the current function being compiled and its
    * class and unit.
@@ -774,7 +774,9 @@ private:
   Unit*       curUnit()   const { return curFunc()->unit(); }
   Offset      bcOff()     const { return m_bcStateStack.back().bcOff; }
   SrcKey      curSrcKey() const { return SrcKey(curFunc(), bcOff()); }
+  size_t      spOffset()  const;
 
+private:
   /*
    * Predictates for testing information about the relationship of a
    * class to the current context class.

hphp/runtime/vm/jit/ir-translator.cpp

@@ -1705,7 +1705,7 @@ void IRTranslator::translateInstr(const NormalizedInstruction& i) {
     // If the instruction takes a slow exit, the exit trace will
     // decrement the post counter for that opcode.
     m_hhbcTrans.emitIncStat(Stats::opcodeToIRPostStatCounter(i.op()),
-                             1, true);
+                            1, true);
   }
 
   if (instrMustInterp(i) || i.interp) {
@@ -1714,6 +1714,10 @@ void IRTranslator::translateInstr(const NormalizedInstruction& i) {
     translateInstrWork(i);
   }
 
+  if (Transl::callDestroysLocals(i, m_hhbcTrans.curFunc())) {
+    m_hhbcTrans.emitSmashLocals();
+  }
+
   passPredictedAndInferredTypes(i);
 }
 

hphp/runtime/vm/jit/region-selection.cpp

@@ -91,7 +91,7 @@ void RegionDesc::Block::addPredicted(SrcKey sk, TypePred pred) {
   checkInvariants();
 }
 
-void RegionDesc::Block::setParamByRef(SrcKey sk, ParamByRef byRef) {
+void RegionDesc::Block::setParamByRef(SrcKey sk, bool byRef) {
   assert(m_byRefs.find(sk) == m_byRefs.end());
   m_byRefs.insert(std::make_pair(sk, byRef));
   checkInvariants();
@@ -213,9 +213,7 @@ RegionDescPtr selectTraceletLegacy(const Transl::Tracelet& tlet) {
       curBlock->setKnownFunc(sk, topFunc);
     }
     if (!ni->noOp && isFPassStar(ni->op())) {
-      curBlock->setParamByRef(
-        sk, ni->preppedByRef ? RegionDesc::ParamByRef::Yes
-                             : RegionDesc::ParamByRef::No);
+      curBlock->setParamByRef(sk, ni->preppedByRef);
     }
     if (ni->op() == OpJmp && ni->next) {
       // A Jmp that isn't the final instruction in a Tracelet means we traced
@@ -408,14 +406,6 @@ std::string show(const RegionDesc::ReffinessPred& pred) {
   return out.str();
 }
 
-std::string show(RegionDesc::ParamByRef byRef) {
-  switch (byRef) {
-    case RegionDesc::ParamByRef::Yes: return "by value";
-    case RegionDesc::ParamByRef::No:  return "by reference";
-  }
-  not_reached();
-}
-
 std::string show(RegionContext::LiveType ta) {
   return folly::format(
     "{} :: {}",
@@ -445,7 +435,7 @@ std::string show(const RegionDesc::Block& b) {
   auto byRefs    = makeMapWalker(b.paramByRefs());
   auto refPreds  = makeMapWalker(b.reffinessPreds());
   auto knownFuncs= makeMapWalker(b.knownFuncs());
-  auto skIter = b.start();
+  auto skIter    = b.start();
 
   const Func* currentFunc = nullptr;
 
@@ -469,7 +459,8 @@ std::string show(const RegionDesc::Block& b) {
 
     std::string byRef;
     if (byRefs.hasNext(skIter)) {
-      byRef = folly::format(" (passed {})", show(byRefs.next())).str();
+      byRef = folly::format(" (passed by {})", byRefs.next() ? "reference"
+                                                             : "value").str();
     }
 
     folly::toAppend(

hphp/runtime/vm/jit/region-selection.h

@@ -57,10 +57,6 @@ struct RegionDesc {
   struct TypePred;
   struct ReffinessPred;
   typedef std::shared_ptr<Block> BlockPtr;
-  enum class ParamByRef : uint8_t {
-    Yes,
-    No,
-  };
 
   template<typename... Args>
   Block* addBlock(Args&&... args) {
@@ -149,7 +145,7 @@ struct RegionDesc::ReffinessPred {
  */
 class RegionDesc::Block {
   typedef flat_multimap<SrcKey, TypePred> TypePredMap;
-  typedef flat_map<SrcKey, ParamByRef> ParamByRefMap;
+  typedef flat_map<SrcKey, bool> ParamByRefMap;
   typedef flat_multimap<SrcKey, ReffinessPred> RefPredMap;
   typedef flat_map<SrcKey, const Func*> KnownFuncMap;
 
@@ -191,7 +187,7 @@ public:
   /*
    * Add information about parameter reffiness to this block.
    */
-  void setParamByRef(SrcKey sk, ParamByRef);
+  void setParamByRef(SrcKey sk, bool);
 
   /*
    * Add a reffiness prediction about a pre-live ActRec.
@@ -308,7 +304,6 @@ RegionDesc::BlockPtr createBlock(const Transl::Tracelet& tlet);
 std::string show(RegionDesc::Location);
 std::string show(RegionDesc::TypePred);
 std::string show(const RegionDesc::ReffinessPred&);
-std::string show(RegionDesc::ParamByRef);
 std::string show(RegionContext::LiveType);
 std::string show(RegionContext::PreLiveAR);
 std::string show(const RegionDesc::Block&);

hphp/runtime/vm/jit/region-tracelet.cpp

@@ -15,6 +15,7 @@
 */
 
 #include "hphp/util/trace.h"
+#include "hphp/runtime/vm/jit/annotation.h"
 #include "hphp/runtime/vm/jit/hhbc-translator.h"
 #include "hphp/runtime/vm/jit/ir-translator.h"
 #include "hphp/runtime/vm/jit/region-selection.h"
@@ -23,55 +24,171 @@
 namespace HPHP { namespace JIT {
 
 using Transl::DynLocation;
+using Transl::ActRecState;
+using Transl::RefDeps;
 
 TRACE_SET_MOD(region);
 
 typedef hphp_hash_set<SrcKey, SrcKey::Hasher> InterpSet;
 
 namespace {
-struct State {
-  const Unit* curUnit;
-  const SrcKey startSk;
-  HhbcTranslator& ht;
-  Unit::MetaHandle metaHand;
+struct RegionFormer {
+  RegionFormer(const RegionContext& ctx, InterpSet& interp);
+
+  RegionDescPtr go();
+
+private:
+  const RegionContext& m_ctx;
+  InterpSet& m_interp;
+  const Func* m_curFunc;
+  const Unit* m_curUnit;
+  SrcKey m_sk;
+  const SrcKey m_startSk;
+  NormalizedInstruction m_inst;
+  RegionDescPtr m_region;
+  RegionDesc::Block* m_curBlock;
+  bool m_blockFinished;
+  int m_pendingLiterals;
+  IRTranslator m_irTrans;
+  HhbcTranslator& m_ht;
+  Unit::MetaHandle m_metaHand;
+  ActRecState m_arState;
+  RefDeps m_refDeps;
+
+  bool prepareInstruction();
+  void addInstruction();
 };
 
+RegionFormer::RegionFormer(const RegionContext& ctx, InterpSet& interp)
+  : m_ctx(ctx)
+  , m_interp(interp)
+  , m_curFunc(ctx.func)
+  , m_curUnit(m_curFunc->unit())
+  , m_sk(m_curFunc, ctx.bcOffset)
+  , m_startSk(m_sk)
+  , m_region(smart::make_unique<RegionDesc>())
+  , m_curBlock(m_region->addBlock(m_curFunc, m_sk.offset(), 0))
+  , m_blockFinished(false)
+  , m_pendingLiterals(0)
+  , m_irTrans(ctx.bcOffset, ctx.spOffset, ctx.func)
+  , m_ht(m_irTrans.hhbcTrans())
+{
+}
+
+RegionDescPtr RegionFormer::go() {
+  uint32_t numJmps = 0;
+  for (auto const& lt : m_ctx.liveTypes) {
+    auto t = lt.type;
+    if (t.strictSubtypeOf(Type::Obj)) t = t.unspecialize();
+
+    if (t.subtypeOf(Type::Cls)) {
+      m_ht.assertTypeStack(lt.location.stackOffset(), t);
+    } else {
+      m_ht.guardTypeLocation(lt.location, t);
+    }
+    m_curBlock->addPredicted(m_sk, RegionDesc::TypePred{lt.location, t});
+  }
+
+  while (true) {
+    if (!prepareInstruction()) break;
+    Transl::annotate(&m_inst);
+
+    // Before doing the translation, check for tracelet-ending control flow.
+    if (m_inst.op() == OpJmp && m_inst.imm[0].u_BA > 0 &&
+        numJmps < Transl::Translator::MaxJmpsTracedThrough) {
+      // Include the Jmp in the region and continue to its destination.
+      ++numJmps;
+      m_sk.setOffset(m_sk.offset() + m_inst.imm[0].u_BA);
+      m_blockFinished = true;
+
+      m_ht.setBcOff(m_sk.offset(), false);
+      continue;
+    } else if (Transl::opcodeBreaksBB(m_inst.op()) ||
+               (Transl::dontGuardAnyInputs(m_inst.op()) &&
+                Transl::opcodeChangesPC(m_inst.op()))) {
+      // This instruction ends the tracelet.
+      break;
+    }
+
+    m_inst.interp = m_interp.count(m_sk);
+    auto const doPrediction = Transl::outputIsPredicted(m_startSk, m_inst);
+
+    try {
+      m_irTrans.translateInstr(m_inst);
+    } catch (const FailedIRGen& exn) {
+      FTRACE(1, "ir generation for {} failed with {}\n",
+             m_inst.toString(), exn.what());
+      always_assert(!m_interp.count(m_sk));
+      m_interp.insert(m_sk);
+      m_region.reset();
+      break;
+    }
+
+    if (isFCallStar(m_inst.op())) m_arState.pop();
+
+    // Advance sk and check the prediction, if any.
+    m_sk.advance(m_curBlock->unit());
+    if (doPrediction) m_ht.checkTypeStack(0, m_inst.outPred, m_sk.offset());
+  }
+
+  if (m_region) {
+    // Record the incrementally constructed reffiness predictions.
+    assert(!m_region->blocks.empty());
+    auto& frontBlock = *m_region->blocks.front();
+    for (auto const& dep : m_refDeps.m_arMap) {
+      frontBlock.addReffinessPred(m_startSk, {dep.second.m_mask,
+                                              dep.second.m_vals,
+                                              dep.first});
+    }
+  }
+
+  return std::move(m_region);
+}
+
 /*
- * Populate most fields of the NormalizedInstruciton, assuming its sk
+ * Populate most fields of the NormalizedInstruction, assuming its sk
  * has already been set. Returns false iff the region should be
  * truncated before inst's SrcKey.
  */
-bool prepareInstruction(NormalizedInstruction& inst, State& state) {
-  inst.m_unit = state.curUnit;
-  inst.breaksTracelet = false;
-  inst.changesPC = Transl::opcodeChangesPC(inst.op());
-  inst.funcd = nullptr;
-  Transl::populateImmediates(inst);
-  Transl::preInputApplyMetaData(state.metaHand, &inst);
+bool RegionFormer::prepareInstruction() {
+  m_inst.~NormalizedInstruction();
+  new (&m_inst) NormalizedInstruction();
+  m_inst.source = m_sk;
+  m_inst.m_unit = m_curUnit;
+  m_inst.breaksTracelet = false;
+  m_inst.changesPC = Transl::opcodeChangesPC(m_inst.op());
+  m_inst.funcd = m_arState.knownFunc();
+  Transl::populateImmediates(m_inst);
+  Transl::preInputApplyMetaData(m_metaHand, &m_inst);
 
   Transl::InputInfos inputInfos;
-  getInputs(state.startSk, inst, inputInfos, [&](int i) {
-    return state.ht.traceBuilder()->getLocalType(i);
+  getInputs(m_startSk, m_inst, inputInfos, [&](int i) {
+    return m_ht.traceBuilder()->getLocalType(i);
   });
 
+  // Read types for all the inputs and apply MetaData.
   auto newDynLoc = [&](const Transl::InputInfo& ii) {
-    auto dl = inst.newDynLoc(ii.loc, state.ht.rttFromLocation(ii.loc));
+    auto dl = m_inst.newDynLoc(ii.loc, m_ht.rttFromLocation(ii.loc));
     FTRACE(2, "rttFromLocation: {} -> {}\n",
            ii.loc.pretty(), dl->rtt.pretty());
     return dl;
   };
 
-  for (auto const& ii : inputInfos) {
-    auto* dl = newDynLoc(ii);
-    auto const& rtt = dl->rtt;
-    inst.inputs.push_back(dl);
+  for (auto const& ii : inputInfos) m_inst.inputs.push_back(newDynLoc(ii));
+  readMetaData(m_metaHand, m_inst, m_ht);
+
+  // Check all the inputs for unknown values.
+  assert(inputInfos.size() == m_inst.inputs.size());
+  for (unsigned i = 0; i < inputInfos.size(); ++i) {
+    auto const& ii = inputInfos[i];
+    auto const& rtt = m_inst.inputs[i]->rtt;
 
     if (ii.dontBreak || ii.dontGuard) continue;
     if (rtt.isVagueValue()) {
       // Trying to consume a value without a precise enough type.
       return false;
     }
-    if (inst.ignoreInnerType || ii.dontGuardInner) continue;
+    if (m_inst.ignoreInnerType || ii.dontGuardInner) continue;
     if (rtt.isValue() && rtt.isRef() &&
         (rtt.innerType() == KindOfInvalid || rtt.innerType() == KindOfAny)) {
       // Trying to consume a boxed value without a guess for the inner type.
@@ -79,112 +196,53 @@ bool prepareInstruction(NormalizedInstruction& inst, State& state) {
     }
   }
 
-  readMetaData(state.metaHand, inst, state.ht);
-  if (!inst.noOp && inputInfos.needsRefCheck) {
-    // not supported yet
-    return false;
+  if (!m_inst.noOp && inputInfos.needsRefCheck) {
+    // Reffiness guards are always at the beginning of the trace for now, so
+    // calculate the delta from the original sp to the ar.
+    auto argNum = m_inst.imm[0].u_IVA;
+    size_t entryArDelta = instrSpToArDelta((Op*)m_inst.pc()) -
+      (m_ht.spOffset() - m_ctx.spOffset);
+    try {
+      m_inst.preppedByRef = m_arState.checkByRef(argNum, entryArDelta,
+                                               &m_refDeps);
+    } catch (const Transl::UnknownInputExc& exn) {
+      // We don't have a guess for the current ActRec.
+      return false;
+    }
+    addInstruction();
+    m_curBlock->setParamByRef(m_inst.source, m_inst.preppedByRef);
+  } else {
+    addInstruction();
   }
 
+  if (isFPush(m_inst.op())) m_arState.pushFunc(m_inst);
+
   return true;
 }
 
-RegionDescPtr regionTraceletImpl(const RegionContext& ctx,
-                                 InterpSet& toInterp) {
-  IRTranslator irTrans(ctx.bcOffset, ctx.spOffset, ctx.func);
-  auto* curFunc = ctx.func;
-  State state{
-    curFunc->unit(),
-    {ctx.func, ctx.bcOffset},
-    irTrans.hhbcTrans(),
-  };
-  auto& ht = state.ht;
-  uint32_t numJmps = 0;
+/*
+ * Add the current instruction to the region. Instructions that push constant
+ * values aren't pushed unless more instructions come after them.
+ */
+void RegionFormer::addInstruction() {
+  if (m_blockFinished) {
+    m_curBlock = m_region->addBlock(m_curFunc, m_inst.source.offset(), 0);
+    m_blockFinished = false;
+  }
 
-  SrcKey sk(state.startSk);
-  auto region = smart::make_unique<RegionDesc>();
-  auto* curBlock = region->addBlock(curFunc, sk.offset(), 0);
-  bool blockFinished = false;
-  uint32_t pendingLiterals = 0;
-
-  auto addInstruction = [&] {
-    if (blockFinished) {
-      curBlock = region->addBlock(curFunc, sk.offset(), 0);
-      blockFinished = false;
-    }
-
-    auto op = toOp(*state.curUnit->at(sk.offset()));
+  auto op = toOp(*m_curUnit->at(m_inst.source.offset()));
+  if (isLiteral(op) || isThisSelfOrParent(op)) {
     // Don't finish a region with literal values or values that have a class
     // related to the current context class. They produce valuable information
     // for optimizations that's lost across region boundaries.
-    if (isLiteral(op) || isThisSelfOrParent(op)) {
-      ++pendingLiterals;
-      return;
-    }
-
+    ++m_pendingLiterals;
+  } else {
     // This op isn't a literal so add any that are pending before the current
     // instruction.
-    for (; pendingLiterals; --pendingLiterals) curBlock->addInstruction();
-    curBlock->addInstruction();
-  };
-
-  for (auto const& lt : ctx.liveTypes) {
-    auto t = lt.type;
-    if (t.strictSubtypeOf(Type::Obj)) t = t.unspecialize();
-
-    ht.guardTypeLocation(lt.location, t);
-    curBlock->addPredicted(sk, RegionDesc::TypePred{lt.location, t});
-  }
-
-  while (true) {
-    NormalizedInstruction inst;
-    inst.source = sk;
-    if (!prepareInstruction(inst, state)) return region;
-
-    // Before doing the translation, check for tracelet-ending control flow.
-    if (inst.op() == OpJmp && inst.imm[0].u_BA > 0 &&
-        numJmps < Transl::Translator::MaxJmpsTracedThrough) {
-      // Include the Jmp in the region and continue to its destination.
-      ++numJmps;
-      addInstruction();
-      sk.setOffset(sk.offset() + inst.imm[0].u_BA);
-      blockFinished = true;
-
-      ht.setBcOff(sk.offset(), false);
-      continue;
-    } else if (Transl::opcodeBreaksBB(inst.op()) ||
-               (Transl::dontGuardAnyInputs(inst.op()) &&
-                Transl::opcodeChangesPC(inst.op()))) {
-      // This instruction ends the tracelet. Include it in the region and
-      // return.
-      addInstruction();
-      return region;
+    for (; m_pendingLiterals; --m_pendingLiterals) {
+      m_curBlock->addInstruction();
     }
-
-    inst.interp = toInterp.count(sk);
-    auto const doPrediction = Transl::outputIsPredicted(state.startSk, inst);
-
-    try {
-      irTrans.translateInstr(inst);
-    } catch (const FailedIRGen& exn) {
-      FTRACE(1, "ir generation for {} failed with {}\n",
-             inst.toString(), exn.what());
-      always_assert(!toInterp.count(sk));
-      toInterp.insert(sk);
-      return RegionDescPtr{ nullptr };
-    }
-
-    if (isFCallStar(inst.op()) || inst.op() == OpFCallBuiltin) {
-      // This is much more conservative than it needs to be.
-      ht.emitSmashLocals();
-    }
-
-    if (doPrediction) {
-      ht.checkTypeStack(0, inst.outPred,
-                        sk.advanced(curBlock->unit()).offset());
-    }
-
-    addInstruction();
-    sk.advance(curBlock->unit());
+    m_curBlock->addInstruction();
   }
 }
 }
@@ -194,22 +252,21 @@ RegionDescPtr regionTraceletImpl(const RegionContext& ctx,
  * given context. The region will be broken before the first instruction that
  * attempts to consume an input with an insufficiently precise type.
  *
+ * Always returns a RegionDesc containing at least one instruction.
  */
 RegionDescPtr selectTracelet(const RegionContext& ctx) {
   InterpSet interp;
   RegionDescPtr region;
   uint32_t tries = 1;
 
-  while (!(region = regionTraceletImpl(ctx, interp))) {
+  while (!(region = RegionFormer(ctx, interp).go())) {
     ++tries;
   }
   FTRACE(1, "regionTracelet returning after {} tries:\n{}\n",
          tries, show(*region));
 
-  if (region->blocks.size() == 1 && region->blocks.front()->length() == 0) {
-    // If we had to break the region at the first instruction, punt.
-    return RegionDescPtr{ nullptr };
-  } else if (region->blocks.back()->length() == 0) {
+  assert(region->blocks.size() > 0 && region->blocks.front()->length() > 0);
+  if (region->blocks.back()->length() == 0) {
     // If the final block is empty because it would've only contained
     // instructions producing literal values, kill it.
     region->blocks.pop_back();

hphp/runtime/vm/jit/trace-builder.cpp

@@ -627,14 +627,19 @@ SSATmp* TraceBuilder::preOptimizeAssertLoc(IRInstruction* inst) {
                                     "wrong about a local variable's type.");
       auto* errorInst = m_irFactory.gen(RaiseError, inst->marker(), cns(error));
       inst->become(&m_irFactory, errorInst);
-      assert_log(false,  [&]{
-          IRTrace& mainTrace = trace()->isMain() ? *trace()
-                                                 : *(trace()->main());
-          return folly::format("\npreOptimizeAssertLoc: prevType: {} "
-                               "typeParam: {}\nin instr: {}\nin trace: {}\n",
-                               prevType.toString(), typeParam.toString(),
-                               inst->toString(), mainTrace.toString()).str();
-        });
+
+      // It's not a disaster to generate this in unreachable code for
+      // now. t2590033.
+      if (false) {
+        assert_log(false,  [&]{
+            IRTrace& mainTrace = trace()->isMain() ? *trace()
+                                                   : *(trace()->main());
+            return folly::format("\npreOptimizeAssertLoc: prevType: {} "
+                                 "typeParam: {}\nin instr: {}\nin trace: {}\n",
+                                 prevType.toString(), typeParam.toString(),
+                                 inst->toString(), mainTrace.toString()).str();
+          });
+      }
     } else {
       inst->convertToNop();
     }

hphp/runtime/vm/jit/translator-x64.cpp

@@ -3352,8 +3352,6 @@ TranslatorX64::translateWork(const TranslArgs& args) {
           m_mode = TransLive;
         }
         result = translateTracelet(t);
-        DEBUG_ONLY static const bool reqRegion = getenv("HHVM_REQUIRE_REGION");
-        assert(IMPLIES(region && reqRegion, result != Success));
       }
 
       if (result != Success) {

hphp/runtime/vm/jit/translator.cpp

@@ -244,9 +244,7 @@ RuntimeType Translator::liveType(Location l,
 }
 
 RuntimeType
-Translator::liveType(const Cell* outer,
-                     const Location& l,
-                     bool specialize) {
+Translator::liveType(const Cell* outer, const Location& l, bool specialize) {
   always_assert(analysisDepth() == 0);
 
   if (!outer) {
@@ -265,9 +263,9 @@ Translator::liveType(const Cell* outer,
     assert(IS_REAL_TYPE(innerType));
     valueType = innerType;
     assert(innerType != KindOfRef);
-    TRACE(2, "liveType Var -> %d\n", innerType);
+    FTRACE(2, "liveType {}: Var -> {}\n", l.pretty(), tname(innerType));
   } else {
-    TRACE(2, "liveType %d\n", outerType);
+    FTRACE(2, "liveType {}: {}\n", l.pretty(), tname(outerType));
   }
   const Class *klass = nullptr;
   if (valueType == KindOfObject) {
@@ -2075,20 +2073,7 @@ void Translator::getOutputs(/*inout*/ Tracelet& t,
       // Pseudo-outputs that affect translator state
       case FStack: {
         currentStackOffset += kNumActRecCells;
-        if (op == OpFPushFuncD || op == OpFPushFuncU) {
-          const Unit& cu = *ni->unit();
-          Id funcId = ni->imm[1].u_SA;
-          const NamedEntityPair &nep = cu.lookupNamedEntityPairId(funcId);
-          const Func* f = Unit::lookupFunc(nep.second);
-          if (f && f->isNameBindingImmutable(&cu)) {
-            t.m_arState.pushFuncD(f);
-          } else {
-            t.m_arState.pushDynFunc();
-          }
-        } else {
-          // Non-deterministic in some way
-          t.m_arState.pushDynFunc();
-        }
+        t.m_arState.pushFunc(*ni);
       } continue; // no instr-associated output
 
       case Local: {
@@ -2959,10 +2944,30 @@ void Translator::relaxDeps(Tracelet& tclet, TraceletContext& tctxt) {
   }
 }
 
-static bool checkTaintFuncs(StringData* name) {
-  static const StringData* s_extract =
-    StringData::GetStaticString("extract");
-  return name->isame(s_extract);
+bool callDestroysLocals(const NormalizedInstruction& inst,
+                        const Func* caller) {
+  auto* unit = caller->unit();
+  auto checkTaintId = [&](Id id) {
+    static const StringData* s_extract = StringData::GetStaticString("extract");
+    return unit->lookupLitstrId(id)->isame(s_extract);
+  };
+
+  if (inst.op() == OpFCallBuiltin) return checkTaintId(inst.imm[2].u_SA);
+  if (!isFCallStar(inst.op()))     return false;
+
+  const FPIEnt *fpi = caller->findFPI(inst.source.offset());
+  assert(fpi);
+  Op* fpushPc = (Op*)unit->at(fpi->m_fpushOff);
+  auto const op = *fpushPc;
+
+  if (op == OpFPushFunc)  return true;
+  if (op == OpFPushFuncD) return checkTaintId(getImm(fpushPc, 1).u_SA);
+  if (op == OpFPushFuncU) {
+    return checkTaintId(getImm(fpushPc, 1).u_SA) ||
+           checkTaintId(getImm(fpushPc, 2).u_SA);
+  }
+
+  return false;
 }
 
 /*
@@ -3281,8 +3286,7 @@ std::unique_ptr<Tracelet> Translator::analyze(SrcKey sk,
     NormalizedInstruction* ni = t.newNormalizedInstruction();
     ni->source = sk;
     ni->stackOffset = stackFrameOffset;
-    ni->funcd = (t.m_arState.getCurrentState() == ActRecState::State::KNOWN) ?
-      t.m_arState.getCurrentFunc() : nullptr;
+    ni->funcd = t.m_arState.knownFunc();
     ni->m_unit = unit;
     ni->breaksTracelet = false;
     ni->changesPC = opcodeChangesPC(ni->op());
@@ -3315,11 +3319,11 @@ std::unique_ptr<Tracelet> Translator::analyze(SrcKey sk,
         // exception, do so here.
         int argNum = ni->imm[0].u_IVA;
         // instrSpToArDelta() returns the delta relative to the sp at the
-        // beginning of the instruction, but getReffiness() wants the delta
+        // beginning of the instruction, but checkByRef() wants the delta
         // relative to the sp at the beginning of the tracelet, so we adjust
         // by subtracting ni->stackOff
         int entryArDelta = instrSpToArDelta((Op*)ni->pc()) - ni->stackOffset;
-        ni->preppedByRef = t.m_arState.getReffiness(argNum, entryArDelta,
+        ni->preppedByRef = t.m_arState.checkByRef(argNum, entryArDelta,
                                                     &t.m_refDeps);
         SKTRACE(1, sk, "passing arg%d by %s\n", argNum,
                 ni->preppedByRef ? "reference" : "value");
@@ -3377,7 +3381,6 @@ std::unique_ptr<Tracelet> Translator::analyze(SrcKey sk,
     SKTRACE(2, sk, "stack args: virtual sfo now %d\n", stackFrameOffset);
 
     bool doVarEnvTaint; // initialized by reference.
-
     try {
       getOutputs(t, ni, stackFrameOffset, doVarEnvTaint);
     } catch (TranslationFailedExc& tfe) {
@@ -3391,34 +3394,8 @@ std::unique_ptr<Tracelet> Translator::analyze(SrcKey sk,
       goto breakBB;
     }
 
-    if (isFCallStar(ni->op())) {
-      if (!doVarEnvTaint) {
-        const FPIEnt *fpi = curFunc()->findFPI(ni->source.offset());
-        assert(fpi);
-        Offset fpushOff = fpi->m_fpushOff;
-        PC fpushPc = curUnit()->at(fpushOff);
-        auto const op = toOp(*fpushPc);
-        if (op == OpFPushFunc) {
-          doVarEnvTaint = true;
-        } else if (op == OpFPushFuncD) {
-          StringData *funcName =
-            curUnit()->lookupLitstrId(getImm((Op*)fpushPc, 1).u_SA);
-          doVarEnvTaint = checkTaintFuncs(funcName);
-        } else if (op == OpFPushFuncU) {
-          StringData *fallbackName =
-            curUnit()->lookupLitstrId(getImm((Op*)fpushPc, 2).u_SA);
-          doVarEnvTaint = checkTaintFuncs(fallbackName);
-        }
-      }
-      t.m_arState.pop();
-    }
-    if (ni->op() == OpFCallBuiltin && !doVarEnvTaint) {
-      StringData* funcName = curUnit()->lookupLitstrId(ni->imm[2].u_SA);
-      doVarEnvTaint = checkTaintFuncs(funcName);
-    }
-    if (doVarEnvTaint) {
-      tas.varEnvTaint();
-    }
+    if (isFCallStar(ni->op())) t.m_arState.pop();
+    if (doVarEnvTaint || callDestroysLocals(*ni, curFunc())) tas.varEnvTaint();
 
     DynLocation* outputs[] = { ni->outStack,
                                ni->outLocal, ni->outLocal2,
@@ -3856,6 +3833,10 @@ Translator::translateRegion(const RegionDesc& region,
     auto knownFuncs = makeMapWalker(block->knownFuncs());
 
     for (unsigned i = 0; i < block->length(); ++i, sk.advance(block->unit())) {
+      // Update bcOff here so any guards or assertions from metadata are
+      // attributed to this instruction.
+      ht.setBcOff(sk.offset(), false);
+
       // Emit prediction guards. If this is the first instruction in the
       // region the guards will go to a retranslate request. Otherwise, they'll
       // go to a side exit.
@@ -3942,8 +3923,7 @@ Translator::translateRegion(const RegionDesc& region,
       readMetaData(metaHand, inst, ht);
       if (!inst.noOp && inputInfos.needsRefCheck) {
         assert(byRefs.hasNext(sk));
-        auto byRef = byRefs.next();
-        inst.preppedByRef = byRef == RegionDesc::ParamByRef::Yes;
+        inst.preppedByRef = byRefs.next();
       }
 
       // Check for a type prediction. Put it in the NormalizedInstruction so
@@ -4132,9 +4112,27 @@ TransRec::print(uint64_t profCount) const {
   return ret;
 }
 
+void
+ActRecState::pushFunc(const NormalizedInstruction& inst) {
+  assert(isFPush(inst.op()));
+  if (inst.op() == OpFPushFuncD || inst.op() == OpFPushFuncU) {
+    const Unit& unit = *inst.unit();
+    Id funcId = inst.imm[1].u_SA;
+    auto const& nep = unit.lookupNamedEntityPairId(funcId);
+    auto const func = Unit::lookupFunc(nep.second);
+    if (func) func->validate();
+    if (func && func->isNameBindingImmutable(&unit)) {
+      pushFuncD(func);
+      return;
+    }
+  }
+  pushDynFunc();
+}
+
 void
 ActRecState::pushFuncD(const Func* func) {
   TRACE(2, "ActRecState: pushStatic func %p(%s)\n", func, func->name()->data());
+  func->validate();
   Record r;
   r.m_state = State::KNOWN;
   r.m_topFunc = func;
@@ -4160,7 +4158,7 @@ ActRecState::pop() {
 }
 
 /**
- * getReffiness() returns true if the parameter specified by argNum is pass
+ * checkByRef() returns true if the parameter specified by argNum is pass
  * by reference, otherwise it returns false. This function may also throw an
  * UnknownInputException if the reffiness cannot be determined.
  *
@@ -4168,9 +4166,9 @@ ActRecState::pop() {
  * the beginning of the tracelet and ar.
  */
 bool
-ActRecState::getReffiness(int argNum, int entryArDelta, RefDeps* outRefDeps) {
-  assert(outRefDeps);
-  TRACE(2, "ActRecState: getting reffiness for arg %d\n", argNum);
+ActRecState::checkByRef(int argNum, int entryArDelta, RefDeps* refDeps) {
+  FTRACE(2, "ActRecState: getting reffiness for arg {}, arDelta {}\n",
+         argNum, entryArDelta);
   if (m_arStack.empty()) {
     // The ActRec in question was pushed before the beginning of the
     // tracelet, so we can make a guess about parameter reffiness and
@@ -4180,6 +4178,7 @@ ActRecState::getReffiness(int argNum, int entryArDelta, RefDeps* outRefDeps) {
     Record r;
     r.m_state = State::GUESSABLE;
     r.m_entryArDelta = entryArDelta;
+    ar->m_func->validate();
     r.m_topFunc = ar->m_func;
     m_arStack.push_back(r);
   }
@@ -4194,19 +4193,20 @@ ActRecState::getReffiness(int argNum, int entryArDelta, RefDeps* outRefDeps) {
   if (r.m_state == State::GUESSABLE) {
     assert(r.m_entryArDelta != InvalidEntryArDelta);
     TRACE(2, "ActRecState: guessing arg%d -> %d\n", argNum, retval);
-    outRefDeps->addDep(r.m_entryArDelta, argNum, retval);
+    refDeps->addDep(r.m_entryArDelta, argNum, retval);
   }
   return retval;
 }
 
 const Func*
-ActRecState::getCurrentFunc() {
-  if (m_arStack.empty()) return nullptr;
+ActRecState::knownFunc() {
+  if (currentState() != State::KNOWN) return nullptr;
+  assert(!m_arStack.empty());
   return m_arStack.back().m_topFunc;
 }
 
 ActRecState::State
-ActRecState::getCurrentState() {
+ActRecState::currentState() {
   if (m_arStack.empty()) return State::GUESSABLE;
   return m_arStack.back().m_state;
 }

hphp/runtime/vm/jit/translator.h

@@ -363,16 +363,20 @@ class TranslationFailedExc : public std::exception {
     m_file(file), m_line(line) { }
 };
 
-class UnknownInputExc : public std::exception {
+class UnknownInputExc : public std::runtime_error {
  public:
   const char* m_file; // must be static
   const int m_line;
   UnknownInputExc(const char* file, int line)
-    : m_file(file), m_line(line) { }
+    : std::runtime_error(folly::format("UnknownInputExc @ {}:{}",
+                                       file, line).str())
+    , m_file(file)
+    , m_line(line)
+  {}
 };
 
 #define punt() do { \
-  throw TranslationFailedExc(__FILE__, __LINE__); \
+  throw Transl::TranslationFailedExc(__FILE__, __LINE__); \
 } while(0)
 
 #define throwUnknownInput() do { \
@@ -507,12 +511,13 @@ struct ActRecState {
   std::vector<Record> m_arStack;
 
   ActRecState() {}
+  void pushFunc(const NormalizedInstruction& ni);
   void pushFuncD(const Func* func);
   void pushDynFunc();
   void pop();
-  bool getReffiness(int argNum, int stackOffset, RefDeps* outRefDeps);
-  const Func* getCurrentFunc();
-  State getCurrentState();
+  bool checkByRef(int argNum, int stackOffset, RefDeps* outRefDeps);
+  const Func* knownFunc();
+  State currentState();
 };
 
 struct Tracelet : private boost::noncopyable {
@@ -1088,6 +1093,8 @@ void getInputsImpl(SrcKey startSk, NormalizedInstruction* inst,
                    int& currentStackOffset, InputInfos& inputs,
                    const LocalTypeFn& localType);
 bool outputIsPredicted(SrcKey startSk, NormalizedInstruction& inst);
+bool callDestroysLocals(const NormalizedInstruction& inst,
+                        const Func* caller);
 int locPhysicalOffset(Location l, const Func* f = nullptr);
 
 namespace InstrFlags {