Remove genDecRef from TraceBuilder

Replace with a preOptimizeDecRef.  Also caught a pseudo-issue
where we relied on the isRefCounted short-circuit for correctness (we
called genDecRef on Type::Cls).  That short-circuit is duplicated in
simplifier, and is stateless, so let's leave it there.  Remove some
dead code about BoxedCells and replace it with a comment in
ir.specification based on a conversation with @bsimmers.

(This is on top of all the other related diffs.)

hphp/doc/ir.specification

@@ -1082,6 +1082,12 @@ DecRef S0:Gen
   Decrease the reference count of S0 by one, and call a destructor for
   types that require it if it goes to zero.
 
+  Note that although DecRef takes a Gen, we don't allow it to use
+  information about the inner types of a BoxedCell.  This is because
+  we don't guard on the inner types of a BoxedCell except when doing
+  LdRef.  For any S0 that is a strict subtype of BoxedCell, the DecRef
+  must just decref it as if it were a BoxedCell.
+
 DecRefMem S0:PtrToGen S1:ConstInt
 
   Decref the value pointed to by S0 at offset S1 (in bytes), calling

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

@@ -121,7 +121,7 @@ void HhbcTranslator::discard(unsigned n) {
 // type is the type expected on the stack.
 void HhbcTranslator::popDecRef(Type type) {
   if (SSATmp* src = m_evalStack.pop()) {
-    m_tb->genDecRef(src);
+    gen(DecRef, src);
     return;
   }
 
@@ -349,7 +349,7 @@ void HhbcTranslator::emitUnboxRAux() {
     push(unboxed);
   } else {
     pushIncRef(unboxed);
-    m_tb->genDecRef(srcBox);
+    gen(DecRef, srcBox);
   }
 }
 
@@ -1065,7 +1065,7 @@ void HhbcTranslator::emitStrlen() {
       push(cns(input->getValStr()->size()));
     } else {
       push(gen(LdRaw, Type::Int, input, cns(RawMemSlot::StrLen)));
-      m_tb->genDecRef(input);
+      gen(DecRef, input);
     }
   } else if (inType.isNull()) {
     popC();
@@ -1114,7 +1114,7 @@ SSATmp* HhbcTranslator::emitLdClsPropAddrOrExit(const StringData* propName,
                            clsTmp,
                            prop,
                            cns(getCurClass()));
-  m_tb->genDecRef(prop); // safe to do early because prop is a string
+  gen(DecRef, prop); // safe to do early because prop is a string
   return addr;
 }
 
@@ -1143,7 +1143,7 @@ SSATmp* HhbcTranslator::emitLdGblAddr(const StringData* gblName, Block* block) {
   // Note: Once we use control flow to implement IssetG/EmptyG, we can
   // use a LdGblAddr helper that decrefs name for us
   SSATmp* addr = gen(LdGblAddr, block, name);
-  m_tb->genDecRef(name);
+  gen(DecRef, name);
   return addr;
 }
 
