Remove some easy remaining TraceBuilder::genFoo functions

This diff should have nothing very interesting in it.

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

@@ -975,7 +975,10 @@ 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);
-  m_tb->genSetPropCell(cont, CONTOFF(m_value), popC());
+  auto const newVal = popC();
+  auto const oldValue = gen(LdProp, Type::Cell, cont, cns(CONTOFF(m_value)));
+  gen(StProp, cont, cns(CONTOFF(m_value)), newVal);
+  gen(DecRef, oldValue);
   gen(
     StRaw, cont, cns(RawMemSlot::ContLabel), cns(labelId)
   );
@@ -997,7 +1000,10 @@ void HhbcTranslator::emitContRetC() {
   gen(
     StRaw, cont, cns(RawMemSlot::ContDone), cns(true)
   );
-  m_tb->genSetPropCell(cont, CONTOFF(m_value), popC());
+  auto const newVal = popC();
+  auto const oldVal = gen(LdProp, Type::Cell, cont, cns(CONTOFF(m_value)));
+  gen(StProp, cont, cns(CONTOFF(m_value)), newVal);
+  gen(DecRef, oldVal);
 
   // transfer control
   emitContExitImpl();
@@ -1007,7 +1013,10 @@ void HhbcTranslator::emitContNext() {
   assert(getCurClass());
   SSATmp* cont = gen(LdThis, m_tb->getFp());
   gen(ContPreNext, getExitSlowTrace(), cont);
-  m_tb->genSetPropCell(cont, CONTOFF(m_received), m_tb->genDefInitNull());
+
+  auto const oldVal = gen(LdProp, Type::Cell, cont, cns(CONTOFF(m_received)));
+  gen(StProp, cont, cns(CONTOFF(m_received)), m_tb->genDefInitNull());
+  gen(DecRef, oldVal);
 }
 
 void HhbcTranslator::emitContSendImpl(bool raise) {
@@ -1017,8 +1026,10 @@ void HhbcTranslator::emitContSendImpl(bool raise) {
   gen(ContPreNext, getExitSlowTrace(), cont);
 
   gen(AssertLoc, Type::Cell, LocalId(0), m_tb->getFp());
-  auto const value = gen(IncRef, m_tb->genLdLoc(0));
-  m_tb->genSetPropCell(cont, CONTOFF(m_received), value);
+  auto const newVal = gen(IncRef, m_tb->genLdLoc(0));
+  auto const oldVal = gen(LdProp, Type::Cell, cont, cns(CONTOFF(m_received)));
+  gen(StProp, cont, cns(CONTOFF(m_received)), newVal);
+  gen(DecRef, oldVal);
   if (raise) {
     gen(
       StRaw, cont, cns(RawMemSlot::ContShouldThrow), cns(true)
@@ -1311,7 +1322,7 @@ void HhbcTranslator::emitCmp(Opcode opc) {
   // src2 opc src1
   SSATmp* src1 = popC();
   SSATmp* src2 = popC();
-  push(m_tb->genCmp(opc, src2, src1));
+  push(gen(opc, src2, src1));
   gen(DecRef, src2);
   gen(DecRef, src1);
 }
@@ -1789,7 +1800,8 @@ void HhbcTranslator::emitFCall(uint32_t numParams,
 }
 
 void HhbcTranslator::emitFCallBuiltin(uint32_t numArgs,
-                                      uint32_t numNonDefault, int32_t funcId) {
+                                      uint32_t numNonDefault,
+                                      int32_t funcId) {
   const NamedEntityPair& nep = lookupNamedEntityPairId(funcId);
   const StringData* name = nep.first;
   const Func* callee = Unit::lookupFunc(nep.second, name);
@@ -1798,12 +1810,13 @@ void HhbcTranslator::emitFCallBuiltin(uint32_t numArgs,
 
   // spill args to stack. We need to spill these for two resons:
   // 1. some of the arguments may be passed by reference, for which
-  //    case we will generate LdStackAddr() (see below).
+  //    case we will pass a stack address.
   // 2. type conversions of the arguments (using tvCast* helpers)
   //    may throw an exception, so we need to have the VM stack
   //    in a clean state at that point.
   exceptionBarrier();
-  // Convert types if needed
+
+  // Convert types if needed.
   for (int i = 0; i < numNonDefault; i++) {
     const Func::ParamInfo& pi = callee->params()[i];
     switch (pi.builtinType()) {
@@ -1821,38 +1834,41 @@ void HhbcTranslator::emitFCallBuiltin(uint32_t numArgs,
     }
   }
 
-  // pass arguments for call
+  // Pass arguments for CallBuiltin.
   SSATmp* args[numArgs + 1];
-
+  args[0] = cns(callee);
   for (int i = numArgs - 1; i >= 0; i--) {
     const Func::ParamInfo& pi = callee->params()[i];
     switch (pi.builtinType()) {
       case KindOfBoolean:
       case KindOfInt64:
-        args[i] = top(Type::fromDataType(pi.builtinType(), KindOfInvalid),
-                      numArgs - i - 1);
+        args[i + 1] = top(Type::fromDataType(pi.builtinType(), KindOfInvalid),
+                          numArgs - i - 1);
         break;
       case KindOfDouble: assert(false);
       default:
-        args[i] = loadStackAddr(numArgs - i - 1);
+        args[i + 1] = loadStackAddr(numArgs - i - 1);
         break;
     }
   }
-  // generate call and set return type
-  SSATmp* func = cns(callee);
-  Type type = Type::fromDataTypeWithRef(callee->returnType(),
-                       (callee->attrs() & ClassInfo::IsReference));
-  SSATmp* ret = m_tb->genCallBuiltin(func, type, numArgs, args);
 
-  // decref and free args
+  // Generate call and set return type
+  SSATmp** decayedPtr = args;
+  auto const ret = gen(
+    CallBuiltin,
+    Type::fromDataTypeWithRef(callee->returnType(),
+                              (callee->attrs() & ClassInfo::IsReference)),
+    std::make_pair(numArgs + 1, decayedPtr)
+  );
+
+  // Decref and free args
   for (int i = 0; i < numArgs; i++) {
-    SSATmp* arg = popR();
+    auto const arg = popR();
     if (i >= numArgs - numNonDefault) {
       gen(DecRef, arg);
     }
   }
 
-  // push return value
   push(ret);
 }
 
@@ -1883,16 +1899,17 @@ void HhbcTranslator::emitRetFromInlined(Type type) {
   emitMarker();
 }
 
-/*
- * In case retVal comes from a local, the logic below tweaks the code
- * so that retVal is DecRef'd and the corresponding local's SSATmp is
- * returned. This enables the ref-count optimization to eliminate the
- * IncRef/DecRef pair in the main trace.
- */
 SSATmp* HhbcTranslator::emitDecRefLocalsInline(SSATmp* retVal) {
   SSATmp* retValSrcLoc = nullptr;
   Opcode  retValSrcOpc = Nop; // Nop flags the ref-count opt is impossible
   IRInstruction* retValSrcInstr = retVal->inst();
+
+  /*
+   * In case retVal comes from a local, the logic below tweaks the code
+   * so that retVal is DecRef'd and the corresponding local's SSATmp is
+   * returned. This enables the ref-count optimization to eliminate the
+   * IncRef/DecRef pair in the main trace.
+   */
   if (retValSrcInstr->op() == IncRef) {
     retValSrcLoc = retValSrcInstr->getSrc(0);
     retValSrcOpc = retValSrcLoc->inst()->op();
@@ -1904,9 +1921,14 @@ SSATmp* HhbcTranslator::emitDecRefLocalsInline(SSATmp* retVal) {
 
   if (mayHaveThis(getCurFunc())) {
     if (retValSrcLoc && retValSrcOpc == LdThis) {
+      // Note that this doesn't need to be DecRefThis or
+      // DecRefKillThis because we're carefully setting things up to
+      // get turned to DecRefNZ.  This means even if a
+      // debug_backtrace() occurs it can't see a stale $this on the
+      // ActRec.
       gen(DecRef, retVal);
     } else {
-      m_tb->genDecRefThis();
+      gen(DecRefThis, m_tb->getFp());
     }
   }
 
@@ -1953,7 +1975,7 @@ void HhbcTranslator::emitRet(Type type, bool freeInline) {
     sp = gen(RetAdjustStack, m_tb->getFp());
   } else {
     if (mayHaveThis(curFunc)) {
-      m_tb->genDecRefThis();
+      gen(DecRefThis, m_tb->getFp());
     }
     sp = gen(
       GenericRetDecRefs, m_tb->getFp(), retVal, cns(curFunc->numLocals())

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

@@ -631,7 +631,6 @@ private:
 
     Kind      getKind()  const { return m_kind;  }
     uint32_t  getIndex() const { return m_index; }
-    Type      getType()  const { return m_type;  }
 
   private:
     Kind      m_kind;

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

@@ -453,9 +453,7 @@ SSATmp* Simplifier::simplifyNot(SSATmp* src) {
       //
       // TODO(#2058865): This would make more sense with a real Not
       // instruction and allowing boolean output types for query ops.
-      return m_tb->genCmp(negateQueryOp(op),
-                          inst->getSrc(0),
-                          inst->getSrc(1));
+      return gen(negateQueryOp(op), inst->getSrc(0), inst->getSrc(1));
     case InstanceOf:
     case NInstanceOf:
     case InstanceOfBitmask:
@@ -854,9 +852,9 @@ SSATmp* Simplifier::simplifyCmp(Opcode opName, SSATmp* src1, SSATmp* src2) {
     }
     // Type is neither a string nor an object - simplify to OpEq/OpNeq
     if (opName == OpSame) {
-      return m_tb->genCmp(OpEq, src1, src2);
+      return gen(OpEq, src1, src2);
     }
-    return m_tb->genCmp(OpNeq, src1, src2);
+    return gen(OpNeq, src1, src2);
   }
 
   // ---------------------------------------------------------------------
@@ -916,9 +914,9 @@ SSATmp* Simplifier::simplifyCmp(Opcode opName, SSATmp* src1, SSATmp* src2) {
     // E.g. `some-int > false` is equivalent to `some-int == true`
     if (opName != OpEq) {
       if (cmpOp(opName, false, b)) {
-        return m_tb->genCmp(OpEq, src1, cns(false));
+        return gen(OpEq, src1, cns(false));
       } else {
-        return m_tb->genCmp(OpEq, src1, cns(true));
+        return gen(OpEq, src1, cns(true));
       }
     }
   }
@@ -931,7 +929,7 @@ SSATmp* Simplifier::simplifyCmp(Opcode opName, SSATmp* src1, SSATmp* src2) {
   if (src1->type() == src2->type() ||
       (src1->type().isString() && src2->type().isString())) {
     if (src1->isConst() && !src2->isConst()) {
-      return m_tb->genCmp(commuteQueryOp(opName), src2, src1);
+      return gen(commuteQueryOp(opName), src2, src1);
     }
     return nullptr;
   }
@@ -943,33 +941,32 @@ SSATmp* Simplifier::simplifyCmp(Opcode opName, SSATmp* src1, SSATmp* src2) {
 
   // nulls get canonicalized to the right
   if (src1->type().isNull()) {
-    return m_tb->genCmp(commuteQueryOp(opName), src2, src1);
+    return gen(commuteQueryOp(opName), src2, src1);
   }
 
   // case 1: null cmp string. Convert null to ""
   if (src1->type().isString() && src2->type().isNull()) {
-    return m_tb->genCmp(opName, src1,
-                        cns(StringData::GetStaticString("")));
+    return gen(opName, src1, cns(StringData::GetStaticString("")));
   }
 
   // case 2a: null cmp anything. Convert null to false
   if (src2->type().isNull()) {
-    return m_tb->genCmp(opName, src1, cns(false));
+    return gen(opName, src1, cns(false));
   }
 
   // bools get canonicalized to the right
   if (src1->type() == Type::Bool) {
-    return m_tb->genCmp(commuteQueryOp(opName), src2, src1);
+    return gen(commuteQueryOp(opName), src2, src1);
   }
 
   // case 2b: bool cmp anything. Convert anything to bool
   if (src2->type() == Type::Bool) {
     if (src1->isConst()) {
       if (src1->type() == Type::Int) {
-        return m_tb->genCmp(opName, cns(bool(src1->getValInt())), src2);
+        return gen(opName, cns(bool(src1->getValInt())), src2);
       } else if (src1->type().isString()) {
         auto str = src1->getValStr();
-        return m_tb->genCmp(opName, cns(str->toBoolean()), src2);
+        return gen(opName, cns(str->toBoolean()), src2);
       }
     }
 
@@ -979,14 +976,14 @@ SSATmp* Simplifier::simplifyCmp(Opcode opName, SSATmp* src1, SSATmp* src2) {
       always_assert(opName == OpEq);
 
       if (src2->getValBool()) {
-        return m_tb->genCmp(OpNeq, src1, cns(0));
+        return gen(OpNeq, src1, cns(0));
       } else {
-        return m_tb->genCmp(OpEq, src1, cns(0));
+        return gen(OpEq, src1, cns(0));
       }
     }
 
     // Nothing fancy to do - perform juggling as normal.
-    return m_tb->genCmp(opName, m_tb->genConvToBool(src1), src2);
+    return gen(opName, m_tb->genConvToBool(src1), src2);
   }
 
   // From here on, we must be careful of how Type::Obj gets dealt with,
@@ -997,12 +994,12 @@ SSATmp* Simplifier::simplifyCmp(Opcode opName, SSATmp* src1, SSATmp* src2) {
 
   // strings get canonicalized to the left
   if (src2->type().isString()) {
-    return m_tb->genCmp(commuteQueryOp(opName), src2, src1);
+    return gen(commuteQueryOp(opName), src2, src1);
   }
 
   // ints get canonicalized to the right
   if (src1->type() == Type::Int) {
-    return m_tb->genCmp(commuteQueryOp(opName), src2, src1);
+    return gen(commuteQueryOp(opName), src2, src1);
   }
 
   // case 4: number/string/resource cmp. Convert to number (int OR double)
@@ -1016,12 +1013,12 @@ SSATmp* Simplifier::simplifyCmp(Opcode opName, SSATmp* src1, SSATmp* src2) {
     int64_t si; double sd;
     auto st = str->isNumericWithVal(si, sd, true /* allow errors */);
     if (st == KindOfDouble) {
-      return m_tb->genCmp(opName, cns(sd), src2);
+      return gen(opName, cns(sd), src2);
     }
     if (st == KindOfNull) {
       si = 0;
     }
-    return m_tb->genCmp(opName, cns(si), src2);
+    return gen(opName, cns(si), src2);
   }
 
   // case 5: array cmp array. No juggling to do

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

@@ -70,12 +70,6 @@ TraceBuilder::~TraceBuilder() {
   for (State* state : m_snapshots) delete state;
 }
 
-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);
-  gen(DecRef, oldVal);
-}
-
 /**
  * Checks if the given SSATmp, or any of its aliases, is available in
  * any VM location, including locals and the This pointer.
@@ -259,10 +253,6 @@ SSATmp* TraceBuilder::genConvToBool(SSATmp* src) {
   }
 }
 
-SSATmp* TraceBuilder::genCmp(Opcode opc, SSATmp* src1, SSATmp* src2) {
-  return gen(opc, src1, src2);
-}
-
 SSATmp* TraceBuilder::genBoxLoc(uint32_t id) {
   SSATmp* prevValue  = genLdLoc(id);
   Type prevType = prevValue->type();
@@ -637,17 +627,6 @@ SSATmp* TraceBuilder::genLdStackAddr(SSATmp* sp, int64_t index) {
   return gen(LdStackAddr, type.ptr(), sp, cns(index));
 }
 
-SSATmp* TraceBuilder::genCallBuiltin(SSATmp* func,
-                                     Type type,
-                                     uint32_t numArgs,
-                                     SSATmp** args) {
-  SSATmp* srcs[numArgs + 1];
-  srcs[0] = func;
-  std::copy(args, args + numArgs, srcs + 1);
-  SSATmp** decayedPtr = srcs;
-  return gen(CallBuiltin, type, std::make_pair(numArgs + 1, decayedPtr));
-}
-
 void TraceBuilder::genDecRefStack(Type type, uint32_t stackOff) {
   bool spansCall = false;
   Type knownType = Type::None;
@@ -665,26 +644,6 @@ void TraceBuilder::genDecRefStack(Type type, uint32_t stackOff) {
   }
 }
 
-void TraceBuilder::genDecRefThis() {
-  if (isThisAvailable()) {
-    auto const thiss = gen(LdThis, m_fpValue);
-    auto const thisInst = thiss->inst();
-
-    if (thisInst->op() == IncRef &&
-        callerLocalHasValue(thisInst->getSrc(0))) {
-      gen(DecRefNZ, thiss);
-      return;
-    }
-
-    // It's a shame to keep a reference to the frame just to kill the
-    // this pointer.  This is handled in optimizeActRecs.
-    gen(DecRefKillThis, thiss, m_fpValue);
-    return;
-  }
-
-  gen(DecRefThis, m_fpValue);
-}
-
 SSATmp* TraceBuilder::genSpillStack(uint32_t stackAdjustment,
                                     uint32_t numOpnds,
                                     SSATmp** spillOpnds) {
@@ -1200,6 +1159,43 @@ SSATmp* TraceBuilder::preOptimizeDecRef(IRInstruction* inst) {
   return nullptr;
 }
 
+SSATmp* TraceBuilder::preOptimizeDecRefThis(IRInstruction* inst) {
+  /*
+   * If $this is available, convert to an instruction sequence that
+   * doesn't need to test if it's already live.
+   */
+  if (isThisAvailable()) {
+    auto const thiss = gen(LdThis, m_fpValue);
+    auto const thisInst = thiss->inst();
+
+    /*
+     * DecRef optimization for $this in an inlined frame: if a caller
+     * local contains the $this, we know it can't go to zero and can
+     * switch DecRef to DecRefNZ.
+     *
+     * It's ok not to do DecRefThis (which normally nulls out the ActRec
+     * $this), because there is still a reference to it in the caller
+     * frame, so debug_backtrace() can't see a non-live pointer value.
+     */
+    if (thisInst->op() == IncRef &&
+        callerLocalHasValue(thisInst->getSrc(0))) {
+      gen(DecRefNZ, thiss);
+      inst->convertToNop();
+      return nullptr;
+    }
+
+    // If we're in an inlined callee, it's a shame to keep a reference
+    // to the frame just to kill the $this pointer.  But this is
+    // handled in optimizeActRecs.
+    assert(inst->getSrc(0) == m_fpValue);
+    gen(DecRefKillThis, thiss, m_fpValue);
+    inst->convertToNop();
+    return nullptr;
+  }
+
+  return nullptr;
+}
+
 SSATmp* TraceBuilder::preOptimize(IRInstruction* inst) {
 #define X(op) case op: return preOptimize##op(inst)
   switch (inst->op()) {
@@ -1208,6 +1204,7 @@ SSATmp* TraceBuilder::preOptimize(IRInstruction* inst) {
     X(LdThis);
     X(LdCtx);
     X(DecRef);
+    X(DecRefThis);
   default:
     break;
   }

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

@@ -230,17 +230,11 @@ struct TraceBuilder {
   //////////////////////////////////////////////////////////////////////
   // dubious
 
-  void    genSetPropCell(SSATmp* base, int64_t offset, SSATmp* value);
-
   // TODO(#2058865): we should have a real not opcode
   SSATmp* genNot(SSATmp* src);
 
-  SSATmp* genCmp(Opcode opc, SSATmp* src1, SSATmp* src2);
   SSATmp* genCastStk(uint32_t id, Type type);
   SSATmp* genConvToBool(SSATmp* src);
-  SSATmp* genCallBuiltin(SSATmp* func, Type type,
-                         uint32_t numArgs, SSATmp** args);
-  void    genDecRefThis();
 
   //////////////////////////////////////////////////////////////////////
   // control flow
@@ -397,6 +391,7 @@ private:
   SSATmp*   preOptimizeLdThis(IRInstruction*);
   SSATmp*   preOptimizeLdCtx(IRInstruction*);
   SSATmp*   preOptimizeDecRef(IRInstruction*);
+  SSATmp*   preOptimizeDecRefThis(IRInstruction*);
 
   SSATmp*   preOptimize(IRInstruction* inst);
   SSATmp*   optimizeWork(IRInstruction* inst);