Allow temporary ExtraData objects in instruction generation

Brett said the inability to pass temporaries here made him
not a huge fan of extra data, so this adds that.  Also simplifies the
int,SSATmp** case a bit by packaging them into a pair (this makes it a
bit less subtle to understand why go2 terminates, which was going to
get way more complicated after Tail becomes Tail&& if I wanted to keep
it that way, so I didn't).

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

@@ -168,7 +168,7 @@ void HhbcTranslator::setBcOff(Offset newOff, bool lastBcOff) {
     marker.func      = getCurFunc();
     marker.stackOff  = m_tb->getSpOffset() +
                          m_evalStack.numCells() - m_stackDeficit;
-    m_tb->gen(Marker, &marker);
+    m_tb->gen(Marker, marker);
   }
   m_lastBcOff = lastBcOff;
 }
@@ -641,7 +641,6 @@ void HhbcTranslator::emitTraitExists(const StringData* traitName) {
 
 void HhbcTranslator::emitStaticLocInit(uint32_t locId, uint32_t litStrId) {
   const StringData* name = lookupStringId(litStrId);
-  LocalId id(locId);
   SSATmp* value = popC();
   SSATmp* box;
 
@@ -668,7 +667,7 @@ void HhbcTranslator::emitStaticLocInit(uint32_t locId, uint32_t litStrId) {
       }
     );
   }
-  m_tb->gen(StLoc, &id, m_tb->getFp(), box);
+  m_tb->gen(StLoc, LocalId(locId), m_tb->getFp(), box);
   m_tb->gen(DecRef, value);
 }
 
@@ -1824,7 +1823,7 @@ void HhbcTranslator::emitSwitch(const ImmVector& iv,
   SSATmp* stack = spillStack();
   m_tb->gen(SyncVMRegs, m_tb->getFp(), stack);
 
-  m_tb->gen(JmpSwitchDest, &data, index);
+  m_tb->gen(JmpSwitchDest, data, index);
   m_hasExit = true;
 }
 