@@ -1217,7 +1217,7 @@ void HhbcTranslator::emitEmptyS(const StringData* propName) {
 void HhbcTranslator::emitIsTypeC(Type t) {
   SSATmp* src = popC();
   push(gen(IsType, t, src));
-  m_tb->genDecRef(src);
+  gen(DecRef, src);
 }
 
 void HhbcTranslator::emitIsTypeL(Type t, int id) {
@@ -1280,7 +1280,7 @@ SSATmp* HhbcTranslator::emitJmpCondHelper(int32_t offset,
     target = getExitTrace(offset);
   }
   SSATmp* boolSrc = m_tb->genConvToBool(src);
-  m_tb->genDecRef(src);
+  gen(DecRef, src);
   return gen(negate ? JmpZero : JmpNZero, target, boolSrc);
 }
 
@@ -1302,8 +1302,8 @@ void HhbcTranslator::emitCmp(Opcode opc) {
   SSATmp* src1 = popC();
   SSATmp* src2 = popC();
   push(m_tb->genCmp(opc, src2, src1));
-  m_tb->genDecRef(src2);
-  m_tb->genDecRef(src1);
+  gen(DecRef, src2);
+  gen(DecRef, src1);
 }
 
 void HhbcTranslator::emitClsCnsD(int32_t cnsNameStrId, int32_t clsNameStrId) {
@@ -1354,8 +1354,8 @@ void HhbcTranslator::emitAKExists() {
   }
 
   push(gen(AKExists, arr, key));
-  m_tb->genDecRef(arr);
-  m_tb->genDecRef(key);
+  gen(DecRef, arr);
+  gen(DecRef, key);
 }
 
 void HhbcTranslator::emitFPassR() {
@@ -1369,7 +1369,7 @@ void HhbcTranslator::emitFPassV() {
   Block* exit = getExitTrace()->front();
   SSATmp* tmp = popV();
   pushIncRef(gen(Unbox, exit, tmp));
-  m_tb->genDecRef(tmp);
+  gen(DecRef, tmp);
 }
 
 void HhbcTranslator::emitFPushCufOp(VM::Op op, Class* cls, StringData* invName,
@@ -1617,7 +1617,7 @@ void HhbcTranslator::emitFPushObjMethodD(int32_t numParams,
             LdClsMethod, clsTmp, cns(func->methodSlot())
           );
           if (res == MethodLookup::MethodFoundNoThis) {
-            m_tb->genDecRef(obj);
+            gen(DecRef, obj);
             objOrCls = clsTmp;
           }
           emitFPushActRec(funcTmp, objOrCls, numParams,
@@ -1638,7 +1638,7 @@ void HhbcTranslator::emitFPushObjMethodD(int32_t numParams,
       // and decref the obj that was pushed as the this pointer since
       // the obj won't be in the actrec and thus MethodCache::lookup won't
       // decref it
-      m_tb->genDecRef(obj);
+      gen(DecRef, obj);
       objOrCls = cns(baseClass);
     }
     emitFPushActRec(cns(func),
@@ -1838,7 +1838,7 @@ void HhbcTranslator::emitFCallBuiltin(uint32_t numArgs,
   for (int i = 0; i < numArgs; i++) {
     SSATmp* arg = popR();
     if (i >= numArgs - numNonDefault) {
-      m_tb->genDecRef(arg);
+      gen(DecRef, arg);
     }
   }
 
@@ -1894,7 +1894,7 @@ SSATmp* HhbcTranslator::emitDecRefLocalsInline(SSATmp* retVal) {
 
   if (mayHaveThis(getCurFunc())) {
     if (retValSrcLoc && retValSrcOpc == LdThis) {
-      m_tb->genDecRef(retVal);
+      gen(DecRef, retVal);
     } else {
       m_tb->genDecRefThis();
     }
@@ -1910,7 +1910,7 @@ SSATmp* HhbcTranslator::emitDecRefLocalsInline(SSATmp* retVal) {
     retValSrcLoc->inst()->getExtra<LocalId>()->locId : -1;
   for (int id = getCurFunc()->numLocals() - 1; id >= 0; --id) {
     if (retValLocId == id) {
-      m_tb->genDecRef(retVal);
+      gen(DecRef, retVal);
     } else {
       m_tb->genDecRefLoc(id);
     }
@@ -2012,7 +2012,7 @@ void HhbcTranslator::emitSwitch(const ImmVector& iv,
     index = gen(LdSwitchObjIndex,
                       switchVal, ssabase, ssatargets);
   } else if (type.subtypeOf(Type::Arr)) {
-    m_tb->genDecRef(switchVal);
+    gen(DecRef, switchVal);
     gen(Jmp_, getExitTrace(defaultOff));
     return;
   } else {
@@ -2080,7 +2080,7 @@ void HhbcTranslator::emitSSwitch(const ImmVector& iv) {
                                     : LdSSwitchDestSlow,
                            data,
                            testVal);
-  m_tb->genDecRef(testVal);
+  gen(DecRef, testVal);
   auto const stack = spillStack();
   gen(SyncVMRegs, m_tb->getFp(), stack);
   gen(JmpIndirect, dest);
@@ -2340,7 +2340,7 @@ void HhbcTranslator::emitInstanceOfD(int classNameStrId) {
   }
   if (!src->isA(Type::Obj)) {
     push(cns(false));
-    m_tb->genDecRef(src);
+    gen(DecRef, src);
     return;
   }
 
@@ -2380,7 +2380,7 @@ void HhbcTranslator::emitInstanceOfD(int classNameStrId) {
               checkClass,
               cns(maybeCls && !isNormalClass))
   );
-  m_tb->genDecRef(src);
+  gen(DecRef, src);
 }
 
 void HhbcTranslator::emitCastArray() {
@@ -2422,7 +2422,7 @@ void HhbcTranslator::emitCastArray() {
 void HhbcTranslator::emitCastBool() {
   SSATmp* src = popC();
   push(m_tb->genConvToBool(src));
-  m_tb->genDecRef(src);
+  gen(DecRef, src);
 }
 
 void HhbcTranslator::emitCastDouble() {
@@ -2434,7 +2434,7 @@ void HhbcTranslator::emitCastDouble() {
     push(cns(0.0));
   } else if (fromType.isArray()) {
     push(gen(ConvArrToDbl, src));
-    m_tb->genDecRef(src);
+    gen(DecRef, src);
   } else if (fromType.isBool()) {
     push(gen(ConvBoolToDbl, src));
   } else if (fromType.isInt()) {
@@ -2459,14 +2459,14 @@ void HhbcTranslator::emitCastInt() {
     push(cns(0));
   } else if (fromType.isArray()) {
     push(gen(ConvArrToInt, src));
-    m_tb->genDecRef(src);
+    gen(DecRef, src);
   } else if (fromType.isBool()) {
     push(gen(ConvBoolToInt, src));
   } else if (fromType.isDbl()) {
     push(gen(ConvDblToInt, src));
   } else if (fromType.isString()) {
     push(gen(ConvStrToInt, src));
-    m_tb->genDecRef(src);
+    gen(DecRef, src);
   } else if (fromType.isObj()) {
     exceptionBarrier();
     push(gen(ConvObjToInt, src));
@@ -2495,7 +2495,7 @@ void HhbcTranslator::emitCastString() {
     push(cns(StringData::GetStaticString("")));
   } else if (fromType.isArray()) {
     push(cns(StringData::GetStaticString("Array")));
-    m_tb->genDecRef(src);
+    gen(DecRef, src);
   } else if (fromType.isBool()) {
     push(gen(ConvBoolToStr, src));
   } else if (fromType.isDbl()) {
@@ -2532,7 +2532,7 @@ void HhbcTranslator::emitAGetC(const StringData* clsName) {
   if (isSupportedAGet(topC(), clsName)) {
     SSATmp* src = popC();
     emitAGet(src, clsName);
-    m_tb->genDecRef(src);
+    gen(DecRef, src);
   } else {
     emitInterpOne(Type::Cls, 1);
   }
@@ -2557,7 +2557,7 @@ void HhbcTranslator::emitBindMem(SSATmp* ptr, SSATmp* src) {
     exitBlock->insert(++markerInst, m_irFactory.gen(DecRef, prevValue));
     gen(DecRefNZOrBranch, exitBlock, prevValue);
   } else {
-    m_tb->genDecRef(prevValue);
+    gen(DecRef, prevValue);
   }
 }
 
@@ -2764,7 +2764,7 @@ void HhbcTranslator::emitBinaryArith(Opcode opc) {
 void HhbcTranslator::emitNot() {
   SSATmp* src = popC();
   push(m_tb->genNot(m_tb->genConvToBool(src)));
-  m_tb->genDecRef(src);
+  gen(DecRef, src);
 }
 
 #define BINOP(Opp) \
@@ -2850,8 +2850,8 @@ void HhbcTranslator::emitXor() {
   SSATmp* tr = m_tb->genConvToBool(btr);
   SSATmp* tl = m_tb->genConvToBool(btl);
   push(m_tb->genConvToBool(gen(OpXor, tl, tr)));
-  m_tb->genDecRef(btl);
-  m_tb->genDecRef(btr);
+  gen(DecRef, btl);
+  gen(DecRef, btr);
 }
 
 /**

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

@@ -73,7 +73,7 @@ TraceBuilder::~TraceBuilder() {
 void TraceBuilder::genSetPropCell(SSATmp* base, int64_t offset, SSATmp* value) {
   SSATmp* oldVal = gen(LdProp, Type::Cell, base, cns(offset));
   gen(StProp, base, cns(offset), value);
-  genDecRef(oldVal);
+  gen(DecRef, oldVal);
 }
 
 /**
@@ -99,38 +99,6 @@ bool TraceBuilder::isValueAvailable(SSATmp* tmp) const {
   }
 }
 
-void TraceBuilder::genDecRef(SSATmp* tmp) {
-  if (!isRefCounted(tmp)) {
-    return;
-  }
-
-  Type type = tmp->type();
-  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 and rely on later optimizations to
-    // refine it based on alias analysis.
-    type = Type::BoxedCell;
-  }
-
-  // refcount optimization:
-  // If the decref'ed value is guaranteed to be available after the decref,
-  // generate DecRefNZ instead of DecRef.
-  // We could do more accurate availability analysis. For now, we handle
-  // simple cases:
-  // 1) LdThis is always available.
-  // 2) A value stored in a local is always available.
-  IRInstruction* incRefInst = tmp->inst();
-  if (incRefInst->op() == IncRef) {
-    if (isValueAvailable(incRefInst->getSrc(0))) {
-      gen(DecRefNZ, tmp);
-      return;
-    }
-  }
-
-  gen(DecRef, tmp);
-}
-
 /*
  * Code generation support for side exits.
  * There are 3 types of side exits as defined by the ExitType enum:
@@ -409,7 +377,7 @@ void TraceBuilder::genDecRefLoc(int id) {
     if (setNull) {
       gen(StLoc, LocalId(id), m_fpValue, genDefUninit());
     }
-    genDecRef(val);
+    gen(DecRef, val);
     return;
   }
 
@@ -442,7 +410,7 @@ void TraceBuilder::genBindLoc(uint32_t id,
       // Silent store: local already contains value being stored
       // NewValue needs to be decref'ed
       if (!trackedType.notCounted() && doRefCount) {
-        genDecRef(prevValue);
+        gen(DecRef, prevValue);
       }
       return;
     }
@@ -458,7 +426,7 @@ void TraceBuilder::genBindLoc(uint32_t id,
   }
   gen(genStoreType ? StLoc : StLocNT, LocalId(id), m_fpValue, newValue);
   if (trackedType.maybeCounted() && doRefCount) {
-    genDecRef(prevValue);
+    gen(DecRef, prevValue);
   }
 }
 
@@ -509,7 +477,7 @@ SSATmp* TraceBuilder::genStLoc(uint32_t id,
   SSATmp* retVal = newValue;
   if (doRefCount) {
     retVal = gen(IncRef, newValue);
-    genDecRef(prevValue);
+    gen(DecRef, prevValue);
   }
   return retVal;
 }
@@ -728,7 +696,7 @@ void TraceBuilder::genDecRefStack(Type type, uint32_t stackOff) {
     }
     gen(DecRefStack, type, m_spValue, cns(int64_t(stackOff)));
   } else {
-    genDecRef(tmp);
+    gen(DecRef, tmp);
   }
 }
 
@@ -1204,11 +1172,35 @@ SSATmp* TraceBuilder::preOptimizeLdCtx(IRInstruction* inst) {
   return nullptr;
 }
 
+SSATmp* TraceBuilder::preOptimizeDecRef(IRInstruction* inst) {
+  /*
+   * Refcount optimization:
+   *
+   * If the decref'ed value is guaranteed to be available after the decref,
+   * generate DecRefNZ instead of DecRef.
+   *
+   * This is safe WRT copy-on-write because all the instructions that
+   * could cause a COW return a new SSATmp that will replace the
+   * tracked state that we are using to determine the value is still
+   * available.  I.e. by the time they get to the DecRef we won't see
+   * it in isValueAvailable anymore and won't convert to DecRefNZ.
+   */
+  auto const srcInst = inst->getSrc(0)->inst();
+  if (srcInst->op() == IncRef) {
+    if (isValueAvailable(srcInst->getSrc(0))) {
+      inst->setOpcode(DecRefNZ);
+    }
+  }
+
+  return nullptr;
+}
+
 SSATmp* TraceBuilder::preOptimize(IRInstruction* inst) {
 #define X(op) case op: return preOptimize##op(inst)
   switch (inst->op()) {
     X(LdThis);
     X(LdCtx);
+    X(DecRef);
   default:
     break;
   }
@@ -1235,6 +1227,7 @@ SSATmp* TraceBuilder::optimizeWork(IRInstruction* inst) {
            indent(), preOpt->inst()->toString());
     return preOpt;
   }
+  if (inst->op() == Nop) return nullptr;
 
   // copy propagation on inst source operands
   copyProp(inst);

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

@@ -116,8 +116,6 @@ public:
   SSATmp* genBoxLoc(uint32_t id);
   void    genBindLoc(uint32_t id, SSATmp* ref, bool doRefCount = true);
 
-  void    genCheckClsCnsDefined(SSATmp* cns, Trace* exitTrace);
-  SSATmp* genLdCurFuncPtr();
   void    genGuardLoc(uint32_t id, Type type, Trace* exitTrace);
   void    genAssertStk(uint32_t id, Type type);
   void    genAssertLoc(uint32_t id,
@@ -139,7 +137,6 @@ public:
   SSATmp* genConvToBool(SSATmp* src);
   SSATmp* genCallBuiltin(SSATmp* func, Type type,
                          uint32_t numArgs, SSATmp** args);
-  void    genDecRef(SSATmp* tmp);
   void    genDecRefStack(Type type, uint32_t stackOff);
   void    genDecRefLoc(int id);
   void    genDecRefThis();
@@ -314,6 +311,7 @@ public:
 private:
   SSATmp*   preOptimizeLdThis(IRInstruction*);
   SSATmp*   preOptimizeLdCtx(IRInstruction*);
+  SSATmp*   preOptimizeDecRef(IRInstruction*);
 
   SSATmp*   preOptimize(IRInstruction* inst);
   SSATmp*   optimizeWork(IRInstruction* inst);

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

@@ -1283,7 +1283,7 @@ void HhbcTranslator::VectorTranslator::emitSetProp() {
     // The object owns a reference now
     SSATmp* increffed = gen(IncRef, value);
     gen(StMem, cellPtr, cns(0), value);
-    m_tb.genDecRef(oldVal);
+    gen(DecRef, oldVal);
     m_result = increffed;
     return;
   }
@@ -2031,7 +2031,10 @@ void HhbcTranslator::VectorTranslator::emitMPost() {
     switch (input.location.space) {
     case Location::Stack: {
       ++nStack;
-      m_tb.genDecRef(getInput(i));
+      auto input = getInput(i);
+      if (input->isA(Type::Gen)) {
+        gen(DecRef, input);
+      }
       break;
     }
     case Location::Local: