Clear local related helpers out of TraceBuilder

Optimizations went to Simplifier or preOptimize (or were
removed in some cases where they duplicated things already done in
simplifier), php-semantics went to hhbctranslator.

hphp/doc/ir.specification

@@ -343,7 +343,7 @@ AssertLoc<T,localId> S0:FramePtr
   GuardLoc except it doesn't imply a runtime check and cannot cause
   control flow.
 
-OverrideLoc<T,localId> S0:StkPtr
+OverrideLoc<T,localId> S0:FramePtr
 
   Overrides tracked information about the type of a local in frame S0.
   This is used to fix tracked state after InterpOne instructions.
@@ -990,8 +990,18 @@ StLocNT<localId> S0:FramePtr S1:Gen
   Store S1 to local number localId on the frame pointed to by S0,
   without storing the type.
 
-StRef
-StRefNT
+D:BoxedCell = StRef S0:BoxedCell S1:Cell
+
+  Store the value in S1 into the RefData pointed to by S0.  Stores the
+  RefData::m_type also.  Returns a new value for S0, which will now
+  have the type of S1 after boxing.
+
+D:BoxedCell = StRefNT S0:BoxedCell S1:Cell
+
+  Store the value in S1 into the RefData pointed to by S0.  Does not
+  change RefData::m_type.  Returns a new value for S0, which will now
+  have the type of S1 after boxing.
+
 StRaw
 
 D:StkPtr = SpillStack S0:StkP S1:ConstInt, S2...

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

@@ -1978,7 +1978,6 @@ void CodeGenerator::cgLdFuncCached(IRInstruction* inst) {
   });
 }
 
