Implement specialized array setting in VectorTranslator

This mimics what TranslatorX64 does in translateSetMArray,
but it does it with fewer helpers and (often) fewer instructions in
translated code. I also found a bug in both jits and the interpreter
when dealing with arrays that hold refs to themselves. The new test
case exercises the fix, which involved a bit of refactoring of the
refcounting logic.

Enabling VectorTranslator while punting to tx64 is no longer a
regression so I removed the punt in emit().

hphp/doc/ir.specification

@@ -1168,6 +1168,17 @@ D:Cell = CGetElem S0:ConstTCA S1:PtrToGen S2:Gen S3:PtrToCell
   Get element with key S2 from S1. S3 should point to an MInstrState
   struct.
 
+D:Arr = ArraySet S0:ConstTCA S1:Arr S2:{Int|Str} S3:Cell
+
+  Set element with key S2 in S1 to S3. The dest will be a new Array
+  that should replace S1.
+
+ArraySet S0:ConstTCA S1:Arr S2:{Int|Str} S3:Cell S4:BoxedArr
+
+  Set element with key S2 in S1 to S3. If S4 points to S1 after the set
+  operation, it will be replaced with the new Array resulting from the set
+  operation.
+
 D:Vector = SetElem S0:ConstTCA S1:PtrToGen S2:Gen S3:Cell
 
   Set element with key S2 in S1 to S3.

hphp/runtime/base/array/hphp_array.cpp

@@ -30,6 +30,7 @@
 #include <util/trace.h>
 #include <util/util.h>
 #include <runtime/base/execution_context.h>
+#include <runtime/vm/member_operations.h>
 #include <runtime/vm/stats.h>
 
 // If PEDANTIC is defined, extra checks are performed to ensure correct
@@ -1762,27 +1763,14 @@ ArrayData* nvCheckedSet(ArrayData* a, StringData* key, TypedValue* value,
          a->nvSet(key, value, copy);
 }
 
-ArrayData* nvCheckedSet(ArrayData* a, int64_t key, TypedValue* value, bool copy) {
+ArrayData* nvCheckedSet(ArrayData* a, int64_t key, TypedValue* value,
+                        bool copy) {
   return a->nvSet(key, value, copy);
 }
 
 void setmDecRef(int64_t i) { /* nop */ }
 void setmDecRef(StringData* sd) { decRefStr(sd); }
 
-static inline ArrayData*
-array_mutate_post(Cell *cell, ArrayData* old, ArrayData* retval) {
-  if (nullptr == retval) {
-    return old;
-  }
-  retval->incRefCount();
-  // TODO Task #1970153: It would be great if there were nvSet()
-  // methods that didn't bump up the refcount so that we didn't
-  // have to decrement it here
-  decRefArr(old);
-  if (cell) cell->m_data.parr = retval;
-  return retval;
-}
-
 template<typename Key, bool DecRefValue, bool CheckInt, bool DecRefKey>
 static inline
 ArrayData*
@@ -1794,8 +1782,17 @@ array_setm(TypedValue* cell, ArrayData* ad, Key key, TypedValue* value) {
   retval = CheckInt ? nvCheckedSet(ad, key, value, copy) :
            ad->nvSet(key, value, copy);
   if (DecRefKey) setmDecRef(key);
+
+  // TODO Task #1970153: It would be great if there were nvSet()
+  // methods that didn't bump up the refcount so that we didn't
+  // have to decrement it here
   if (DecRefValue) tvRefcountedDecRef(value);
-  return array_mutate_post(cell, ad, retval);
+  if (cell == nullptr) {
+    return arrayRefShuffle<false>(ad, retval, cell);
+  } else {
+    arrayRefShuffle<true>(ad, retval, cell);
+    return nullptr;
+  }
 }
 
 /**

hphp/runtime/vm/member_operations.h

@@ -524,6 +524,46 @@ static inline void SetElemNumberish(Cell* value) {
   }
 }
 
+/*
+ * arrayRefShuffle is used by SetElemArray and by helpers for translated code
+ * to do the necessary bookkeeping after mutating an array. The helpers return
+ * an ArrayData* if and only if the base array was not in a php reference. If
+ * the base array was in a reference, that reference may no longer refer to an
+ * array after the set operation, so the helpers don't return anything.
+ */
+template<bool setRef> struct ShuffleReturn {};
+template<> struct ShuffleReturn<true> {
+  typedef void return_type;
+  static void do_return(ArrayData* a) {}
+};
+template<> struct ShuffleReturn<false> {
+  typedef ArrayData* return_type;
+  static ArrayData* do_return(ArrayData* a) { return a; }
+};
+
+template<bool setRef> inline
+typename ShuffleReturn<setRef>::return_type
+arrayRefShuffle(ArrayData* oldData, ArrayData* newData, TypedValue* base) {
+  if (newData == nullptr) {
+    return ShuffleReturn<setRef>::do_return(oldData);
+  }
+
+  newData->incRefCount();
+  if (setRef) {
+    if (base->m_type == KindOfArray && base->m_data.parr == oldData) {
+      base->m_data.parr = newData;
+    } else {
+      // The base was in a reference that was overwritten by the set operation,
+      // so we don't want to store the new ArrayData to it. oldData has already
+      // been decrefed and there's nobody left to care about newData, so decref
+      // newData instead of oldData.
+      oldData = newData;
+    }
+  }
+  decRefArr(oldData);
+  return ShuffleReturn<setRef>::do_return(newData);
+}
+
 static inline ArrayData* SetElemArrayRawKey(ArrayData* a,
                                             int64_t key,
                                             Cell* value,
@@ -570,11 +610,7 @@ static inline void SetElemArray(TypedValue* base, TypedValue* key,
     }
   }
 
-  if (newData != nullptr && newData != a) {
-    newData->incRefCount();
-    decRefArr(a);
-    base->m_data.parr = newData;
-  }
+  arrayRefShuffle<true>(a, newData, base);
 }
 // SetElem() leaves the result in 'value', rather than returning it as in
 // SetOpElem(), because doing so avoids a dup operation that SetOpElem() can't

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

