Apply metadata to the ir in translateTracelet

In one of my pending region compiler diffs, I added code to forget the
values/types of locals after some FCalls (not doing so was wrong but we were
mostly getting away with it). This exposed an issue where the known type of a
local in the IR was less specific that what Translator::analyze was using. The
more specific type came from metadata on instructions after the FCall, so this
diff applies metadat to the IR before translating each instruction, using the
same code I wrote for the region compiler.

The changes to apply the metadata are pretty small; most of the
changes in this diffs are real bugs I found while attempting an
alternate solution.

hphp/runtime/vm/jit/extra-data.h

@@ -358,6 +358,12 @@ struct InterpOneData : IRExtraData {
   // Opcode, in case we need to fix the stack differently. Some byte-
   // code instructions modify things below the top of the stack.
   Op opcode;
+
+  std::string show() const {
+    return folly::format("{}: bcOff:{}, popped:{}, pushed:{}",
+                         opcodeToName(opcode), bcOff, cellsPopped,
+                         cellsPushed).str();
+  }
 };
 
 /*

hphp/runtime/vm/jit/hhbc-translator.cpp

@@ -222,6 +222,7 @@ void HhbcTranslator::replace(uint32_t index, SSATmp* tmp) {
 }
 
 Type HhbcTranslator::topType(uint32_t idx) const {
+  FTRACE(5, "Asking for type of stack elem {}\n", idx);
   if (idx < m_evalStack.size()) {
     return m_evalStack.top(idx)->type();
   } else {
@@ -3663,6 +3664,7 @@ Type HhbcTranslator::interpOutputType(const NormalizedInstruction& inst) const {
     case OutClassRef:   return Type::Cls;
     case OutSetM:       return Type::Cell; // Imprecise but we can translate
                                            // all cases that matter.
+    case OutFPushCufSafe: return Type::None;
 
     case OutNone:       return Type::None;
   }
@@ -3678,6 +3680,14 @@ void HhbcTranslator::interpOutputLocals(const NormalizedInstruction& inst) {
   };
 
   switch (inst.op()) {
+    case OpCreateCont: {
+      auto numLocals = curFunc()->numLocals();
+      for (unsigned i = 0; i < numLocals; ++i) {
+        gen(OverrideLoc, Type::Uninit, LocalId(i), m_tb->fp());
+      }
+      break;
+    }
+
     case OpSetN:
     case OpSetOpN:
     case OpIncDecN:
@@ -3833,10 +3843,10 @@ std::string HhbcTranslator::showStack() const {
     out << folly::format("+{:-^62}+\n", str);
   };
 
-  const int32_t frameCells = curFunc()->numSlotsInFrame();
+  const int32_t frameCells =
+    curFunc()->isGenerator() ? 0 : curFunc()->numSlotsInFrame();
   const int32_t stackDepth =
-    m_tb->spOffset() + m_evalStack.size() - m_stackDeficit -
-    (curFunc()->isGenerator() ? 0 : frameCells);
+    m_tb->spOffset() + m_evalStack.size() - m_stackDeficit - frameCells;
   auto spOffset = stackDepth;
   auto elem = [&](const std::string& str) {
     out << folly::format("| {:<60} |\n",
@@ -3845,12 +3855,23 @@ std::string HhbcTranslator::showStack() const {
     assert(spOffset > 0);
     --spOffset;
   };
-  auto fpiStack = m_fpiStack;
+
+  auto fpi = curFunc()->findFPI(bcOff());
   auto checkFpi = [&]() {
-    if (!fpiStack.empty() &&
-        spOffset - kNumActRecCells == fpiStack.top().second) {
-      for (unsigned i = 0; i < kNumActRecCells; ++i) elem("ActRec");
-      fpiStack.pop();
+    if (fpi && spOffset + frameCells == fpi->m_fpOff) {
+      auto fpushOff = fpi->m_fpushOff;
+      auto after = fpushOff + instrLen((Op*)curUnit()->at(fpushOff));
+      std::ostringstream msg;
+      msg << "ActRec from ";
+      curUnit()->prettyPrint(msg, Unit::PrintOpts().range(fpushOff, after)
+                                                   .noLineNumbers()
+                                                   .indent(0));
+      auto msgStr = msg.str();
+      assert(msgStr.back() == '\n');
+      msgStr.erase(msgStr.size() - 1);
+      for (unsigned i = 0; i < kNumActRecCells; ++i) elem(msgStr);
+      fpi = fpi->m_parentIndex != -1 ? &curFunc()->fpitab()[fpi->m_parentIndex]
+                                     : nullptr;
       return true;
     }
     return false;

hphp/runtime/vm/jit/ir-translator.cpp

@@ -1337,8 +1337,10 @@ IRTranslator::translateFCall(const NormalizedInstruction& i) {
         m_hhbcTrans.profileInlineFunctionShape(traceletShape(*i.calleeTrace));
       }
 
+      Unit::MetaHandle metaHand;
       for (auto* ni = i.calleeTrace->m_instrStream.first;
            ni; ni = ni->next) {
+        readMetaData(metaHand, *ni, m_hhbcTrans, MetaMode::Legacy);
         translateInstr(*ni);
       }
       return;
@@ -1682,15 +1684,14 @@ void IRTranslator::interpretInstr(const NormalizedInstruction& i) {
 }
 
 void IRTranslator::translateInstr(const NormalizedInstruction& i) {
+  m_hhbcTrans.setBcOff(i.source.offset(),
+                       i.breaksTracelet && !m_hhbcTrans.isInlining());
   FTRACE(1, "\n{:-^60}\n", folly::format("translating {} with stack:\n{}",
                                          i.toString(),
                                          m_hhbcTrans.showStack()));
   // When profiling, we disable type predictions to avoid side exits
   assert(Transl::tx64->mode() != TransProfile || !i.outputPredicted);
 
-  m_hhbcTrans.setBcOff(i.source.offset(),
-                       i.breaksTracelet && !m_hhbcTrans.isInlining());
-
   if (i.guardedThis) {
     // Task #2067635: This should really generate an AssertThis
     m_hhbcTrans.setThisAvailable();

hphp/runtime/vm/jit/simplifier.cpp

@@ -33,6 +33,7 @@ TRACE_SET_MOD(hhir);
 //////////////////////////////////////////////////////////////////////
 
 StackValueInfo getStackValue(SSATmp* sp, uint32_t index) {
+  FTRACE(5, "getStackValue: idx = {}, {}\n", index, sp->inst()->toString());
   assert(sp->isA(Type::StkPtr));
   IRInstruction* inst = sp->inst();
 
@@ -84,7 +85,7 @@ StackValueInfo getStackValue(SSATmp* sp, uint32_t index) {
       getStackValue(inst->src(0),
                     // Pushes a return value, pops an ActRec and args Array
                     index -
-                      (1 /* pushed */ - kNumActRecCells + 1 /* popped */));
+                      (1 /* pushed */ - (kNumActRecCells + 1) /* popped */));
     info.spansCall = true;
     return info;
   }
