Some cleanup on the IR Call instruction

Summary: I was looking at how hard it would be to leave the things on
the stack instead of popping them (I think it is doable---I will try
in a little bit), but I did this separately in case that change
doesn't work.  This diff moves the compile time constants to extra
data, (finally) hooks it into the type assertion pass, and fixes the
docs a bit (a bit misleading about the meaning of S3, and still
referred to Type::None wrt a broken optimization we deleted a while
ago).

Reviewed By: @edwinsmith

Differential Revision: D1310673

hphp/doc/ir.specification

@@ -1194,12 +1194,12 @@ D:StkPtr = CallArray S0:StkPtr
   arguments is pushed. CallArray pops the array off the stack, pushes the
   elements of the array as arguments, and invokes the function in the ActRec.
 
-D:StkPtr = Call S0:StkPtr S1:FramePtr S2:ConstInt S3:Func S4...
+D:StkPtr = Call<returnOff,funcd,destroyLocals> S0:StkPtr S1:FramePtr [S2...SN]
 
-  Invoke the function S3 with ActRec S0 and variadic arguments S4...
-  representing values to pass to the function. A value of type None means the
-  value to be passed is already on the stack. S1 is the caller FP and S2 is
-  the bytecode offset of the next instruction to execute when the call returns.
+  Transfer control to a callee, based on the pre-live activation record pointed
+  to by S0.  S1 is the current caller frame pointer, and a variadic list of
+  SSATmps is passed for the arguments. The `funcd' in the extra data is a Func*
+  for the callee if we know the callee statically.
 
 NativeImpl = S0:ConstFunc S1:FramePtr
 

hphp/runtime/vm/jit/code-gen-arm.cpp

@@ -1671,29 +1671,29 @@ void CodeGenerator::cgCallBuiltin(IRInstruction* inst) {
 }
 
 void CodeGenerator::cgCall(IRInstruction* inst) {
-  auto spReg = x2a(srcLoc(0).reg());
-  auto fpReg = x2a(srcLoc(1).reg());
-  auto* returnBcOffset = inst->src(2);
-  auto* func = inst->src(3);
-  SrcRange args = inst->srcs().subpiece(4);
+  auto const spReg = x2a(srcLoc(0).reg());
+  auto const fpReg = x2a(srcLoc(1).reg());
+  auto const extra = inst->extra<Call>();
+  SrcRange args = inst->srcs().subpiece(2);
   int32_t numArgs = args.size();
 
   int64_t adjustment = cellsToBytes((int64_t)-numArgs);
   for (int32_t i = 0; i < numArgs; ++i) {
-    emitStore(spReg, cellsToBytes(-(i + 1)), args[i], srcLoc(i + 4));
+    emitStore(spReg, cellsToBytes(-(i + 1)), args[i], srcLoc(i + 2));
   }
 
   m_as.  Str  (fpReg, spReg[AROFF(m_sfp)]);
-  m_as.  Mov  (rAsm.W(), returnBcOffset->intVal());
+  m_as.  Mov  (rAsm.W(), extra->after);
   m_as.  Str  (rAsm.W(), spReg[AROFF(m_soff)]);
   emitRegGetsRegPlusImm(m_as, spReg, spReg, adjustment);
 
   assert(m_curInst->marker().valid());
-  SrcKey srcKey = m_curInst->marker().sk();
-  bool isImmutable = func->isConst() && !func->isA(Type::Null);
-  const Func* funcd = isImmutable ? func->funcVal() : nullptr;
-  int32_t adjust  = emitBindCall(mcg->code.main(), mcg->code.stubs(),
-                                 srcKey, funcd, numArgs);
+  auto const srcKey = m_curInst->marker().sk();
+  int32_t adjust = emitBindCall(mcg->code.main(),
+                                mcg->code.stubs(),
+                                srcKey,
+                                extra->callee,
+                                numArgs);
 
   emitRegGetsRegPlusImm(m_as, rVmSp, rVmSp, adjust);
 }

hphp/runtime/vm/jit/code-gen-x64.cpp

@@ -3618,33 +3618,34 @@ void CodeGenerator::cgCallArray(IRInstruction* inst) {
 }
 
 void CodeGenerator::cgCall(IRInstruction* inst) {
-  auto spReg = srcLoc(0).reg();
-  auto fpReg = srcLoc(1).reg();
-  SSATmp* returnBcOffset = inst->src(2);
-  SSATmp* func           = inst->src(3);
-  SrcRange args          = inst->srcs().subpiece(4);
-  int numArgs            = args.size();
+  auto const extra   = inst->extra<Call>();
+  auto const spReg   = srcLoc(0).reg();
+  auto const fpReg   = srcLoc(1).reg();
+  auto const args    = inst->srcs().subpiece(2);
+  auto const numArgs = args.size();
 
   // put all outgoing arguments onto the VM stack
   int adjustment = -numArgs * sizeof(Cell);
   for (int i = 0; i < numArgs; i++) {
-    cgStore(spReg[-(i + 1) * sizeof(Cell)], args[i], srcLoc(i+4), Width::Full);
+    cgStore(spReg[-(i + 1) * sizeof(Cell)], args[i],
+      srcLoc(i + 2), Width::Full);
   }
   // store the return bytecode offset into the outgoing actrec
-  auto returnBc = safe_cast<int32_t>(returnBcOffset->intVal());
+  auto returnBc = safe_cast<int32_t>(extra->after);
   m_as.storeq(fpReg, spReg[AROFF(m_sfp)]);
   m_as.storel(returnBc, spReg[AROFF(m_soff)]);
   if (adjustment != 0) {
     m_as.addq(adjustment, spReg);
   }
 
   assert(m_curInst->marker().valid());
-  SrcKey srcKey = m_curInst->marker().sk();
-  bool isImmutable = func->isConst(Type::Func);
-  const Func* funcd = isImmutable ? func->funcVal() : nullptr;
+  auto const srcKey = m_curInst->marker().sk();
   assert(m_as.base() == m_mainCode.base());
-  int32_t adjust = emitBindCall(m_mainCode, m_stubsCode,
-                                srcKey, funcd, numArgs);
+  int32_t adjust = emitBindCall(m_mainCode,
+                                m_stubsCode,
+                                srcKey,
+                                extra->callee,
+                                numArgs);
   if (adjust) {
     m_as.addq (adjust, rVmSp);
   }

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

@@ -360,34 +360,56 @@ struct DefInlineFPData : IRExtraData {
   Type retTypePred;
 };
 
-/*
- * FCallArray offsets
- */
 struct CallArrayData : IRExtraData {
-  explicit CallArrayData(Offset pcOffset, Offset aft, bool destroyLocals)
+  explicit CallArrayData(Offset pcOffset, Offset after, bool destroyLocals)
     : pc(pcOffset)
-    , after(aft)
+    , after(after)
     , destroyLocals(destroyLocals)
   {}
 
   std::string show() const {
     return folly::to<std::string>(pc, ",", after,
-                                  destroyLocals ? " destroy locals" : "");
+                                  destroyLocals ? ",destroyLocals" : "");
+  }
+
+  Offset pc;                    // XXX why isn't this available in the marker?
+  Offset after;
+  bool destroyLocals;
+};
+
+struct CallBuiltinData : IRExtraData {
+  explicit CallBuiltinData(bool destroyLocals)
+    : destroyLocals(destroyLocals)
+  {}
+
+  std::string show() const {
+    return destroyLocals ? "destroyLocals" : "";
   }
 
-  Offset pc, after;
   bool destroyLocals;
 };
 
 struct CallData : IRExtraData {
-  explicit CallData(bool destroy)
-    : destroyLocals(destroy)
+  explicit CallData(Offset after,
+                    const Func* callee,
+                    bool destroy)
+    : after(after)
+    , callee(callee)
+    , destroyLocals(destroy)
   {}
 
   std::string show() const {
-    return destroyLocals ? "destroy locals" : "";
+    return folly::to<std::string>(
+      after,
+      callee
+        ? folly::format(",{}", callee->fullName()->data()).str()
+        : std::string{},
+      destroyLocals ? ",destroyLocals" : ""
+    );
   }
 
+  Offset after;
+  const Func* callee;  // nullptr if not statically known
   bool destroyLocals;
 };
 
@@ -807,7 +829,7 @@ X(ReqRetranslateOpt,            ReqRetransOptData);
 X(CheckCold,                    TransIDData);
 X(IncProfCounter,               TransIDData);
 X(Call,                         CallData);
-X(CallBuiltin,                  CallData);
+X(CallBuiltin,                  CallBuiltinData);
 X(CallArray,                    CallArrayData);
 X(RetCtrl,                      RetCtrlData);
 X(FunctionExitSurpriseHook,     RetCtrlData);

hphp/runtime/vm/jit/frame-state.cpp

@@ -196,8 +196,9 @@ void FrameState::getLocalEffects(const IRInstruction* inst,
   };
 
   auto killedCallLocals = false;
-  if ((inst->is(CallArray) && inst->extra<CallArrayData>()->destroyLocals) ||
-      (inst->is(Call, CallBuiltin) && inst->extra<CallData>()->destroyLocals)) {
+  if ((inst->is(CallArray) && inst->extra<CallArray>()->destroyLocals) ||
+      (inst->is(Call) && inst->extra<Call>()->destroyLocals) ||
+      (inst->is(CallBuiltin) && inst->extra<CallBuiltin>()->destroyLocals)) {
     clearLocals(hook);
     killedCallLocals = true;
   }

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

@@ -3056,43 +3056,45 @@ void HhbcTranslator::emitFPushClsMethodF(int32_t numParams) {
 void HhbcTranslator::emitFCallArray(const Offset pcOffset,
                                     const Offset after,
                                     bool destroyLocals) {
-  SSATmp* stack = spillStack();
-  gen(CallArray, CallArrayData(pcOffset, after, destroyLocals), stack);
+  auto const stack = spillStack();
+  gen(CallArray, CallArrayData { pcOffset, after, destroyLocals }, stack);
 }
 
 void HhbcTranslator::emitFCall(uint32_t numParams,
                                Offset returnBcOffset,
                                const Func* callee,
                                bool destroyLocals) {
-  SSATmp* params[numParams + 4];
+  SSATmp* params[numParams + 2];
   std::memset(params, 0, sizeof params);
   for (uint32_t i = 0; i < numParams; i++) {
     // DataTypeGeneric is used because the Call instruction just spills the
     // values to the stack unmodified.
-    params[numParams + 4 - i - 1] = popF(DataTypeGeneric);
+    params[numParams + 2 - i - 1] = popF(DataTypeGeneric);
   }
 
   params[0] = spillStack();
   params[1] = m_irb->fp();
-  params[2] = cns(returnBcOffset);
-  params[3] = callee ? cns(callee) : cns(Type::InitNull);
 
   if (RuntimeOption::EvalRuntimeTypeProfile) {
     for (uint32_t i = 0; i < numParams; i++) {
       if (callee != nullptr &&
-          params[numParams + 4 - i - 1]) {
+          params[numParams + 2 - i - 1]) {
         gen(TypeProfileFunc, TypeProfileData(i),
-            params[numParams + 4 - i - 1], cns(callee));
+            params[numParams + 2 - i - 1], cns(callee));
       } else  {
-        SSATmp* func = gen(LdARFuncPtr, m_irb->sp(), cns(0));
+        auto const func = gen(LdARFuncPtr, m_irb->sp(), cns(0));
         gen(TypeProfileFunc, TypeProfileData(i),
-            params[numParams + 4 - i - 1], func);
+            params[numParams + 2 - i - 1], func);
       }
     }
   }
 
   SSATmp** decayedPtr = params;
-  gen(Call, CallData(destroyLocals), std::make_pair(numParams + 4, decayedPtr));
+  gen(
+    Call,
+    CallData { returnBcOffset, callee, destroyLocals },
+    std::make_pair(numParams + 2, decayedPtr)
+  );
   if (!m_fpiStack.empty()) {
     m_fpiStack.pop();
   }
@@ -3184,7 +3186,7 @@ void HhbcTranslator::emitFCallBuiltin(uint32_t numArgs,
   auto const ret = gen(
     CallBuiltin,
     retType,
-    CallData(destroyLocals),
+    CallBuiltinData { destroyLocals },
     makeCatch(),
     std::make_pair(argsSize, (SSATmp**)&args)
   );

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

@@ -127,8 +127,8 @@ bool IRInstruction::consumesReference(int srcNo) const {
       return srcNo == 2;
 
     case Call:
-      // Inputs 3+ are arguments to the function
-      return srcNo >= 4;
+      // Inputs past 2 are arguments to the function
+      return srcNo >= 2;
 
     case ColAddElemC:
       // value at index 2

hphp/runtime/vm/jit/ir.h

@@ -513,7 +513,9 @@ O(Clone,                        D(Obj), S(Obj),                   N|E|PRc|Er) \
 O(LdRaw,                        DLdRaw, S(Str,Obj,Func),                  NF) \
 O(FreeActRec,              D(FramePtr), S(FramePtr),                      NF) \
 /*    name                      dstinfo srcinfo                      flags */ \
-O(Call,                      D(StkPtr), SUnk,                          E|CRc) \
+O(Call,                      D(StkPtr), S(StkPtr)                             \
+                                          S(FramePtr)                         \
+                                          SSpills,                     E|CRc) \
 O(CallArray,                 D(StkPtr), S(StkPtr),                   E|N|CRc) \
 O(CallBuiltin,                DBuiltin, SUnk,                     E|Er|N|PRc) \
 O(NativeImpl,                       ND, C(Func) S(FramePtr),             E|N) \

hphp/runtime/vm/jit/reg-alloc-x64.h

@@ -89,8 +89,7 @@ bool mayUseConst(const IRInstruction& inst, unsigned i) {
     if (i == 1) return okStore(cint);
     break;
   case Call:
-    if (i == 3) return true; // func
-    if (i >= 4) return okStore(cint);
+    if (i >= 2) return okStore(cint);
     break;
   case CallBuiltin:
     if (i >= 2) return true; // args -> ArgGroup.ssa()
@@ -285,7 +284,7 @@ bool storesCell(const IRInstruction& inst, uint32_t srcIdx) {
       return srcIdx >= 2 && srcIdx < inst.numSrcs();
 
     case Call:
-      return srcIdx >= 4 && srcIdx < inst.numSrcs();
+      return srcIdx >= 2 && srcIdx < inst.numSrcs();
 
     case CallBuiltin:
       return srcIdx >= 1 && srcIdx < inst.numSrcs();

hphp/runtime/vm/jit/reg-alloc.cpp

@@ -124,7 +124,6 @@ bool mustUseConst(const IRInstruction& inst, int i) {
   // handle special cases we can't derive from IR_OPCODES macro
   switch (inst.op()) {
   case LdAddr: return check(i == 1); // offset
-  case Call: return check(i == 2); // returnBcOffset
   case CallBuiltin: return check(i == 0); // f
   default: break;
   }