@@ -346,6 +346,8 @@ CALL_OPCODE(SetProp)
 CALL_OPCODE(ElemX)
 CALL_OPCODE(ElemDX)
 CALL_OPCODE(CGetElem)
+CALL_OPCODE(ArraySet)
+CALL_OPCODE(ArraySetRef)
 CALL_OPCODE(SetElem)
 CALL_OPCODE(SetNewElem)
 CALL_OPCODE(IssetElem)

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

@@ -431,6 +431,7 @@ private:
     void emitSetOpNewElem();
     void emitIncDecNewElem();
     void emitBindNewElem();
+    void emitSimpleArraySet(SSATmp* key, SSATmp* value);
 
     // Misc Helpers
     void numberStackInputs();
@@ -440,6 +441,7 @@ private:
     SSATmp* checkInitProp(SSATmp* baseAsObj, SSATmp* propAddr, int propOffset,
                           bool warn, bool define);
 
+    bool isSimpleArraySet();
     bool generateMVal() const;
     bool needFirstRatchet() const;
     bool needFinalRatchet() const;

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

@@ -21,6 +21,7 @@
 #include <forward_list>
 #include <sstream>
 #include <type_traits>
+#include <boost/algorithm/string.hpp>
 
 #include "folly/Format.h"
 #include "folly/Traits.h"
@@ -380,6 +381,10 @@ bool IRInstruction::consumesReference(int srcNo) const {
     // StProp[NT]|StMem[NT] <base>, <offset>, <value>
     return srcNo == 2;
   }
+  if (m_op == ArraySet || m_op == ArraySetRef) {
+    // Only consumes the reference to its input array
+    return srcNo == 1;
+  }
   return true;
 }
 