-
 void CodeGenerator::cgLdFuncCachedSafe(IRInstruction* inst) {
   cgLdFuncCachedCommon(inst);
   if (Block* taken = inst->getTaken()) {

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

@@ -605,18 +605,26 @@ void HhbcTranslator::emitInitThisLoc(int32_t id) {
 
 void HhbcTranslator::emitCGetL(int32_t id) {
   Trace* exitTrace = getExitTrace();
-  pushIncRef(emitLdLocWarn(id, exitTrace));
+  pushIncRef(ldLocInnerWarn(id, exitTrace));
 }
 
 void HhbcTranslator::emitCGetL2(int32_t id) {
   Trace* exitTrace = getExitTrace();
   SSATmp* oldTop = pop(Type::Gen);
-  pushIncRef(emitLdLocWarn(id, exitTrace));
+  pushIncRef(ldLocInnerWarn(id, exitTrace));
   push(oldTop);
 }
 
 void HhbcTranslator::emitVGetL(int32_t id) {
-  pushIncRef(m_tb->genBoxLoc(id));
+  auto value = ldLoc(id);
+  if (!value->type().isBoxed()) {
+    if (value->isA(Type::Uninit)) {
+      value = m_tb->genDefInitNull();
+    }
+    value = gen(Box, value);
+    gen(StLoc, LocalId(id), m_tb->getFp(), value);
+  }
+  pushIncRef(value);
 }
 
 void HhbcTranslator::emitUnsetN() {
@@ -630,45 +638,49 @@ void HhbcTranslator::emitUnsetG(const StringData* gblName) {
 }
 
 void HhbcTranslator::emitUnsetL(int32_t id) {
-  m_tb->genBindLoc(id, m_tb->genDefUninit());
+  auto const prev = ldLoc(id);
+  gen(StLoc, LocalId(id), m_tb->getFp(), m_tb->genDefUninit());
+  gen(DecRef, prev);
 }
 
 void HhbcTranslator::emitBindL(int32_t id) {
-  SSATmp* src = popV();
+  auto const newValue = popV();
+  // XXX: does this cause user-visible semantic differences?  What
+  // about the stack being different when we re-enter the dtor?  What
+  // is this for?
   if (getCurFunc()->isPseudoMain()) {
     // in pseudo mains, the value of locals could change in functions
     // called explicitly (or implicitly via exceptions or destructors)
     // so we need to incref eagerly in case one of these functions
     // changes the value of our local and makes src dead.
-    pushIncRef(src);
+    pushIncRef(newValue);
   }
-  m_tb->genBindLoc(id, src);
+  auto const oldValue = ldLoc(id);
+  gen(StLoc, LocalId(id), m_tb->getFp(), newValue);
+  gen(DecRef, oldValue);
   if (!getCurFunc()->isPseudoMain()) {
-    pushIncRef(src);
+    pushIncRef(newValue);
   }
 }
 
 void HhbcTranslator::emitSetL(int32_t id) {
-  Trace* exitTrace = getExitTrace();
-  SSATmp* src = popC();
-  // Note we can't use the same trick as emitBindL in which we
-  // move the incref to after the store because the stored location
-  // might contain a ref, which could then be modified by the decref
-  // inserted after the stloc
-  push(m_tb->genStLoc(id, src, true, true, exitTrace));
+  auto const exitTrace = getExitTrace();
+  auto const src = popC();
+  push(stLoc(id, exitTrace, src));
 }
 
 void HhbcTranslator::emitIncDecL(bool pre, bool inc, uint32_t id) {
-  // Handle only integer inc/dec for now
   Trace* exitTrace = getExitTrace();
-  SSATmp* src = m_tb->genLdLocAsCell(id, exitTrace);
+  auto const src = ldLocInner(id, exitTrace);
+
+  // Inc/Dec of a bool is a no-op.
   if (src->isA(Type::Bool)) {
-    // inc/dec of a bool is a no-op
     push(src);
-  } else {
-    SSATmp* res = emitIncDec(pre, inc, src);
-    m_tb->genStLoc(id, res, false, false, nullptr);
+    return;
   }
+
+  auto const res = emitIncDec(pre, inc, src);
+  stLoc(id, exitTrace, res);
 }
 
 // only handles integer or double inc/dec
@@ -693,41 +705,43 @@ void HhbcTranslator::emitIncDecMem(bool pre,
   gen(StMemNT, propAddr, cns(0), res);
 }
 
-static bool isSupportedBinaryArith(Opcode opc,
-                                   Type t1,
-                                   Type t2) {
+static bool isSupportedBinaryArith(Opcode opc, Type t1, Type t2) {
   switch (opc) {
-    // Opcodes supporting FP
-    case OpAdd:
-    case OpSub:
-    case OpMul: return (t1.subtypeOf(Type::Int | Type::Bool | Type::Dbl) &&
-                        t2.subtypeOf(Type::Int | Type::Bool | Type::Dbl));
+  case OpAdd:
+  case OpSub:
+  case OpMul: return t1.subtypeOfAny(Type::Int, Type::Bool, Type::Dbl) &&
+                     t2.subtypeOfAny(Type::Int, Type::Bool, Type::Dbl);
 
-    default:    return (t1.subtypeOf(Type::Int | Type::Bool) &&
-                        t2.subtypeOf(Type::Int | Type::Bool));
+  default:    return t1.subtypeOfAny(Type::Int, Type::Bool) &&
+                     t2.subtypeOfAny(Type::Int, Type::Bool);
   }
 }
 
 void HhbcTranslator::emitSetOpL(Opcode subOpc, uint32_t id) {
-  Trace* exitTrace = getExitTrace();
-  SSATmp* loc = emitLdLocWarn(id, exitTrace);
+  auto const exitTrace = getExitTrace();
+  auto const loc       = ldLocInnerWarn(id, exitTrace);
+
   if (subOpc == Concat) {
-    // The concat helpers decref their args, so don't decref pop'ed values
-    // and don't decref the old value held in the local. The concat helpers
-    // also incref their results, which will be consumed by the stloc. We
-    // need an extra incref for the push onto the stack.
-    SSATmp* val = popC();
-    SSATmp* result = gen(Concat, loc, val);
-    pushIncRef(m_tb->genStLoc(id, result, false, true, exitTrace));
-  } else if (isSupportedBinaryArith(subOpc,
-                                    loc->type(),
-                                    topC()->type())) {
-    SSATmp* val = popC();
-    SSATmp* result = gen(subOpc, loc, val);
-    push(m_tb->genStLoc(id, result, true, true, exitTrace));
-  } else {
-    PUNT(SetOpL);
+    /*
+     * The concat helpers decref their args, so don't decref pop'ed values
+     * and don't decref the old value held in the local. The concat helpers
+     * also incref their results, which will be consumed by the stloc. We
+     * need an extra incref for the push onto the stack.
+     */
+    auto const val    = popC();
+    auto const result = gen(Concat, loc, val);
+    pushIncRef(stLocNRC(id, exitTrace, result));
+    return;
   }
+
+  if (isSupportedBinaryArith(subOpc, loc->type(), topC()->type())) {
+    auto const val    = popC();
+    auto const result = gen(subOpc, loc, val);
+    push(stLoc(id, exitTrace, result));
+    return;
+  }
+
+  PUNT(SetOpL);
 }
 
 void HhbcTranslator::emitClassExists(const StringData* clsName) {
@@ -891,10 +905,7 @@ void HhbcTranslator::emitCreateCont(bool getArgs,
       // We must generate an AssertLoc because we don't have tracelet
       // guards on the object type in these outer generator functions.
       gen(AssertLoc, Type::Gen, LocalId(i), m_tb->getFp());
-      auto const loc = gen(
-        IncRef,
-        m_tb->genLdLoc(i)
-      );
+      auto const loc = gen(IncRef, ldLoc(i));
       gen(
         StMem,
         locals,
@@ -955,14 +966,14 @@ void HhbcTranslator::emitContExit() {
 void HhbcTranslator::emitUnpackCont() {
   gen(LinkContVarEnv, m_tb->getFp());
   gen(AssertLoc, Type::Obj, LocalId(0), m_tb->getFp());
-  auto const cont = m_tb->genLdLoc(0);
+  auto const cont = ldLoc(0);
   push(gen(LdRaw, Type::Int, cont, cns(RawMemSlot::ContLabel)));
 }
 
 void HhbcTranslator::emitPackCont(int64_t labelId) {
   gen(UnlinkContVarEnv, m_tb->getFp());
   gen(AssertLoc, Type::Obj, LocalId(0), m_tb->getFp());
-  auto const cont = m_tb->genLdLoc(0);
+  auto const cont = ldLoc(0);
   auto const newVal = popC();
   auto const oldValue = gen(LdProp, Type::Cell, cont, cns(CONTOFF(m_value)));
   gen(StProp, cont, cns(CONTOFF(m_value)), newVal);
@@ -974,7 +985,7 @@ void HhbcTranslator::emitPackCont(int64_t labelId) {
 
 void HhbcTranslator::emitContReceive() {
   gen(AssertLoc, Type::Obj, LocalId(0), m_tb->getFp());
-  auto const cont = m_tb->genLdLoc(0);
+  auto const cont = ldLoc(0);
   gen(ContRaiseCheck, getExitSlowTrace(), cont);
   auto const valOffset = cns(CONTOFF(m_received));
   push(gen(LdProp, Type::Cell, cont, valOffset));
@@ -983,7 +994,7 @@ void HhbcTranslator::emitContReceive() {
 
 void HhbcTranslator::emitContRetC() {
   gen(AssertLoc, Type::Obj, LocalId(0), m_tb->getFp());
-  auto const cont = m_tb->genLdLoc(0);
+  auto const cont = ldLoc(0);
   gen(ExitWhenSurprised, getExitSlowTrace());
   gen(
     StRaw, cont, cns(RawMemSlot::ContDone), cns(true)
@@ -1014,7 +1025,7 @@ void HhbcTranslator::emitContSendImpl(bool raise) {
   gen(ContPreNext, getExitSlowTrace(), cont);
 
   gen(AssertLoc, Type::Cell, LocalId(0), m_tb->getFp());
-  auto const newVal = gen(IncRef, m_tb->genLdLoc(0));
+  auto const newVal = gen(IncRef, ldLoc(0));
   auto const oldVal = gen(LdProp, Type::Cell, cont, cns(CONTOFF(m_received)));
   gen(StProp, cont, cns(CONTOFF(m_received)), newVal);
   gen(DecRef, oldVal);
@@ -1175,16 +1186,9 @@ void HhbcTranslator::emitMInstr(const NormalizedInstruction& ni) {
  * Unboxes var if necessary when var is not uninit.
  */
 void HhbcTranslator::emitIssetL(int32_t id) {
-  Type trackedType = m_tb->getLocalType(id);
-  // guards should ensure we have type info at this point
-  assert(trackedType != Type::None);
-  if (trackedType.subtypeOf(Type::Uninit)) {
-    push(cns(false));
-  } else {
-    Trace* exitTrace = getExitTrace();
-    SSATmp* ld = m_tb->genLdLocAsCell(id, exitTrace);
-    push(gen(IsNType, Type::Null, ld));
-  }
+  auto const exitTrace = getExitTrace();
+  auto const ld = ldLocInner(id, exitTrace);
+  push(gen(IsNType, Type::Null, ld));
 }
 
 void HhbcTranslator::emitIssetG(const StringData* gblName) {
@@ -1200,15 +1204,9 @@ void HhbcTranslator::emitIssetS(const StringData* propName) {
 }
 
 void HhbcTranslator::emitEmptyL(int32_t id) {
-  Type trackedType = m_tb->getLocalType(id);
-  assert(trackedType != Type::None);
-  if (trackedType == Type::Uninit) {
-    push(cns(true));
-  } else {
-    Trace* exitTrace = getExitTrace();
-    SSATmp* ld = m_tb->genLdLocAsCell(id, exitTrace);
-    push(gen(OpNot, gen(ConvCellToBool, ld)));
-  }
+  auto const exitTrace = getExitTrace();
+  auto const ld = ldLocInner(id, exitTrace);
+  push(gen(OpNot, gen(ConvCellToBool, ld)));
 }
 
 void HhbcTranslator::emitEmptyG(const StringData* gblName) {
@@ -1231,7 +1229,7 @@ void HhbcTranslator::emitIsTypeC(Type t) {
 
 void HhbcTranslator::emitIsTypeL(Type t, int id) {
   Trace* exitTrace = getExitTrace();
-  push(gen(IsType, t, emitLdLocWarn(id, exitTrace)));
+  push(gen(IsType, t, ldLocInnerWarn(id, exitTrace)));
 }
 
 void HhbcTranslator::emitIsNullL(int id)   { emitIsTypeL(Type::Null, id);}
@@ -1857,7 +1855,7 @@ void HhbcTranslator::emitFCallBuiltin(uint32_t numArgs,
         break;
       case KindOfDouble: assert(false);
       default:
-        args[i + 1] = loadStackAddr(numArgs - i - 1);
+        args[i + 1] = ldStackAddr(numArgs - i - 1);
         break;
     }
   }
@@ -1957,8 +1955,21 @@ SSATmp* HhbcTranslator::emitDecRefLocalsInline(SSATmp* retVal) {
   for (int id = getCurFunc()->numLocals() - 1; id >= 0; --id) {
     if (retValLocId == id) {
       gen(DecRef, retVal);
+      continue;
+    }
+
+    // When a parameter goes out of scope during return, we need to
+    // null it out so that debug_backtrace can't capture stale values.
+    // TODO(#2332775): we shouldn't have to do this.
+    const bool needSetNull = id < getCurFunc()->numParams();
+    if (needSetNull) {
+      auto const loc = ldLoc(id);
+      if (needSetNull) {
+        gen(StLoc, LocalId(id), m_tb->getFp(), m_tb->genDefUninit());
+      }
+      gen(DecRef, loc);
     } else {
-      m_tb->genDecRefLoc(id);
+      gen(DecRefLoc, Type::Gen, LocalId(id), m_tb->getFp());
     }
   }
 
@@ -2216,7 +2227,7 @@ void HhbcTranslator::emitLoadDeps() {
   for (auto& guard : m_typeGuards) {
     switch (guard.getKind()) {
     case TypeGuard::Local:
-      m_tb->genLdLoc(guard.getIndex());
+      ldLoc(guard.getIndex());
       break;
     case TypeGuard::Stack:
       break;
@@ -2270,7 +2281,7 @@ Trace* HhbcTranslator::guardRefs(int64_t               entryArDelta,
 void HhbcTranslator::emitVerifyParamType(int32_t paramId) {
   const Func* func = getCurFunc();
   const TypeConstraint& tc = func->params()[paramId].typeConstraint();
-  SSATmp* locVal = m_tb->genLdLoc(paramId);
+  auto locVal = ldLoc(paramId);
   Type locType = locVal->type().unbox();
   assert(locType.isKnownDataType());
 
@@ -2580,7 +2591,7 @@ void HhbcTranslator::emitAGetC(const StringData* clsName) {
 }
 
 void HhbcTranslator::emitAGetL(int id, const StringData* clsName) {
-  SSATmp* src = m_tb->genLdLocAsCell(id, getExitTrace());
+  auto const src = ldLocInner(id, getExitTrace());
   if (isSupportedAGet(src, clsName)) {
     emitAGet(src, clsName);
   } else {
@@ -3035,7 +3046,7 @@ void HhbcTranslator::exceptionBarrier() {
   gen(ExceptionBarrier, sp);
 }
 
-SSATmp* HhbcTranslator::loadStackAddr(int32_t offset) {
+SSATmp* HhbcTranslator::ldStackAddr(int32_t offset) {
   // You're almost certainly doing it wrong if you want to get the address of a
   // stack cell that's in m_evalStack.
   assert(offset >= (int32_t)m_evalStack.numCells());
@@ -3047,13 +3058,47 @@ SSATmp* HhbcTranslator::loadStackAddr(int32_t offset) {
   );
 }
 
-//
-// This is a wrapper to TraceBuilder::genLdLoc() that also emits the
-// RaiseUninitLoc if the local is uninitialized
-//
-SSATmp* HhbcTranslator::emitLdLocWarn(uint32_t id,
-                                      Trace* target) {
-  SSATmp* locVal = m_tb->genLdLocAsCell(id, target);
+SSATmp* HhbcTranslator::ldLoc(uint32_t locId) {
+  return gen(
+    LdLoc,
+    Type::Gen,
+    LocalId(locId),
+    m_tb->getFp()
+  );
+}
+
+SSATmp* HhbcTranslator::ldLocAddr(uint32_t locId) {
+  return gen(
+    LdLocAddr,
+    Type::PtrToGen,
+    LocalId(locId),
+    m_tb->getFp()
+  );
+}
+
+/*
+ * Load a local, and if it's boxed dereference to get the inner cell.
+ *
+ * Note: For boxed values, this will generate a LdRef instruction which
+ *       takes the given exit trace in case the inner type doesn't match
+ *       the tracked type for this local.  This check may be optimized away
+ *       if we can determine that the inner type must match the tracked type.
+ */
+SSATmp* HhbcTranslator::ldLocInner(uint32_t locId, Trace* exitTrace) {
+  auto loc = ldLoc(locId);
+  assert((loc->type().isBoxed() || loc->type().notBoxed()) &&
+         "Currently we don't handle traces where locals are maybeBoxed");
+  return loc->type().isBoxed()
+    ? gen(LdRef, loc->type().innerType(), exitTrace, loc)
+    : loc;
+}
+
+/*
+ * This is a wrapper to ldLocInner that also emits the RaiseUninitLoc
+ * if the local is uninitialized
+ */
+SSATmp* HhbcTranslator::ldLocInnerWarn(uint32_t id, Trace* target) {
+  auto const locVal = ldLocInner(id, target);
 
   if (locVal->type().subtypeOf(Type::Uninit)) {
     exceptionBarrier();
@@ -3064,6 +3109,59 @@ SSATmp* HhbcTranslator::emitLdLocWarn(uint32_t id,
   return locVal;
 }
 
+/*
+ * Store to a local, if it's boxed set the value on the inner cell.
+ *
+ * Returns the value that was stored to the local, after incrementing
+ * its reference count.
+ *
+ * Pre: !newVal->type().isBoxed() && !newVal->type().maybeBoxed()
+ * Pre: exitTrace != nullptr if the local may be boxed
+ */
+SSATmp* HhbcTranslator::stLocImpl(uint32_t id,
+                                  Trace* exitTrace,
+                                  SSATmp* newVal,
+                                  bool doRefCount) {
+  assert(!newVal->type().maybeBoxed());
+
+  auto const oldLoc = ldLoc(id);
+  assert((oldLoc->type().isBoxed() || oldLoc->type().notBoxed()) &&
+         "We don't support maybeBoxed locals right now");
+
+  if (oldLoc->type().notBoxed()) {
+    gen(StLoc, LocalId(id), m_tb->getFp(), newVal);
+    auto const ret = doRefCount ? gen(IncRef, newVal) : newVal;
+    if (doRefCount) {
+      gen(DecRef, oldLoc);
+    }
+    return ret;
+  }
+
+  // It's important that the IncRef happens after the LdRef, since the
+  // LdRef is also a guard on the inner type and may side-exit.
+  assert(exitTrace);
+  auto const innerCell = gen(
+    LdRef, oldLoc->type().innerType(), exitTrace, oldLoc
+  );
+  auto const ret = doRefCount ? gen(IncRef, newVal) : newVal;
+  gen(StRef, oldLoc, newVal);
+  if (doRefCount) {
+    gen(DecRef, innerCell);
+  }
+
+  return ret;
+}
+
+SSATmp* HhbcTranslator::stLoc(uint32_t id, Trace* exit, SSATmp* newVal) {
+  const bool doRefCount = true;
+  return stLocImpl(id, exit, newVal, doRefCount);
+}
+
+SSATmp* HhbcTranslator::stLocNRC(uint32_t id, Trace* exit, SSATmp* newVal) {
+  const bool doRefCount = false;
+  return stLocImpl(id, exit, newVal, doRefCount);
+}
+
 void HhbcTranslator::end(int nextPc) {
   if (m_hasExit) return;
 

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

@@ -547,7 +547,6 @@ private:
   Trace* getExitTrace(uint32_t targetBcOff, uint32_t notTakenBcOff);
   Trace* getExitSlowTrace();
   Trace* getGuardExit();
-  SSATmp* emitLdLocWarn(uint32_t id, Trace* target);
   void emitInterpOneOrPunt(Type type, int numPopped, int numExtraPushed = 0);
   void emitBinaryArith(Opcode);
   template<class Lambda>
@@ -580,7 +579,7 @@ private:
   const NamedEntityPair& lookupNamedEntityPairId(int id);
 
   /*
-   * Eval stack helpers
+   * Eval stack helpers.
    */
   SSATmp* push(SSATmp* tmp);
   SSATmp* pushIncRef(SSATmp* tmp) { return push(gen(IncRef, tmp)); }
@@ -596,12 +595,23 @@ private:
   std::vector<SSATmp*> getSpillValues() const;
   SSATmp* spillStack();
   void    exceptionBarrier();
-  SSATmp* loadStackAddr(int32_t offset);
+  SSATmp* ldStackAddr(int32_t offset);
   SSATmp* top(Type type, uint32_t index = 0);
   void    extendStack(uint32_t index, Type type);
   void    replace(uint32_t index, SSATmp* tmp);
   void    refineType(SSATmp* tmp, Type type);
 
+  /*
+   * Local instruction helpers.
+   */
+  SSATmp* ldLoc(uint32_t id);
+  SSATmp* ldLocAddr(uint32_t id);
+  SSATmp* ldLocInner(uint32_t id, Trace* exitTrace);
+  SSATmp* ldLocInnerWarn(uint32_t id, Trace* target);
+  SSATmp* stLoc(uint32_t id, Trace* exitTrace, SSATmp* newVal);
+  SSATmp* stLocNRC(uint32_t id, Trace* exitTrace, SSATmp* newVal);
+  SSATmp* stLocImpl(uint32_t id, Trace*, SSATmp* newVal, bool doRefCount);
+
 private:
   // Tracks information about the current bytecode offset and which
   // function we are in.  Goes in m_bcStateStack; we push and pop as

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

@@ -343,8 +343,8 @@ O(StProp,                           ND, S(Obj) S(Int) S(Gen), E|Mem|CRc|Refs) \
 O(StPropNT,                         ND, S(Obj) S(Int) S(Gen),      E|Mem|CRc) \
 O(StLoc,                            ND, S(FramePtr) S(Gen),        E|Mem|CRc) \
 O(StLocNT,                          ND, S(FramePtr) S(Gen),        E|Mem|CRc) \
-O(StRef,                       DBox(1), SUnk,                 E|Mem|CRc|Refs) \
-O(StRefNT,                     DBox(1), SUnk,                      E|Mem|CRc) \
+O(StRef,                       DBox(1), S(BoxedCell) S(Cell), E|Mem|CRc|Refs) \
+O(StRefNT,                     DBox(1), S(BoxedCell) S(Cell),      E|Mem|CRc) \
 O(StRaw,                            ND, SUnk,                          E|Mem) \
 O(LdStaticLocCached,      D(BoxedCell), C(CacheHandle),                   NF) \
 O(StaticLocInit,          D(BoxedCell), CStr                                  \

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

@@ -295,6 +295,9 @@ SSATmp* Simplifier::simplify(IRInstruction* inst) {
   case LdStack:      return simplifyLdStack(inst);
   case LdStackAddr:  return simplifyLdStackAddr(inst);
   case DecRefStack:  return simplifyDecRefStack(inst);
+  case DecRefLoc:    return simplifyDecRefLoc(inst);
+  case LdLoc:        return simplifyLdLoc(inst);
+  case StRef:        return simplifyStRef(inst);
 
   case ExitOnVarEnv: return simplifyExitOnVarEnv(inst);
 
@@ -1615,6 +1618,32 @@ SSATmp* Simplifier::simplifyLdStack(IRInstruction* inst) {
   return nullptr;
 }
 
+SSATmp* Simplifier::simplifyDecRefLoc(IRInstruction* inst) {
+  if (inst->getTypeParam().notCounted()) {
+    inst->convertToNop();
+  }
+  return nullptr;
+}
+
+SSATmp* Simplifier::simplifyLdLoc(IRInstruction* inst) {
+  if (inst->getTypeParam().isNull()) {
+    return cns(inst->getTypeParam());
+  }
+  return nullptr;
+}
+
+// Replace StRef with StRefNT when we know we aren't going to change
+// its m_type field.
+SSATmp* Simplifier::simplifyStRef(IRInstruction* inst) {
+  auto const oldUnbox = inst->getSrc(0)->type().unbox();
+  auto const newType = inst->getSrc(1)->type();
+  if (oldUnbox.isKnownDataType() &&
+      oldUnbox.equals(newType) && !oldUnbox.isString()) {
+    inst->setOpcode(StRefNT);
+  }
+  return nullptr;
+}
+
 SSATmp* Simplifier::simplifyLdStackAddr(IRInstruction* inst) {
   auto const info = getStackValue(inst->getSrc(0),
                                   inst->getExtra<StackOffset>()->offset);

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

@@ -123,6 +123,9 @@ private:
   SSATmp* simplifyLdStack(IRInstruction*);
   SSATmp* simplifyLdStackAddr(IRInstruction*);
   SSATmp* simplifyDecRefStack(IRInstruction*);
+  SSATmp* simplifyDecRefLoc(IRInstruction*);
+  SSATmp* simplifyLdLoc(IRInstruction*);
+  SSATmp* simplifyStRef(IRInstruction*);
 
 private: // tracebuilder forwarders
   template<class... Args> SSATmp* cns(Args&&...);

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

@@ -28,10 +28,6 @@ namespace JIT {
 
 static const HPHP::Trace::Module TRACEMOD = HPHP::Trace::hhir;
 
-static inline Type noneToGen(Type t) {
-  return t.subtypeOf(Type::None) ? Type::Gen : t;
-}
-
 TraceBuilder::TraceBuilder(Offset initialBcOffset,
                            uint32_t initialSpOffsetFromFp,
                            IRFactory& irFactory,
@@ -226,190 +222,6 @@ SSATmp* TraceBuilder::genDefNone() {
   return gen(DefConst, Type::None, ConstData(0));
 }
 
-SSATmp* TraceBuilder::genBoxLoc(uint32_t id) {
-  SSATmp* prevValue  = genLdLoc(id);
-  Type prevType = prevValue->type();
-  // Don't box if local's value already boxed
-  if (prevType.isBoxed()) {
-    return prevValue;
-  }
-  assert(prevType.notBoxed());
-  // The Box helper requires us to incref the values its boxing, but in
-  // this case we don't need to incref prevValue because we are simply
-  // transfering its refcount from the local to the box.
-  if (prevValue->isA(Type::Uninit)) {
-    // No box can ever contain Uninit, so promote it to InitNull here.
-    prevValue = genDefInitNull();
-  }
-  SSATmp* newValue = gen(Box, prevValue);
-  gen(StLoc, LocalId(id), m_fpValue, newValue);
-  return newValue;
-}
-
-/**
- * Returns an SSATmp containing the current value of the given local.
- * This generates a LdLoc instruction if needed.
- *
- * Note: the type of the local must be known already (due to type guards
- *       or assertions).
- */
-SSATmp* TraceBuilder::genLdLoc(uint32_t id) {
-  SSATmp* tmp = getLocalValue(id);
-  if (tmp) {
-    return tmp;
-  }
-  // No prior value for this local is available, so actually generate a LdLoc.
-  auto type = getLocalType(id);
-  assert(type != Type::None); // tracelet guards guarantee we have a type
-  assert(type != Type::Null); // we can get Uninit or InitNull but not both
-  if (type.isNull()) {
-    tmp = cns(type);
-  } else {
-    tmp = gen(LdLoc, type, LocalId(id), m_fpValue);
-  }
-  return tmp;
-}
-
-SSATmp* TraceBuilder::genLdLocAsCell(uint32_t id, Trace* exitTrace) {
-  SSATmp*    tmp = genLdLoc(id);
-  Type type = tmp->type();
-  assert(type.isBoxed() || type.notBoxed());
-  if (!type.isBoxed()) {
-    return tmp;
-  }
-  // Unbox tmp into a cell via a LdRef
-  return gen(LdRef, type.innerType(), exitTrace, tmp);
-}
-
-SSATmp* TraceBuilder::genLdLocAddr(uint32_t id) {
-  return gen(LdLocAddr, getLocalType(id).ptr(), LocalId(id), getFp());
-}
-
-void TraceBuilder::genDecRefLoc(int id) {
-  Type type = getLocalType(id);
-
-  // Don't generate code if type is not refcounted
-  if (type != Type::None && type.notCounted()) {
-    return;
-  }
-  type = noneToGen(type);
-  // When a parameter goes out of scope, we need to null
-  // it out so that debug_backtrace can't capture stale
-  // values.
-  bool setNull = id < m_curFunc->getValFunc()->numParams();
-  SSATmp* val = getLocalValue(id);
-  if (val == nullptr && setNull) {
-    val = gen(LdLoc, type, LocalId(id), m_fpValue);
-  }
-  if (val) {
-    if (setNull) {
-      gen(StLoc, LocalId(id), m_fpValue, genDefUninit());
-    }
-    gen(DecRef, val);
-    return;
-  }
-
-  if (type.isBoxed()) {
-    // we can't really rely on the types held in the boxed values since
-    // aliasing stores may change them. We conservatively set the type
-    // of the decref to a boxed cell.
-    type = Type::BoxedCell;
-  }
-
-  gen(DecRefLoc, type, LocalId(id), m_fpValue);
-}
-
-/*
- * Stores a ref (boxed value) to a local. Also handles unsetting a local.
- */
-void TraceBuilder::genBindLoc(uint32_t id,
-                              SSATmp* newValue,
-                              bool doRefCount /* = true */) {
-  Type trackedType = getLocalType(id);
-  SSATmp* prevValue = 0;
-  if (trackedType == Type::None) {
-    if (doRefCount) {
-      prevValue = gen(LdLoc, Type::Gen, LocalId(id), m_fpValue);
-    }
-  } else {
-    prevValue = getLocalValue(id);
-    assert(prevValue == nullptr || prevValue->type() == trackedType);
-    if (prevValue == newValue) {
-      // Silent store: local already contains value being stored
-      // NewValue needs to be decref'ed
-      if (!trackedType.notCounted() && doRefCount) {
-        gen(DecRef, prevValue);
-      }
-      return;
-    }
-    if (trackedType.maybeCounted() && !prevValue && doRefCount) {
-      prevValue = gen(LdLoc, trackedType, LocalId(id), m_fpValue);
-    }
-  }
-  bool genStoreType = true;
-  if ((trackedType.isBoxed() && newValue->type().isBoxed()) ||
-      (trackedType == newValue->type() && !trackedType.isString())) {
-    // no need to store type with local value
-    genStoreType = false;
-  }
-  gen(genStoreType ? StLoc : StLocNT, LocalId(id), m_fpValue, newValue);
-  if (trackedType.maybeCounted() && doRefCount) {
-    gen(DecRef, prevValue);
-  }
-}
-
-/*
- * Store a cell value to a local that might be boxed.
- */
-SSATmp* TraceBuilder::genStLoc(uint32_t id,
-                               SSATmp* newValue,
-                               bool doRefCount,
-                               bool genStoreType,
-                               Trace* exit) {
-  assert(!newValue->type().isBoxed());
-  /*
-   * If prior value of local is a cell, then  re-use genBindLoc.
-   * Otherwise, if prior value of local is a ref:
-   *
-   * prevLocValue = LdLoc<T>{id} fp
-   *    prevValue = LdRef [prevLocValue]
-   *       newRef = StRef [prevLocValue], newValue
-   * DecRef prevValue
-   * -- track local value in newRef
-   */
-  Type trackedType = getLocalType(id);
-  assert(trackedType != Type::None);  // tracelet guards guarantee a type
-  if (trackedType.notBoxed()) {
-    SSATmp* retVal = doRefCount ? gen(IncRef, newValue) : newValue;
-    genBindLoc(id, newValue, doRefCount);
-    return retVal;
-  }
-  assert(trackedType.isBoxed());
-  SSATmp* prevRef = getLocalValue(id);
-  assert(prevRef == nullptr || prevRef->type() == trackedType);
-  // prevRef is a ref
-  if (prevRef == nullptr) {
-    // prevRef = ldLoc
-    prevRef = gen(LdLoc, trackedType, LocalId(id), m_fpValue);
-  }
-  SSATmp* prevValue = nullptr;
-  if (doRefCount) {
-    assert(exit);
-    Type innerType = trackedType.innerType();
-    prevValue = gen(LdRef, innerType, exit, prevRef);
-  }
-  // stref [prevRef] = t1
-  Opcode opc = genStoreType ? StRef : StRefNT;
-  gen(opc, prevRef, newValue);
-
-  SSATmp* retVal = newValue;
-  if (doRefCount) {
-    retVal = gen(IncRef, newValue);
-    gen(DecRef, prevValue);
-  }
-  return retVal;
-}
-
 void TraceBuilder::updateTrackedState(IRInstruction* inst) {
   Opcode opc = inst->op();
   // Update tracked state of local values/types, stack/frame pointer, CSE, etc.
@@ -930,6 +742,81 @@ SSATmp* TraceBuilder::preOptimizeDecRefThis(IRInstruction* inst) {
   return nullptr;
 }
 
+SSATmp* TraceBuilder::preOptimizeDecRefLoc(IRInstruction* inst) {
+  auto const locId = inst->getExtra<DecRefLoc>()->locId;
+
+  /*
+   * Refine the type if we can.
+   *
+   * We can't really rely on the types held in the boxed values since
+   * aliasing stores may change them, and we only guard during LdRef.
+   * So we have to change any boxed type to BoxedCell.
+   */
+  auto knownType = getLocalType(locId);
+  if (knownType.isBoxed()) {
+    knownType = Type::BoxedCell;
+  }
+  if (knownType != Type::None) { // TODO(#2135185)
+    inst->setTypeParam(
+      Type::mostRefined(knownType, inst->getTypeParam())
+    );
+  }
+
+  /*
+   * If we have the local value in flight, use a DecRef on it instead
+   * of doing it in memory.
+   */
+  if (auto tmp = getLocalValue(locId)) {
+    gen(DecRef, tmp);
+    inst->convertToNop();
+  }
+
+  return nullptr;
+}
+
+SSATmp* TraceBuilder::preOptimizeLdLoc(IRInstruction* inst) {
+  auto const locId = inst->getExtra<LdLoc>()->locId;
+  if (auto tmp = getLocalValue(locId)) {
+    return tmp;
+  }
+  if (getLocalType(locId) != Type::None) { // TODO(#2135185)
+    inst->setTypeParam(
+      Type::mostRefined(getLocalType(locId), inst->getTypeParam())
+    );
+  }
+  return nullptr;
+}
+
+SSATmp* TraceBuilder::preOptimizeLdLocAddr(IRInstruction* inst) {
+  auto const locId = inst->getExtra<LdLocAddr>()->locId;
+  if (getLocalType(locId) != Type::None) { // TODO(#2135185)
+    inst->setTypeParam(
+      Type::mostRefined(getLocalType(locId).ptr(), inst->getTypeParam())
+    );
+  }
+  return nullptr;
+}
+
+SSATmp* TraceBuilder::preOptimizeStLoc(IRInstruction* inst) {
+  auto const curType = getLocalType(inst->getExtra<StLoc>()->locId);
+  auto const newType = inst->getSrc(1)->type();
+
+  assert(inst->getTypeParam().equals(Type::None));
+
+  // There's no need to store the type if it's going to be the same
+  // KindOfFoo.  We still have to store string types because we don't
+  // guard on KindOfStaticString vs. KindOfString.
+  auto const bothBoxed = curType.isBoxed() && newType.isBoxed();
+  auto const sameUnboxed = curType != Type::None && // TODO(#2135185)
+    curType.isKnownDataType() &&
+    curType.equals(newType) && !curType.isString();
+  if (bothBoxed || sameUnboxed) {
+    inst->setOpcode(StLocNT);
+  }
+
+  return nullptr;
+}
+
 SSATmp* TraceBuilder::preOptimize(IRInstruction* inst) {
 #define X(op) case op: return preOptimize##op(inst)
   switch (inst->op()) {
@@ -939,6 +826,10 @@ SSATmp* TraceBuilder::preOptimize(IRInstruction* inst) {
     X(LdCtx);
     X(DecRef);
     X(DecRefThis);
+    X(DecRefLoc);
+    X(LdLoc);
+    X(LdLocAddr);
+    X(StLoc);
   default:
     break;
   }
@@ -1186,7 +1077,7 @@ void TraceBuilder::dropLocalRefsInnerTypes() {
  * locals, however.
  */
 void TraceBuilder::killLocals() {
-   for (uint32_t i = 0; i < m_localValues.size(); i++) {
+  for (uint32_t i = 0; i < m_localValues.size(); i++) {
     SSATmp* t = m_localValues[i];
     // should not kill DefConst, and LdConst should be replaced by DefConst
     if (!t || t->inst()->op() == DefConst) {

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

@@ -101,6 +101,7 @@ struct TraceBuilder {
                      int32_t prevSPOff);
   void endInlining();
 
+  // XXX: Accessible for inlining, for now.
   void setLocalValue(unsigned id, SSATmp* value);
 
   void setEnableCse(bool val)            { m_enableCse = val; }
@@ -153,36 +154,6 @@ struct TraceBuilder {
     return instr;
   }
 
-  //////////////////////////////////////////////////////////////////////
-  // locals
-
-  Type getLocalType(unsigned id) const;
-
-  SSATmp* genLdLoc(uint32_t id);
-  SSATmp* genLdLocAddr(uint32_t id);
-
-  /*
-   * Returns an SSATmp containing the (inner) value of the given local.
-   * If the local is a boxed value, this returns its inner value.
-   *
-   * Note: For boxed values, this will generate a LdRef instruction which
-   *       takes the given exit trace in case the inner type doesn't match
-   *       the tracked type for this local.  This check may be optimized away
-   *       if we can determine that the inner type must match the tracked type.
-   */
-  SSATmp* genLdLocAsCell(uint32_t id, Trace* exitTrace);
-
-  SSATmp* genStLoc(uint32_t id,
-                   SSATmp* src,
-                   bool doRefCount,
-                   bool genStoreType,
-                   Trace* exit);
-
-  SSATmp* genBoxLoc(uint32_t id);
-  void    genBindLoc(uint32_t id, SSATmp* ref, bool doRefCount = true);
-
-  void    genDecRefLoc(int id);
-
   //////////////////////////////////////////////////////////////////////
   // constants
 
@@ -368,6 +339,10 @@ private:
   SSATmp*   preOptimizeLdCtx(IRInstruction*);
   SSATmp*   preOptimizeDecRef(IRInstruction*);
   SSATmp*   preOptimizeDecRefThis(IRInstruction*);
+  SSATmp*   preOptimizeDecRefLoc(IRInstruction*);
+  SSATmp*   preOptimizeLdLoc(IRInstruction*);
+  SSATmp*   preOptimizeLdLocAddr(IRInstruction*);
+  SSATmp*   preOptimizeStLoc(IRInstruction*);
 
   SSATmp*   preOptimize(IRInstruction* inst);
   SSATmp*   optimizeWork(IRInstruction* inst);
@@ -386,6 +361,7 @@ private:
   void      killLocals();
   void      killLocalValue(uint32_t id);
   void      setLocalType(uint32_t id, Type type);
+  Type      getLocalType(unsigned id) const;
   SSATmp*   getLocalValue(unsigned id) const;
   bool      isValueAvailable(SSATmp*) const;
   bool      anyLocalHasValue(SSATmp*) const;

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

@@ -515,7 +515,7 @@ SSATmp* HhbcTranslator::VectorTranslator::getInput(unsigned i) {
     }
 
     case Location::Local:
-      return m_tb.genLdLoc(l.offset);
+      return m_ht.ldLoc(l.offset);
 
     case Location::Litstr:
       return cns(m_ht.lookupStringId(l.offset));
@@ -544,8 +544,12 @@ void HhbcTranslator::VectorTranslator::emitBaseLCR() {
         gen(RaiseUninitLoc, LocalId(base.location.offset));
       }
       if (mia & MIA_define) {
-        m_tb.genStLoc(base.location.offset, m_tb.genDefInitNull(),
-                      false, true, nullptr);
+        gen(
+          StLoc,
+          LocalId(base.location.offset),
+          m_tb.getFp(),
+          m_tb.genDefInitNull()
+        );
         baseType = Type::InitNull;
       }
     }
@@ -575,12 +579,12 @@ void HhbcTranslator::VectorTranslator::emitBaseLCR() {
     }
 
     if (base.location.space == Location::Local) {
-      m_base = m_tb.genLdLocAddr(base.location.offset);
+      m_base = m_ht.ldLocAddr(base.location.offset);
     } else {
       assert(base.location.space == Location::Stack);
       m_ht.spillStack();
       assert(m_stackInputs.count(m_iInd));
-      m_base = m_ht.loadStackAddr(m_stackInputs[m_iInd]);
+      m_base = m_ht.ldStackAddr(m_stackInputs[m_iInd]);
     }
     assert(m_base->type().isPtr());
   }
@@ -1711,8 +1715,8 @@ static inline ArrayData* checkedSet(ArrayData* a, int64_t key,
 
 template<KeyType keyType, bool checkForInt, bool setRef>
 static inline typename ShuffleReturn<setRef>::return_type arraySetImpl(
-  ArrayData* a, typename KeyTypeTraits<keyType>::rawType key,
-  CVarRef value, RefData* ref) {
+    ArrayData* a, typename KeyTypeTraits<keyType>::rawType key,
+    CVarRef value, RefData* ref) {
   static_assert(keyType != AnyKey, "AnyKey is not supported in arraySetMImpl");
   const bool copy = a->getCount() > 1;
   ArrayData* ret = checkForInt ? checkedSet(a, key, value, copy)
@@ -1772,7 +1776,9 @@ void HhbcTranslator::VectorTranslator::emitArraySet(SSATmp* key,
 
     // Update the base's value with the new array
     if (base.location.space == Location::Local) {
-      m_tb.genStLoc(base.location.offset, newArr, false, false, nullptr);
+      // We know it's not boxed (setRef above handles that), and
+      // newArr has already been incref'd in the helper.
+      gen(StLoc, LocalId(base.location.offset), m_tb.getFp(), newArr);
     } else if (base.location.space == Location::Stack) {
       VectorEffects ve(newArr->inst());
       assert(ve.baseValChanged);