Avoid storing to the stack the same value just loaded from there

Sometimes SpillStack and Call end up storing onto the stack a value
that was just loaded from the same stack location.  There was already
code in cgSpillStack to avoid the extra stores in some cases, but that
still kept the LdStack alive.  This diff detects some of those cases
with SpillStack in the Simplifier, which allows the LdStack to be
eliminated by DCE.  This diff also adds similar support for Call.  The
operands that don't need to be store onto the stack are replaced with
None.

Note that the optimization in cgSpillStack is not subsumed by this
diff.  In the Simplifier, we cannot chase through IncRefs as done in
cgSpillStack -- that would result in an IncRef that is not consumed.

hphp/doc/ir.specification

@@ -782,8 +782,10 @@ D:StkPtr = FreeActRec S0:StkPtr
 D:StkPtr = Call S0:ActRec S1:ConstInt S2:Func S3...
 
   Invoke the function S2 with ActRec S0 and variadic arguments S3...
-  representing values to pass to the function.  S2 is the bytecode
-  offset of the next instruction to execute when the call returns.
+  representing values to pass to the function.  A value of type None
+  means the value to be passed is already on the stack.  S2 is the
+  bytecode offset of the next instruction to execute when the call
+  returns.
 
 NativeImpl = S0:ConstFunc S1:StkPtr
 
@@ -864,7 +866,7 @@ StRef
 StRefNT
 StRaw
 
-D:StkPtr = SpillStack         S0:StkP S1:ConstInt, S2...
+D:StkPtr = SpillStack S0:StkP S1:ConstInt, S2...
 
   SpillStack synchronizes the virtual execution stack with the
   physical stack by storing a variadic list of SSATmps to the physical
@@ -879,9 +881,10 @@ D:StkPtr = SpillStack         S0:StkP S1:ConstInt, S2...
                is pushed
 
      S2...   - variadic list of elements to spill, with values
-               representing cells or ActRecs.  Each temp of type
+               representing cells, ActRecs, or None.  Each temp of type
                ActRec is followed by kSpillStackActRecExtraArgs for
-               the contents of the ActRec.
+               the contents of the ActRec.  A temp with type None means
+               to keep the previous value on the stack.
 
 
 11. Trace exits

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