@@ -444,9 +449,15 @@ bool IRInstruction::killsSources() const {
 bool IRInstruction::killsSource(int idx) const {
   if (!killsSources()) return false;
 
-  assert(m_op == DecRef);
-  assert(idx == 0);
-  return true;
+  if (m_op == DecRef) {
+    assert(idx == 0);
+    return true;
+  }
+  if (m_op == ArraySet || m_op == ArraySetRef) {
+    // Kills its input array
+    return idx == 1;
+  }
+  not_reached();
 }
 
 bool IRInstruction::mayReenterHelper() const {
@@ -819,8 +830,12 @@ static void printConst(std::ostream& os, IRInstruction* inst) {
     auto ne = c->as<const NamedEntity*>();
     os << "NamedEntity(" << ne << ")";
   } else if (t == Type::TCA) {
-    void* vp = c->as<TCA>();
-    os << folly::format("TCA: {}", vp);
+    TCA tca = c->as<TCA>();
+    char* nameRaw = Util::getNativeFunctionName(tca);
+    SCOPE_EXIT { free(nameRaw); };
+    std::string name(nameRaw);
+    boost::algorithm::trim(name);
+    os << folly::format("TCA: {}({})", tca, name);
   } else if (t == Type::None) {
     os << "None:" << c->as<int64_t>();
   } else if (t.isPtr()) {

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

@@ -391,6 +391,15 @@ O(CGetElem,                    D(Cell), C(TCA)                                \
                                           S(PtrToGen)                         \
                                           S(Gen)                              \
                                           S(PtrToCell),      E|N|Mem|Refs|Er) \
+O(ArraySet,                     D(Arr), C(TCA)                                \
+                                          S(Arr)                              \
+                                          S(Int,Str)                          \
+                                          S(Cell),    E|N|PRc|CRc|Refs|Mem|K) \
+O(ArraySetRef,                      ND, C(TCA)                                \
+                                          S(Arr)                              \
+                                          S(Int,Str)                          \
+                                          S(Cell)                             \
+                                          S(BoxedArr),E|N|PRc|CRc|Refs|Mem|K) \
 O(SetElem,                     DVector, C(TCA)                                \
                                           S(PtrToGen)                         \
                                           S(Gen)                              \

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

@@ -268,8 +268,10 @@ private:
       case SetProp:
       case SetElem:
       case SetNewElem:
-      case ElemDX:      return true;
-      default:          return false;
+      case ElemDX:
+        return true;
+      default:
+        return false;
     }
   }
 

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

@@ -111,6 +111,10 @@ static CallMap s_callMap({
                  {{SSA, 1}, {VecKeyIS, 2}, {SSA, 3}}},
     {CGetElem, {FSSA, 0}, DTV, SSync,
                  {{SSA, 1}, {VecKeyIS, 2}, {SSA, 3}}},
+    {ArraySet, {FSSA, 0}, DSSA, SSync,
+                 {{SSA, 1}, {SSA, 2}, {TV, 3}}},
+    {ArraySetRef, {FSSA, 0}, DSSA, SSync,
+                 {{SSA, 1}, {SSA, 2}, {TV, 3}, {SSA, 4}}},
     {SetElem,  {FSSA, 0}, DTV, SSync,
                  {{SSA, 1}, {VecKeyIS, 2}, {TV, 3}}},
     {SetNewElem, (TCA)setNewElem, DTV, SSync, {{SSA, 0}, {TV, 1}}},

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

@@ -167,8 +167,8 @@ bool TraceBuilder::isValueAvailable(SSATmp* tmp) const {
 
     if (srcOpcode == LdThis) return true;
 
-    if (srcOpcode == IncRef || srcOpcode == Mov) {
-      tmp = srcInstr->getSrc(0);
+    if (srcInstr->isPassthrough()) {
+      tmp = srcInstr->getPassthroughValue();
     } else {
       return false;
     }

hphp/runtime/vm/translator/hopt/vectortranslator-internal.h

@@ -80,10 +80,10 @@ inline unsigned buildBitmask(T c, Args... args) {
 }
 
 // FILL_ROW and BUILD_OPTAB* build up the static table of function pointers
-#define FILL_ROW(nm, ...) do {                          \
-    OpFunc* dest = &optab[buildBitmask(__VA_ARGS__)];   \
-    assert(*dest == nullptr);                           \
-    *dest = nm;                                         \
+#define FILL_ROW(nm, ...) do {                                  \
+    OpFunc* dest = &optab[buildBitmask(__VA_ARGS__)];           \
+    assert(*dest == nullptr);                                   \
+    *dest = (OpFunc)nm;                                         \
   } while (false);
 #define FILL_ROW_HOT(nm, hot, ...) FILL_ROW(nm, __VA_ARGS__)
 

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

@@ -216,7 +216,6 @@ HhbcTranslator::VectorTranslator::VectorTranslator(
 }
 
 void HhbcTranslator::VectorTranslator::emit() {
-  PUNT_WITH_TX64(VectorTranslator);
   // Assign stack slots to our stack inputs
   numberStackInputs();
   // Emit the base and every intermediate op
@@ -414,6 +413,12 @@ void HhbcTranslator::VectorTranslator::emitBaseLCR() {
         m_base = m_tb.gen(LdRef, Type::Obj, failedRef, m_base);
       }
       assert(m_base->isA(Type::Obj));
+    } else if (isSimpleArraySet()) {
+      m_base = m_tb.genLdLoc(loc);
+      if (baseType.isBoxed()) {
+        m_base = m_tb.gen(LdRef, Type::Arr, failedRef, m_base);
+      }
+      assert(m_base->isA(Type::Arr));
     } else {
       // Everything else is passed by reference. We don't have to worry about
       // unboxing here, since all the generic helpers understand boxed bases.
@@ -430,6 +435,22 @@ void HhbcTranslator::VectorTranslator::emitBaseLCR() {
   }
 }
 
+// Is the current instruction a 1-element vector, with an Arr base and Int or
+// Str key?
+bool HhbcTranslator::VectorTranslator::isSimpleArraySet() {
+  SSATmp* base = getInput(m_mii.valCount());
+  LocationCode loc = m_ni.immVec.locationCode();
+  if (m_ni.mInstrOp() == OpSetM &&
+      (loc == LL || loc == LC || loc == LR) &&
+      m_ni.immVecM.size() == 1 &&
+      mcodeMaybeArrayKey(m_ni.immVecM[0]) &&
+      base->getType().subtypeOfAny(Type::Arr, Type::BoxedArr)) {
+    SSATmp* key = getInput(m_mii.valCount() + 1);
+    return key->isA(Type::Int) || key->isA(Type::Str);
+  }
+  return false;
+}
+
 void HhbcTranslator::VectorTranslator::emitBaseH() {
   m_base = m_tb.genLdThis(nullptr);
 }
@@ -1167,13 +1188,118 @@ static TypedValue nm(TypedValue* base, TypedValue key, Cell val) {      \
 HELPER_TABLE(ELEM)
 #undef ELEM
 
+static inline ArrayData* checkedSet(ArrayData* a, StringData* key,
+                                    CVarRef value, bool copy) {
+  int64_t i;
+  return UNLIKELY(key->isStrictlyInteger(i)) ? a->set(i, value, copy)
+                                             : a->set(key, value, copy);
+}
+
+static inline ArrayData* checkedSet(ArrayData* a, int64_t key,
+                                    CVarRef value, bool copy) {
+  not_reached();
+}
+
+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) {
+  static_assert(keyType != AnyKey, "AnyKey is not supported in arraySetMImpl");
+  const bool copy = a->getCount() > 1;
+  ArrayData* ret = checkForInt ? checkedSet(a, key, value, copy)
+                               : a->set(key, value, copy);
+
+  return arrayRefShuffle<setRef>(a, ret, setRef ? ref->tv() : nullptr);
+}
+
+#define HELPER_TABLE_ARRAY_SET(m)                                       \
+  /* name        hot          keyType  checkForInt setRef */            \
+  m(arraySetS,   HOT_FUNC_VM, StrKey,   false,     false)               \
+  m(arraySetSC,  HOT_FUNC_VM, StrKey,    true,     false)               \
+  m(arraySetI,   HOT_FUNC_VM, IntKey,   false,     false)               \
+  m(arraySetSR,  ,            StrKey,   false,      true)               \
+  m(arraySetSiR, ,            StrKey,    true,      true)               \
+  m(arraySetIR,  ,            IntKey,   false,      true)
+
+#define ELEM(nm, hot, keyType, checkForInt, setRef)                     \
+hot                                                                     \
+static typename ShuffleReturn<setRef>::return_type                      \
+nm(ArrayData* a, TypedValue* key, TypedValue value, RefData* ref) {     \
+  return arraySetImpl<keyType, checkForInt, setRef>(                    \
+    a, keyAsRaw<keyType>(key), tvAsCVarRef(&value), ref);               \
+}
+HELPER_TABLE_ARRAY_SET(ELEM)
+#undef ELEM
+
+void HhbcTranslator::VectorTranslator::emitSimpleArraySet(SSATmp* key,
+                                                          SSATmp* value) {
+  assert(key->getType().notBoxed());
+  assert(value->getType().notBoxed());
+  bool checkForInt = false;
+  KeyType keyType;
+  if (key->isA(Type::Int)) {
+    keyType = IntKey;
+  } else {
+    assert(key->isA(Type::Str));
+    keyType = StrKey;
+    if (key->isConst()) {
+      int64_t i;
+      if (key->getValStr()->isStrictlyInteger(i)) {
+        keyType = IntKey;
+        key = cns(i);
+      }
+    } else {
+      checkForInt = true;
+    }
+  }
+  const DynLocation& base = *m_ni.inputs[m_mii.valCount()];
+  bool setRef = base.outerType() == KindOfRef;
+  typedef ArrayData* (*OpFunc)(ArrayData*, TypedValue*, TypedValue, RefData*);
+  BUILD_OPTAB_ARG(HELPER_TABLE_ARRAY_SET(FILL_ROW_HOT),
+                  keyType, checkForInt, setRef);
+
+  // Don't spillStack below because the helper can't throw. It may reenter to
+  // call destructors so it has a sync point in nativecalls.cpp, but exceptions
+  // are swallowed at destructor boundaries right now: #2182869.
+  if (setRef) {
+    SSATmp* box;
+    if (base.location.space == Location::Local) {
+      box = m_tb.genLdLoc(base.location.offset);
+    } else if (base.location.space == Location::Stack) {
+      not_implemented();
+    } else {
+      not_reached();
+    }
+
+    m_tb.gen(ArraySetRef, cns((TCA)opFunc), m_base, key, value, box);
+  } else {
+    SSATmp* newArr = m_tb.gen(ArraySet, cns((TCA)opFunc), m_base, key, value);
+
+    // 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);
+    } else if (base.location.space == Location::Stack) {
+      not_implemented();
+    } else {
+      not_reached();
+    }
+  }
+
+  m_result = value;
+}
+
 void HhbcTranslator::VectorTranslator::emitSetElem() {
   const int kValIdx = 0;
   SSATmp* value = getInput(kValIdx);
+  SSATmp* key = getInput(m_iInd);
+
+  if (isSimpleArraySet()) {
+    emitSimpleArraySet(key, value);
+    return;
+  }
 
   // Emit the appropriate helper call.
   typedef TypedValue (*OpFunc)(TypedValue*, TypedValue, Cell);
-  SSATmp* key = getInput(m_iInd);
   BUILD_OPTAB_HOT(getKeyTypeIS(key), key->isBoxed());
   m_ht.spillStack();
   SSATmp* result = m_tb.gen(SetElem, cns((TCA)opFunc), ptr(m_base), key, value);
@@ -1181,6 +1307,7 @@ void HhbcTranslator::VectorTranslator::emitSetElem() {
   m_result = ve.valTypeChanged ? result : value;
 }
 #undef HELPER_TABLE
+#undef HELPER_TABLE_ARRAY_SET
 
 void HhbcTranslator::VectorTranslator::emitSetOpElem() {
   SPUNT(__func__);

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

@@ -3153,7 +3153,7 @@ TranslatorX64::translateSetMArray(const Tracelet& t,
 
   recordReentrantCall(i);
   // If we did not used boxed form, we need to tell the register allocator
-  // to associate rax with arrLoc
+  // to associate rax with arrLoc.
   if (!useBoxedForm) {
     // The array_setm helper returns the up-to-date array pointer in rax.
     // Therefore, we can bind rax to arrLoc and mark it as dirty.

hphp/test/vm/array-ref.php

@@ -0,0 +1,22 @@
+<?php
+
+function yes() { return true; }
+
+function main() {
+  $a = array();
+  $a['wat'] =& $a;
+  $b = $a; // Make sure the next line triggers COW
+  if (yes()) {
+    // Force a new tracelet
+    $a['wat'] = 5;
+  }
+  var_dump($a);
+}
+main();
+
+function main2(&$a) {
+  $a = array();
+  $a['foo'] = 'flee';
+}
+main2($z);
+var_dump($z);

hphp/test/vm/array-ref.php.exp

@@ -0,0 +1,5 @@
+int(5)
+array(1) {
+  ["foo"]=>
+  string(4) "flee"
+}