@@ -1868,7 +1867,7 @@ void HhbcTranslator::emitSSwitch(const ImmVector& iv) {
 
   SSATmp* dest = m_tb->gen(fastPath ? LdSSwitchDestFast
                                     : LdSSwitchDestSlow,
-                           &data,
+                           data,
                            testVal);
   m_tb->genDecRef(testVal);
   SSATmp* stack = spillStack();

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

@@ -52,6 +52,8 @@ namespace JIT {
 
 using namespace HPHP::VM::Transl;
 
+struct IRInstruction;
+
 class FailedIRGen : public std::exception {
  public:
   const char* file;
@@ -725,8 +727,8 @@ private:
  * to the list head.
  */
 struct EdgeData : IRExtraData {
-  struct IRInstruction* jmp;  // owner of this edge
-  EdgeData* next;             // next edge to same target
+  IRInstruction* jmp;    // owner of this edge
+  EdgeData* next;        // next edge to same target
 };
 
 /*
@@ -1654,6 +1656,13 @@ struct IRInstruction {
    */
   void setExtra(IRExtraData* data) { assert(!m_extra); m_extra = data; }
 
+  /*
+   * Clear the extra data pointer in a IRInstruction.  Used during
+   * IRFactory::gen to avoid having dangling IRExtraData*'s into stack
+   * memory.
+   */
+  void clearExtra() { m_extra = nullptr; }
+
   /*
    * Replace an instruction in place with a Nop.  This sometimes may
    * be a result of a simplification pass.

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

@@ -18,13 +18,13 @@
 #define incl_HPHP_VM_IRFACTORY_H_
 
 #include <type_traits>
+#include <vector>
 
+#include "folly/ScopeGuard.h"
 #include "util/arena.h"
 #include "runtime/vm/translator/hopt/ir.h"
 #include "runtime/vm/translator/hopt/cse.h"
 
-#include <vector>
-
 namespace HPHP { namespace VM { namespace JIT {
 
 //////////////////////////////////////////////////////////////////////
@@ -37,7 +37,7 @@ namespace HPHP { namespace VM { namespace JIT {
  *
  *   Normally IRInstruction creation should go through either
  *   IRFactory::gen or TraceBuilder::gen.  This utility is used to
- *   implement those.
+ *   implement those.  The lambda must not escape the IRInstruction*.
  */
 
 namespace detail {
@@ -46,12 +46,26 @@ template<class Ret, class Func>
 struct InstructionBuilder {
   explicit InstructionBuilder(const Func& func) : func(func) {}
 
-  // Create an IRInstruction, and then recursively chew on the Args
-  // list to populate its fields.
+  /*
+   * Create an IRInstruction, and then recursively chew on the Args
+   * list to populate its fields.
+   *
+   * The IRInstruction is stack allocated, and should not escape the
+   * lambda, so we fill it with 0xc0 in debug builds after we're done.
+   */
   template<class... Args>
-  Ret go(Opcode op, Args... args) {
-    IRInstruction inst(op);
-    return go2(&inst, args...);
+  Ret go(Opcode op, Args&&... args) {
+    std::aligned_storage<
+      sizeof(IRInstruction)
+    >::type buffer;
+    void* const vpBuffer = &buffer;
+    SCOPE_EXIT { if (debug) memset(&buffer, 0xc0, sizeof buffer); };
+
+    new (vpBuffer) IRInstruction(op);
+    auto const inst = static_cast<IRInstruction*>(vpBuffer);
+
+    SCOPE_EXIT { inst->clearExtra(); };
+    return go2(inst, std::forward<Args>(args)...);
   }
 
 private:
@@ -59,9 +73,9 @@ private:
   // until there's no overload for the Head type; then it will fall
   // through to the base case.
   template<class Head, class... Tail>
-  Ret go2(IRInstruction* inst, Head x, Tail... xs) {
+  Ret go2(IRInstruction* inst, const Head& x, Tail&&... xs) {
     setter(inst, x);
-    return go2(inst, xs...);
+    return go2(inst, std::forward<Tail>(xs)...);
   }
 
   // Base cases: either there are no SSATmps, or there's a variadic
@@ -76,15 +90,13 @@ private:
     return stop(inst);
   }
 
-  template<class Int>
-  Ret go2(IRInstruction* inst, Int num, SSATmp** ssas) {
-    inst->initializeSrcs(num, ssas);
-    return stop(inst);
-  }
-
   // For each of the optional parameters, there's an overloaded
   // setter:
 
+  void setter(IRInstruction* inst, std::pair<uint32_t,SSATmp**> ssas) {
+    inst->initializeSrcs(ssas.first, ssas.second);
+  }
+
   void setter(IRInstruction* inst, Block* target) {
     inst->setTaken(target);
   }
@@ -97,8 +109,21 @@ private:
     inst->setTypeParam(t);
   }
 
-  void setter(IRInstruction* inst, IRExtraData* extra) {
-    inst->setExtra(extra);
+  void setter(IRInstruction* inst, const IRExtraData& extra) {
+    /*
+     * Taking the address of this temporary seems scary, but actually
+     * it is safe: `extra' was forwarded in all the way from the
+     * makeInstruction call, but then we bound a const reference to it
+     * at go2() when it was the head of the varargs list, so it must
+     * last until the end of the full-expression that called that
+     * go2().
+     *
+     * This is long enough for it to outlast our call to func,
+     * although the transient IRInstruction actually will outlive it.
+     * We null out the extra data in go() before ~IRInstruction runs,
+     * though.
+     */
+    inst->setExtra(const_cast<IRExtraData*>(&extra));
   }
 
   // Finally we end up here.
@@ -115,7 +140,7 @@ private:
 
 template<class Func, class... Args>
 typename std::result_of<Func (IRInstruction*)>::type
-makeInstruction(Func func, Args... args) {
+makeInstruction(Func func, Args&&... args) {
   typedef typename std::result_of<Func (IRInstruction*)>::type Ret;
   return detail::InstructionBuilder<Ret,Func>(func).go(args...);
 }
@@ -139,17 +164,17 @@ public:
    *
    * Arguments are passed in the following format:
    *
-   *   gen(Opcode, [IRExtraData*,] [type param,] [exit label,] [srcs ...]);
+   *   gen(Opcode, [IRExtraData&,] [type param,] [exit label,] [srcs ...]);
    *
    * All arguments are optional except the opcode.  `srcs' may be
    * specified either as a list of SSATmp*'s, or as a integer size and
    * a SSATmp**.
    */
   template<class... Args>
-  IRInstruction* gen(Args... args) {
+  IRInstruction* gen(Args&&... args) {
     return makeInstruction(
       [this] (IRInstruction* inst) { return inst->clone(this); },
-      args...
+      std::forward<Args>(args)...
     );
   }
 

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

@@ -505,9 +505,8 @@ void LinearScan::insertAllocFreeSpill(Trace* trace, uint32_t numExtraSpillLocs)
 
 void LinearScan::insertAllocFreeSpillAux(Trace* trace,
                                          uint32_t numExtraSpillLocs) {
-  ConstData numSpillConst(numExtraSpillLocs);
   SSATmp* tmp = m_irFactory->gen(DefConst, Type::Int,
-    &numSpillConst)->getDst();
+    ConstData(numExtraSpillLocs))->getDst();
 
   for (Block* block : trace->getBlocks()) {
     for (auto it = block->begin(); it != block->end(); ) {
@@ -1016,8 +1015,12 @@ void LinearScan::rematerializeAux() {
           // Rematerialize LdLoc.
           int loc = findLocal(spilledTmp);
           if (loc != -1) {
-            LocalId localId(loc);
-            newInst = m_irFactory->gen(LdLoc, dst->getType(), &localId, curFp);
+            newInst = m_irFactory->gen(
+              LdLoc,
+              dst->getType(),
+              LocalId(loc),
+              curFp
+            );
           }
         }
         if (newInst) {

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

@@ -436,9 +436,11 @@ SSATmp* Simplifier::simplifyNot(SSATmp* src) {
     case NInstanceOfBitmask:
       // TODO: combine this with the above check and use isQueryOp or
       // add an isNegatable.
-      return m_tb->gen(negateQueryOp(op),
-                       inst->getNumSrcs(),
-                       inst->getSrcs().begin());
+      return m_tb->gen(
+        negateQueryOp(op),
+        std::make_pair(inst->getNumSrcs(), inst->getSrcs().begin())
+      );
+      return nullptr;
     // TODO !(X | non_zero) --> 0
     default: (void)op;
   }
@@ -1390,14 +1392,15 @@ SSATmp* Simplifier::simplifyCondJmp(IRInstruction* inst) {
   // Fuse jumps with query operators.
   if (isQueryOp(srcOpcode) && !disableBranchFusion(srcOpcode)) {
     SrcRange ssas = srcInst->getSrcs();
-    return m_tb->gen(queryToJmpOp(
-                       inst->getOpcode() == JmpZero
-                         ? negateQueryOp(srcOpcode)
-                         : srcOpcode),
-                     srcInst->getTypeParam(), // if it had a type param
-                     inst->getTaken(),
-                     ssas.size(),
-                     ssas.begin());
+    return m_tb->gen(
+      queryToJmpOp(
+        inst->getOpcode() == JmpZero
+          ? negateQueryOp(srcOpcode)
+          : srcOpcode),
+        srcInst->getTypeParam(), // if it had a type param
+        inst->getTaken(),
+        std::make_pair(ssas.size(), ssas.begin())
+    );
   }
 
   return nullptr;

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

@@ -238,7 +238,7 @@ Trace* TraceBuilder::genExitGuardFailure(uint32_t bcOff) {
   marker.bcOff    = bcOff;
   marker.stackOff = m_spOffset;
   marker.func     = m_curFunc->getValFunc();
-  gen(Marker, &marker); // goes on main trace
+  gen(Marker, marker); // goes on main trace
 
   SSATmp* pc = genDefConst((int64_t)bcOff);
   // TODO change exit trace to a control flow instruction that
@@ -305,7 +305,7 @@ Trace* TraceBuilder::genExitTrace(uint32_t   bcOff,
   marker.bcOff    = bcOff;
   marker.stackOff = m_spOffset + numOpnds - stackDeficit;
   marker.func     = m_curFunc->getValFunc();
-  exitTrace->back()->push_back(m_irFactory.gen(Marker, &marker));
+  exitTrace->back()->push_back(m_irFactory.gen(Marker, marker));
 
   if (beforeExit) {
     beforeExit(&m_irFactory, exitTrace);
@@ -317,7 +317,11 @@ Trace* TraceBuilder::genExitTrace(uint32_t   bcOff,
     srcs[1] = genDefConst<int64_t>(stackDeficit);
     std::copy(opnds, opnds + numOpnds, srcs + 2);
 
-    auto* spillInst = m_irFactory.gen(SpillStack, numOpnds + 2, srcs);
+    SSATmp** decayedPtr = srcs;
+    auto* spillInst = m_irFactory.gen(
+      SpillStack,
+      std::make_pair(numOpnds + 2, decayedPtr)
+    );
     sp = spillInst->getDst();
     exitTrace->back()->push_back(spillInst);
   }
@@ -369,33 +373,27 @@ SSATmp* TraceBuilder::genNot(SSATmp* src) {
 }
 
 SSATmp* TraceBuilder::genDefUninit() {
-  ConstData cdata(0);
-  return gen(DefConst, Type::Uninit, &cdata);
+  return gen(DefConst, Type::Uninit, ConstData(0));
 }
 
 SSATmp* TraceBuilder::genDefInitNull() {
-  ConstData cdata(0);
-  return gen(DefConst, Type::InitNull, &cdata);
+  return gen(DefConst, Type::InitNull, ConstData(0));
 }
 
 SSATmp* TraceBuilder::genDefNull() {
-  ConstData cdata(0);
-  return gen(DefConst, Type::Null, &cdata);
+  return gen(DefConst, Type::Null, ConstData(0));
 }
 
 SSATmp* TraceBuilder::genPtrToInitNull() {
-  ConstData cdata(&null_variant);
-  return gen(DefConst, Type::PtrToUninit, &cdata);
+  return gen(DefConst, Type::PtrToUninit, ConstData(&null_variant));
 }
 
 SSATmp* TraceBuilder::genPtrToUninit() {
-  ConstData cdata(&init_null_variant);
-  return gen(DefConst, Type::PtrToInitNull, &cdata);
+  return gen(DefConst, Type::PtrToInitNull, ConstData(&init_null_variant));
 }
 
 SSATmp* TraceBuilder::genDefNone() {
-  ConstData cdata(0);
-  return gen(DefConst, Type::None, &cdata);
+  return gen(DefConst, Type::None, ConstData(0));
 }
 
 SSATmp* TraceBuilder::genConvToBool(SSATmp* src) {
@@ -436,8 +434,7 @@ SSATmp* TraceBuilder::genJmpCond(SSATmp* boolSrc, Trace* target, bool negate) {
 }
 
 void TraceBuilder::genJmp(Block* target, SSATmp* src) {
-  EdgeData edge;
-  gen(Jmp_, target, &edge, src);
+  gen(Jmp_, target, EdgeData(), src);
 }
 
 void TraceBuilder::genExitWhenSurprised(Trace* targetTrace) {
@@ -460,8 +457,7 @@ void TraceBuilder::genGuardLoc(uint32_t id, Type type, Trace* exitTrace) {
   }
   Type prevType = getLocalType(id);
   if (prevType == Type::None) {
-    LocalId local(id);
-    gen(GuardLoc, type, getFirstBlock(exitTrace), &local, m_fpValue);
+    gen(GuardLoc, type, getFirstBlock(exitTrace), LocalId(id), m_fpValue);
   } else {
     // It doesn't make sense to be guarding on something that's deemed to fail
     assert(prevType == type);
@@ -477,8 +473,7 @@ void TraceBuilder::genAssertLoc(uint32_t id, Type type,
                                 bool overrideType /* =false */) {
   Type prevType = overrideType ? Type::None : getLocalType(id);
   if (prevType == Type::None || type.strictSubtypeOf(prevType)) {
-    LocalId local(id);
-    gen(AssertLoc, type, &local, m_fpValue);
+    gen(AssertLoc, type, LocalId(id), m_fpValue);
   } else {
     assert(prevType == type || prevType.strictSubtypeOf(type));
   }
@@ -586,8 +581,7 @@ SSATmp* TraceBuilder::genLdLoc(uint32_t id) {
   if (type.isNull()) {
     tmp = genDefConst(type);
   } else {
-    LocalId loc(id);
-    tmp = gen(LdLoc, type, &loc, m_fpValue);
+    tmp = gen(LdLoc, type, LocalId(id), m_fpValue);
   }
   return tmp;
 }
@@ -604,14 +598,12 @@ SSATmp* TraceBuilder::genLdLocAsCell(uint32_t id, Trace* exitTrace) {
 }
 
 SSATmp* TraceBuilder::genLdLocAddr(uint32_t id) {
-  LocalId baseLocalId(id);
-  return gen(LdLocAddr, getLocalType(id).ptr(), &baseLocalId, getFp());
+  return gen(LdLocAddr, getLocalType(id).ptr(), LocalId(id), getFp());
 }
 
 void TraceBuilder::genStLocAux(uint32_t id, SSATmp* newValue, bool storeType) {
-  LocalId locId(id);
   gen(storeType ? StLoc : StLocNT,
-      &locId,
+      LocalId(id),
       m_fpValue,
       newValue);
 }
@@ -637,8 +629,7 @@ void TraceBuilder::genDecRefLoc(int id) {
   bool setNull = id < m_curFunc->getValFunc()->numParams();
   SSATmp* val = getLocalValue(id);
   if (val == nullptr && setNull) {
-    LocalId locId(id);
-    val = gen(LdLoc, type, &locId, m_fpValue);
+    val = gen(LdLoc, type, LocalId(id), m_fpValue);
   }
   if (val) {
     if (setNull) {
@@ -655,8 +646,7 @@ void TraceBuilder::genDecRefLoc(int id) {
     type = Type::BoxedCell;
   }
 
-  LocalId local(id);
-  gen(DecRefLoc, type, &local, m_fpValue);
+  gen(DecRefLoc, type, LocalId(id), m_fpValue);
 }
 
 /*
@@ -665,12 +655,11 @@ void TraceBuilder::genDecRefLoc(int id) {
 void TraceBuilder::genBindLoc(uint32_t id,
                               SSATmp* newValue,
                               bool doRefCount /* = true */) {
-  LocalId locId(id);
   Type trackedType = getLocalType(id);
   SSATmp* prevValue = 0;
   if (trackedType == Type::None) {
     if (doRefCount) {
-      prevValue = gen(LdLoc, Type::Gen, &locId, m_fpValue);
+      prevValue = gen(LdLoc, Type::Gen, LocalId(id), m_fpValue);
     }
   } else {
     prevValue = getLocalValue(id);
@@ -684,7 +673,7 @@ void TraceBuilder::genBindLoc(uint32_t id,
       return;
     }
     if (trackedType.maybeCounted() && !prevValue && doRefCount) {
-      prevValue = gen(LdLoc, trackedType, &locId, m_fpValue);
+      prevValue = gen(LdLoc, trackedType, LocalId(id), m_fpValue);
     }
   }
   bool genStoreType = true;
@@ -731,8 +720,7 @@ SSATmp* TraceBuilder::genStLoc(uint32_t id,
   // prevRef is a ref
   if (prevRef == nullptr) {
     // prevRef = ldLoc
-    LocalId locId(id);
-    prevRef = gen(LdLoc, trackedType, &locId, m_fpValue);
+    prevRef = gen(LdLoc, trackedType, LocalId(id), m_fpValue);
   }
   SSATmp* prevValue = nullptr;
   if (doRefCount) {
@@ -1003,7 +991,8 @@ SSATmp* TraceBuilder::genCall(SSATmp* actRec,
   srcs[1] = genDefConst<int64_t>(returnBcOffset);
   srcs[2] = func;
   std::copy(params, params + numParams, srcs + 3);
-  return gen(Call, numParams + 3, srcs);
+  SSATmp** decayedPtr = srcs;
+  return gen(Call, std::make_pair(numParams + 3, decayedPtr));
 }
 
 SSATmp* TraceBuilder::genCallBuiltin(SSATmp* func,
@@ -1013,7 +1002,8 @@ SSATmp* TraceBuilder::genCallBuiltin(SSATmp* func,
   SSATmp* srcs[numArgs + 1];
   srcs[0] = func;
   std::copy(args, args + numArgs, srcs + 1);
-  return gen(CallBuiltin, type, numArgs + 1, srcs);
+  SSATmp** decayedPtr = srcs;
+  return gen(CallBuiltin, type, std::make_pair(numArgs + 1, decayedPtr));
 }
 
 void TraceBuilder::genRetVal(SSATmp* val) {
@@ -1081,7 +1071,8 @@ SSATmp* TraceBuilder::genSpillStack(uint32_t stackAdjustment,
   srcs[0] = m_spValue;
   srcs[1] = genDefConst<int64_t>(stackAdjustment);
   std::copy(spillOpnds, spillOpnds + numOpnds, srcs + 2);
-  return gen(SpillStack, numOpnds + 2, srcs);
+  SSATmp** decayedPtr = srcs;
+  return gen(SpillStack, std::make_pair(numOpnds + 2, decayedPtr));
 }
 
 SSATmp* TraceBuilder::genLdStack(int32_t stackOff, Type type) {

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

@@ -67,10 +67,10 @@ public:
    * IRFactory::gen.
    */
   template<class... Args>
-  SSATmp* gen(Args... args) {
+  SSATmp* gen(Args&&... args) {
     return makeInstruction(
       [this] (IRInstruction* inst) { return optimizeInst(inst); },
-      args...
+      std::forward<Args>(args)...
     );
   }
 
@@ -277,20 +277,17 @@ public:
 
   template<typename T>
   SSATmp* genDefConst(T val) {
-    ConstData cdata(val);
-    return gen(DefConst, typeForConst(val), &cdata);
+    return gen(DefConst, typeForConst(val), ConstData(val));
   }
 
   template<typename T>
   SSATmp* genDefConst(T val, Type type) {
-    ConstData cdata(val);
-    return gen(DefConst, type, &cdata);
+    return gen(DefConst, type, ConstData(val));
   }
 
   template<typename T>
   SSATmp* genLdConst(T val) {
-    ConstData cdata(val);
-    return gen(LdConst, typeForConst(val), &cdata);
+    return gen(LdConst, typeForConst(val), ConstData(val));
   }
 
   Trace* getTrace() const { return m_trace.get(); }
@@ -508,8 +505,7 @@ private:
 
 template<>
 inline SSATmp* TraceBuilder::genDefConst(Type t) {
-  ConstData cdata(0);
-  return gen(DefConst, t, &cdata);
+  return gen(DefConst, t, ConstData(0));
 }
 
 }}}

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

@@ -308,8 +308,7 @@ SSATmp* HhbcTranslator::VectorTranslator::genMisPtr() {
   if (m_needMIS) {
     return m_tb.genLdAddr(m_misBase, kReservedRSPSpillSpace);
   } else {
-    ConstData cdata(nullptr);
-    return m_tb.gen(DefConst, Type::PtrToCell, &cdata);
+    return m_tb.gen(DefConst, Type::PtrToCell, ConstData(nullptr));
   }
 }
 
@@ -559,8 +558,7 @@ void HhbcTranslator::VectorTranslator::emitBaseLCR() {
     if (baseType.subtypeOf(Type::Uninit)) {
       if (mia & MIA_warn) {
         m_ht.spillStack();
-        LocalId data(base.location.offset);
-        m_tb.gen(RaiseUninitLoc, &data);
+        m_tb.gen(RaiseUninitLoc, LocalId(base.location.offset));
       }
       if (mia & MIA_define) {
         m_tb.genStLoc(base.location.offset, m_tb.genDefInitNull(),