@@ -3110,7 +3110,11 @@ void CodeGenerator::cgCall(IRInstruction* inst) {
   // put all outgoing arguments onto the VM stack
   int64_t adjustment = (-(int64_t)numArgs) * sizeof(Cell);
   for (int32_t i = 0; i < numArgs; i++) {
-    cgStore(spReg, -(i + 1) * sizeof(Cell), args[i]);
+    // Type::None here means that the simplifier proved that the value
+    // matches the value already in memory, thus the store is redundant.
+    if (args[i]->getType() != Type::None) {
+      cgStore(spReg, -(i + 1) * sizeof(Cell), args[i]);
+    }
   }
   // store the return bytecode offset into the outgoing actrec
   uint64_t returnBc = returnBcOffset->getValInt();
@@ -3239,6 +3243,10 @@ void CodeGenerator::cgSpillStack(IRInstruction* inst) {
       emitSpillActRec(sp, offset, spillVals[i]);
       cellOff += kNumActRecCells;
       i += kSpillStackActRecExtraArgs;
+    } else if (spillVals[i]->getType() == Type::None) {
+      // The simplifier detected that we're storing the same value
+      // already in there.
+      ++cellOff;
     } else {
       auto* val = spillVals[i];
       auto* inst = val->getInstruction();

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

@@ -1167,6 +1167,7 @@ void Trace::print(std::ostream& os, const AsmInfo* asmInfo) const {
 }
 
 int32_t spillValueCells(IRInstruction* spillStack) {
+  assert(spillStack->getOpcode() == SpillStack);
   int32_t numSrcs = spillStack->getNumSrcs();
   int32_t ret = 0;
   for (int i = 2; i < numSrcs; ++i) {

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

@@ -135,6 +135,8 @@ SSATmp* Simplifier::simplify(IRInstruction* inst) {
   case LdClsCtx:     return simplifyLdClsCtx(inst);
   case GetCtxFwdCall:return simplifyGetCtxFwdCall(inst);
 
+  case SpillStack:   return simplifySpillStack(inst);
+  case Call:         return simplifyCall(inst);
 
   case Jmp_:
   case JmpInstanceOf:
@@ -162,8 +164,6 @@ SSATmp* Simplifier::simplify(IRInstruction* inst) {
   case LdClsMethodCache:
   case LdClsMethodFCache:
   case LdClsMethod:
-  case Call:
-  case SpillStack:
   case ExitTrace:
   case ExitSlow:
   case ExitGuardFailure:
@@ -234,6 +234,63 @@ static bool hoistGuardToLoad(SSATmp* tmp, Type type) {
   return false;
 }
 
+SSATmp* Simplifier::simplifySpillStack(IRInstruction* inst) {
+  SSATmp* sp = inst->getSrc(0);
+  auto const spDeficit = inst->getSrc(1)->getValInt();
+  auto       spillVals = inst->getSrcs().subpiece(2);
+  auto const numSpillSrcs = spillVals.size();
+  auto const spillCells = spillValueCells(inst);
+  int64_t adjustment = spDeficit - spillCells;
+  for (uint32_t i = 0, cellOff = 0; i < numSpillSrcs; i++) {
+    const int64_t offset = cellOff + adjustment;
+    if (spillVals[i]->getType() == Type::ActRec) {
+      cellOff += kNumActRecCells;
+      i += kSpillStackActRecExtraArgs;
+      continue;
+    }
+    auto* srcInst = spillVals[i]->getInstruction();
+    // If our value came from a LdStack on the same sp and offset,
+    // we don't need to spill it.
+    if (srcInst->getOpcode() == LdStack && srcInst->getSrc(0) == sp &&
+        srcInst->getSrc(1)->getValInt() == offset) {
+      spillVals[i] = m_tb->genDefNone();
+    }
+    cellOff++;
+  }
+
+  // Note: although the instruction might have been modified above, we still
+  // need to return nullptr so that it gets cloned later if it's stack-allocated
+  return nullptr;
+}
+
+SSATmp* Simplifier::simplifyCall(IRInstruction* inst) {
+  auto spillVals  = inst->getSrcs().subpiece(3);
+  IRInstruction* spillStack = m_tb->getSp()->getInstruction();
+  if (spillStack->getOpcode() != SpillStack) {
+    return nullptr;
+  }
+
+  SSATmp* sp = spillStack->getSrc(0);
+  int baseOffset = spillStack->getSrc(1)->getValInt() -
+                   spillValueCells(spillStack);
+  auto const numSpillSrcs = spillVals.size();
+  for (int32_t i = 0; i < numSpillSrcs; i++) {
+    const int64_t offset = -(i + 1) + baseOffset;
+    assert(spillVals[i]->getType() != Type::ActRec);
+    IRInstruction* srcInst = spillVals[i]->getInstruction();
+    // If our value came from a LdStack on the same sp and offset,
+    // we don't need to spill it.
+    if (srcInst->getOpcode() == LdStack && srcInst->getSrc(0) == sp &&
+        srcInst->getSrc(1)->getValInt() == offset) {
+      spillVals[i] = m_tb->genDefNone();
+    }
+  }
+
+  // Note: although the instruction might have been modified above, we still
+  // need to return nullptr so that it gets cloned later if it's stack-allocated
+  return nullptr;
+}
+
 SSATmp* Simplifier::simplifyLdCtx(IRInstruction* inst) {
   const Func* func = inst->getSrc(1)->getValFunc();
   if (func->isStatic()) {

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

@@ -75,6 +75,8 @@ private:
   SSATmp* simplifyLdCtx(IRInstruction*);
   SSATmp* simplifyLdClsCtx(IRInstruction*);
   SSATmp* simplifyGetCtxFwdCall(IRInstruction* inst);
+  SSATmp* simplifySpillStack(IRInstruction* inst);
+  SSATmp* simplifyCall(IRInstruction* inst);
 
 private:
   SSATmp* genDefInt(int64_t val);

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

@@ -376,6 +376,11 @@ SSATmp* TraceBuilder::genDefNull() {
   return gen(DefConst, Type::Null, &cdata);
 }
 
+SSATmp* TraceBuilder::genDefNone() {
+  ConstData cdata(0);
+  return gen(DefConst, Type::None, &cdata);
+}
+
 SSATmp* TraceBuilder::genConvToInt(SSATmp* src) {
   return gen(Conv, Type::Int, src);
 }
@@ -835,7 +840,9 @@ static SSATmp* getStackValue(SSATmp* sp,
           tmp = tmp->getInstruction()->getSrc(0);
         }
         type = tmp->getType();
-        return tmp;
+        if (!type.equals(Type::None)) {
+          return tmp;
+        }
       }
       ++numPushed;
     }

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

@@ -162,6 +162,7 @@ public:
   SSATmp* genDefUninit();
   SSATmp* genDefInitNull();
   SSATmp* genDefNull();
+  SSATmp* genDefNone();
   SSATmp* genJmp(Trace* target);
   SSATmp* genJmpCond(SSATmp* src, Trace* target, bool negate);
   void    genJmp(Block* target, SSATmp* src);

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

@@ -238,7 +238,8 @@ void assertOperandTypes(const IRInstruction* inst) {
                              "invalid src num");
 #define DofS(src)   checkDst(src < inst->getNumSrcs(),  \
                              "invalid src num");
-#define DParam      checkDst(inst->getTypeParam() != Type::None,        \
+#define DParam      checkDst(inst->getTypeParam() != Type::None ||      \
+                             inst->getOpcode() == DefConst /* for DefNone */, \
                              "DParam with paramType None");
 
 #define O(opcode, dstinfo, srcinfo, flags)      \