@@ -97,7 +98,7 @@ StackValueInfo getStackValue(SSATmp* sp, uint32_t index) {
     auto info =
       getStackValue(inst->src(0),
                     index -
-                     (1 /* pushed */ - kNumActRecCells /* popped */));
+                    (1 /* pushed */ - kNumActRecCells /* popped */));
     info.spansCall = true;
     return info;
   }
@@ -148,6 +149,10 @@ StackValueInfo getStackValue(SSATmp* sp, uint32_t index) {
       if (index == 0) return StackValueInfo { Type::Cell };
       if (index == 1) return StackValueInfo { Type::Int };
       break;
+    case Op::FPushCufSafe:
+      if (index == kNumActRecCells) return StackValueInfo { Type::Bool };
+      if (index == kNumActRecCells + 1) return getStackValue(prevSp, 0);
+      break;
 
     default:
       if (index == 0 && !resultType.equals(Type::None)) {
@@ -165,9 +170,9 @@ StackValueInfo getStackValue(SSATmp* sp, uint32_t index) {
 
   case SpillFrame:
   case CufIterSpillFrame:
-    return getStackValue(inst->src(0),
-                         // pushes an ActRec
-                         index - kNumActRecCells);
+    // pushes an ActRec
+    if (index < kNumActRecCells) return StackValueInfo { nullptr };
+    return getStackValue(inst->src(0), index - kNumActRecCells);
 
   default:
     {

hphp/runtime/vm/jit/simplifier.h

@@ -163,17 +163,34 @@ struct StackValueInfo {
     : value(value)
     , knownType(value ? value->type() : Type::None)
     , spansCall(false)
-  {}
+  {
+    TRACE(5, "%s created\n", show().c_str());
+  }
 
   explicit StackValueInfo(Type type)
     : value(nullptr)
     , knownType(type)
     , spansCall(false)
-  {}
+  {
+    TRACE(5, "%s created\n", show().c_str());
+  }
+
+  std::string show() const {
+    std::ostringstream out;
+    out << "StackValueInfo {";
+    out << (value ? value->inst()->toString() : knownType.toString());
+    if (spansCall) out << ", spans call";
+    out << "}";
+
+    return out.str();
+  }
 
   SSATmp* value;   // may be null
   Type knownType;  // currently Type::None if we don't know (TODO(#2135185)
   bool spansCall;  // whether the tmp's definition was above a call
+
+ private:
+  TRACE_SET_MOD(hhir);
 };
 
 /*

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

@@ -3468,9 +3468,12 @@ TranslatorX64::translateTracelet(Tracelet& t) {
       ht.profileSmallFunctionShape(traceletShape(t));
     }();
 
+    Unit::MetaHandle metaHand;
     // Translate each instruction in the tracelet
     for (auto* ni = t.m_instrStream.first; ni && !ht.hasExit();
          ni = ni->next) {
+      readMetaData(metaHand, *ni, m_irTrans->hhbcTrans(), MetaMode::Legacy);
+
       try {
         SKTRACE(1, ni->source, "HHIR: translateInstr\n");
         assert(!(m_mode == TransProfile && ni->outputPredicted && ni->next));

hphp/runtime/vm/jit/translator.cpp

@@ -1177,7 +1177,7 @@ static const struct {
   { OpFPushCufF,   {Stack1,           FStack,       OutFDesc,
                                                      kNumActRecCells - 1 }},
   { OpFPushCufSafe,{StackTop2|DontGuardAny,
-                                      StackCufSafe, OutFDesc,
+                                      StackTop2|FStack, OutFPushCufSafe,
                                                          kNumActRecCells }},
   { OpFPassC,      {FuncdRef,         None,         OutSameAsInput,    0 }},
   { OpFPassCW,     {FuncdRef,         None,         OutSameAsInput,    0 }},
@@ -1266,7 +1266,7 @@ static const struct {
 
   /*** 14. Continuation instructions ***/
 
-  { OpCreateCont,  {None,             Stack1,       OutObject,         1 }},
+  { OpCreateCont,  {None,             Stack1|Local, OutObject,         1 }},
   { OpContEnter,   {Stack1,           None,         OutNone,          -1 }},
   { OpUnpackCont,  {None,             StackTop2,    OutInt64,          2 }},
   { OpContSuspend, {Stack1,           None,         OutNone,          -1 }},
@@ -1360,12 +1360,6 @@ int64_t getStackPopped(const NormalizedInstruction& ni) {
 }
 
 int64_t getStackPushed(const NormalizedInstruction& ni) {
-  switch (ni.op()) {
-    case OpFPushCufSafe:   return kNumActRecCells + 2;
-
-    default:               break;
-  }
-
   return countOperands(getInstrInfo(ni.op()).out);
 }
 
@@ -1973,6 +1967,7 @@ bool outputDependsOnInput(const Op instr) {
     case OutStrlen:
     case OutNone:
       return false;
+
     case OutFDesc:
     case OutSameAsInput:
     case OutCInput:
@@ -1986,6 +1981,7 @@ bool outputDependsOnInput(const Op instr) {
     case OutSetOp:
     case OutIncDec:
     case OutSetM:
+    case OutFPushCufSafe:
       return true;
   }
   not_reached();
@@ -2101,6 +2097,13 @@ void Translator::getOutputs(/*inout*/ Tracelet& t,
           varEnvTaint = true;
           continue;
         }
+        if (op == OpCreateCont) {
+          // CreateCont stores Uninit to all locals but NormalizedInstruction
+          // doesn't have enough output fields, so we special case it in
+          // analyze().
+          continue;
+        }
+
         ASSERT_NOT_IMPLEMENTED(op == OpSetOpL ||
                                op == OpSetM || op == OpSetOpM ||
                                op == OpBindM ||
@@ -2283,9 +2286,22 @@ void Translator::getOutputs(/*inout*/ Tracelet& t,
       } continue; // already pushed an output for the local
 
       case Stack1:
-      case Stack2:
+      case Stack2: {
         loc = Location(Location::Stack, currentStackOffset++);
-        break;
+        if (ni->op() == OpFPushCufSafe) {
+          // FPushCufSafe pushes its first stack input, then a bool.
+          if (opnd == Stack2) {
+            assert(ni->outStack == nullptr);
+            auto* dl = t.newDynLocation(loc, ni->inputs[0]->rtt);
+            ni->outStack = dl;
+          } else {
+            assert(ni->outStack2 == nullptr);
+            auto* dl = t.newDynLocation(loc, KindOfBoolean);
+            ni->outStack2 = dl;
+          }
+          continue;
+        }
+      } break;
       case StackIns1: {
         // First stack output is where the inserted element will go.
         // The output code for the instruction will affect what we
@@ -3416,6 +3432,15 @@ std::unique_ptr<Tracelet> Translator::analyze(SrcKey sk,
         tas.recordWrite(o);
       }
     }
+    if (ni->op() == OpCreateCont) {
+      // CreateCont stores Uninit to all locals but NormalizedInstruction
+      // doesn't have enough output fields, so we special case it here.
+      auto const numLocals = curFunc()->numLocals();
+      for (unsigned i = 0; i < numLocals; ++i) {
+        tas.recordWrite(t.newDynLocation(Location(Location::Local, i),
+                                         KindOfUninit));
+      }
+    }
 
     SKTRACE(1, sk, "stack args: virtual sfo now %d\n", stackFrameOffset);
 
@@ -3625,7 +3650,7 @@ const char* Translator::translateResultName(TranslateResult r) {
  * eventually replace applyInputMetaData.
  */
 void readMetaData(Unit::MetaHandle& handle, NormalizedInstruction& inst,
-                  HhbcTranslator& hhbcTrans) {
+                  HhbcTranslator& hhbcTrans, MetaMode metaMode /* = Normal */) {
   if (!handle.findMeta(inst.unit(), inst.offset())) return;
 
   Unit::MetaInfo info;
@@ -3660,6 +3685,13 @@ void readMetaData(Unit::MetaHandle& handle, NormalizedInstruction& inst,
                    !(iInfo.in & Stack2) ? 1 :
                    !(iInfo.in & Stack3) ? 2 : 3;
 
+  auto stackFilter = [metaMode, &inst](Location loc) {
+    if (metaMode == MetaMode::Legacy && loc.space == Location::Stack) {
+      loc.offset = -(loc.offset + 1) + inst.stackOffset;
+    }
+    return loc;
+  };
+
   do {
     SKTRACE(3, inst.source, "considering MetaInfo of kind %d\n", info.m_kind);
 
@@ -3667,7 +3699,7 @@ void readMetaData(Unit::MetaHandle& handle, NormalizedInstruction& inst,
       base + (info.m_arg & ~Unit::MetaInfo::VectorArg) : info.m_arg;
     auto updateType = [&]{
       auto& input = *inst.inputs[arg];
-      input.rtt = hhbcTrans.rttFromLocation(input.location);
+      input.rtt = hhbcTrans.rttFromLocation(stackFilter(input.location));
     };
 
     switch (info.m_kind) {
@@ -3681,7 +3713,8 @@ void readMetaData(Unit::MetaHandle& handle, NormalizedInstruction& inst,
         inst.imm[0].u_IVA = info.m_data;
         break;
       case Unit::MetaInfo::Kind::DataTypePredicted: {
-        auto const loc = inst.inputs[arg]->location.toLocation();
+        if (metaMode == MetaMode::Legacy) break;
+        auto const loc = stackFilter(inst.inputs[arg]->location).toLocation();
         auto const t = Type::fromDataType(DataType(info.m_data));
         auto const offset = inst.source.offset();
 
@@ -3695,14 +3728,15 @@ void readMetaData(Unit::MetaHandle& handle, NormalizedInstruction& inst,
       }
       case Unit::MetaInfo::Kind::DataTypeInferred: {
         hhbcTrans.assertTypeLocation(
-          inst.inputs[arg]->location.toLocation(),
+          stackFilter(inst.inputs[arg]->location).toLocation(),
           Type::fromDataType(DataType(info.m_data)));
         updateType();
         break;
       }
       case Unit::MetaInfo::Kind::String: {
-        hhbcTrans.assertString(inst.inputs[arg]->location.toLocation(),
-                               inst.unit()->lookupLitstrId(info.m_data));
+        hhbcTrans.assertString(
+          stackFilter(inst.inputs[arg]->location).toLocation(),
+          inst.unit()->lookupLitstrId(info.m_data));
         updateType();
         break;
       }

hphp/runtime/vm/jit/translator.h

@@ -1073,7 +1073,12 @@ SrcKey nextSrcKey(const NormalizedInstruction& i);
 
 void populateImmediates(NormalizedInstruction&);
 void preInputApplyMetaData(Unit::MetaHandle, NormalizedInstruction*);
-void readMetaData(Unit::MetaHandle&, NormalizedInstruction&, HhbcTranslator&);
+enum class MetaMode {
+  Normal,
+  Legacy,
+};
+void readMetaData(Unit::MetaHandle&, NormalizedInstruction&, HhbcTranslator&,
+                  MetaMode m = MetaMode::Normal);
 bool instrMustInterp(const NormalizedInstruction&);
 
 typedef std::function<Type(int)> LocalTypeFn;
@@ -1122,6 +1127,8 @@ enum OutTypeConstraints {
   OutStrlen,            // OpStrLen
   OutClassRef,          // KindOfClass
   OutSetM,              // SetM is a very special snowflake
+  OutFPushCufSafe,      // FPushCufSafe pushes two values of different
+                        // types and an ActRec
   OutNone
 };
 
@@ -1156,7 +1163,6 @@ enum Operands {
                              // n = imm[0].u_IVA
   StackTop2 = Stack1 | Stack2,
   StackTop3 = Stack1 | Stack2 | Stack3,
-  StackCufSafe = StackIns1 | FStack
 };
 
 inline Operands operator|(const Operands& l, const Operands& r) {