From 3e93f5404ee11b6960569e585fa0bcf8456354fa Mon Sep 17 00:00:00 2001 From: Jordan DeLong Date: Wed, 24 Apr 2013 21:15:01 -0700 Subject: [PATCH] Remove genDecRef from TraceBuilder Replace with a preOptimizeDecRef. Also caught a pseudo-issue where we relied on the isRefCounted short-circuit for correctness (we called genDecRef on Type::Cls). That short-circuit is duplicated in simplifier, and is stateless, so let's leave it there. Remove some dead code about BoxedCells and replace it with a comment in ir.specification based on a conversation with @bsimmers. (This is on top of all the other related diffs.) --- hphp/doc/ir.specification | 6 ++ .../vm/translator/hopt/hhbctranslator.cpp | 62 ++++++++--------- .../vm/translator/hopt/tracebuilder.cpp | 69 +++++++++---------- .../runtime/vm/translator/hopt/tracebuilder.h | 4 +- .../vm/translator/hopt/vectortranslator.cpp | 7 +- 5 files changed, 74 insertions(+), 74 deletions(-) diff --git a/hphp/doc/ir.specification b/hphp/doc/ir.specification index eb79daa66..4bbccbe07 100644 --- a/hphp/doc/ir.specification +++ b/hphp/doc/ir.specification @@ -1082,6 +1082,12 @@ DecRef S0:Gen Decrease the reference count of S0 by one, and call a destructor for types that require it if it goes to zero. + Note that although DecRef takes a Gen, we don't allow it to use + information about the inner types of a BoxedCell. This is because + we don't guard on the inner types of a BoxedCell except when doing + LdRef. For any S0 that is a strict subtype of BoxedCell, the DecRef + must just decref it as if it were a BoxedCell. + DecRefMem S0:PtrToGen S1:ConstInt Decref the value pointed to by S0 at offset S1 (in bytes), calling diff --git a/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp b/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp index c8740da8b..b6c7ff51b 100644 --- a/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp +++ b/hphp/runtime/vm/translator/hopt/hhbctranslator.cpp @@ -121,7 +121,7 @@ void HhbcTranslator::discard(unsigned n) { // type is the type expected on the stack. void HhbcTranslator::popDecRef(Type type) { if (SSATmp* src = m_evalStack.pop()) { - m_tb->genDecRef(src); + gen(DecRef, src); return; } @@ -349,7 +349,7 @@ void HhbcTranslator::emitUnboxRAux() { push(unboxed); } else { pushIncRef(unboxed); - m_tb->genDecRef(srcBox); + gen(DecRef, srcBox); } } @@ -1065,7 +1065,7 @@ void HhbcTranslator::emitStrlen() { push(cns(input->getValStr()->size())); } else { push(gen(LdRaw, Type::Int, input, cns(RawMemSlot::StrLen))); - m_tb->genDecRef(input); + gen(DecRef, input); } } else if (inType.isNull()) { popC(); @@ -1114,7 +1114,7 @@ SSATmp* HhbcTranslator::emitLdClsPropAddrOrExit(const StringData* propName, clsTmp, prop, cns(getCurClass())); - m_tb->genDecRef(prop); // safe to do early because prop is a string + gen(DecRef, prop); // safe to do early because prop is a string return addr; } @@ -1143,7 +1143,7 @@ SSATmp* HhbcTranslator::emitLdGblAddr(const StringData* gblName, Block* block) { // Note: Once we use control flow to implement IssetG/EmptyG, we can // use a LdGblAddr helper that decrefs name for us SSATmp* addr = gen(LdGblAddr, block, name); - m_tb->genDecRef(name); + gen(DecRef, name); return addr; } @@ -1217,7 +1217,7 @@ void HhbcTranslator::emitEmptyS(const StringData* propName) { void HhbcTranslator::emitIsTypeC(Type t) { SSATmp* src = popC(); push(gen(IsType, t, src)); - m_tb->genDecRef(src); + gen(DecRef, src); } void HhbcTranslator::emitIsTypeL(Type t, int id) { @@ -1280,7 +1280,7 @@ SSATmp* HhbcTranslator::emitJmpCondHelper(int32_t offset, target = getExitTrace(offset); } SSATmp* boolSrc = m_tb->genConvToBool(src); - m_tb->genDecRef(src); + gen(DecRef, src); return gen(negate ? JmpZero : JmpNZero, target, boolSrc); } @@ -1302,8 +1302,8 @@ void HhbcTranslator::emitCmp(Opcode opc) { SSATmp* src1 = popC(); SSATmp* src2 = popC(); push(m_tb->genCmp(opc, src2, src1)); - m_tb->genDecRef(src2); - m_tb->genDecRef(src1); + gen(DecRef, src2); + gen(DecRef, src1); } void HhbcTranslator::emitClsCnsD(int32_t cnsNameStrId, int32_t clsNameStrId) { @@ -1354,8 +1354,8 @@ void HhbcTranslator::emitAKExists() { } push(gen(AKExists, arr, key)); - m_tb->genDecRef(arr); - m_tb->genDecRef(key); + gen(DecRef, arr); + gen(DecRef, key); } void HhbcTranslator::emitFPassR() { @@ -1369,7 +1369,7 @@ void HhbcTranslator::emitFPassV() { Block* exit = getExitTrace()->front(); SSATmp* tmp = popV(); pushIncRef(gen(Unbox, exit, tmp)); - m_tb->genDecRef(tmp); + gen(DecRef, tmp); } void HhbcTranslator::emitFPushCufOp(VM::Op op, Class* cls, StringData* invName, @@ -1617,7 +1617,7 @@ void HhbcTranslator::emitFPushObjMethodD(int32_t numParams, LdClsMethod, clsTmp, cns(func->methodSlot()) ); if (res == MethodLookup::MethodFoundNoThis) { - m_tb->genDecRef(obj); + gen(DecRef, obj); objOrCls = clsTmp; } emitFPushActRec(funcTmp, objOrCls, numParams, @@ -1638,7 +1638,7 @@ void HhbcTranslator::emitFPushObjMethodD(int32_t numParams, // and decref the obj that was pushed as the this pointer since // the obj won't be in the actrec and thus MethodCache::lookup won't // decref it - m_tb->genDecRef(obj); + gen(DecRef, obj); objOrCls = cns(baseClass); } emitFPushActRec(cns(func), @@ -1838,7 +1838,7 @@ void HhbcTranslator::emitFCallBuiltin(uint32_t numArgs, for (int i = 0; i < numArgs; i++) { SSATmp* arg = popR(); if (i >= numArgs - numNonDefault) { - m_tb->genDecRef(arg); + gen(DecRef, arg); } } @@ -1894,7 +1894,7 @@ SSATmp* HhbcTranslator::emitDecRefLocalsInline(SSATmp* retVal) { if (mayHaveThis(getCurFunc())) { if (retValSrcLoc && retValSrcOpc == LdThis) { - m_tb->genDecRef(retVal); + gen(DecRef, retVal); } else { m_tb->genDecRefThis(); } @@ -1910,7 +1910,7 @@ SSATmp* HhbcTranslator::emitDecRefLocalsInline(SSATmp* retVal) { retValSrcLoc->inst()->getExtra()->locId : -1; for (int id = getCurFunc()->numLocals() - 1; id >= 0; --id) { if (retValLocId == id) { - m_tb->genDecRef(retVal); + gen(DecRef, retVal); } else { m_tb->genDecRefLoc(id); } @@ -2012,7 +2012,7 @@ void HhbcTranslator::emitSwitch(const ImmVector& iv, index = gen(LdSwitchObjIndex, switchVal, ssabase, ssatargets); } else if (type.subtypeOf(Type::Arr)) { - m_tb->genDecRef(switchVal); + gen(DecRef, switchVal); gen(Jmp_, getExitTrace(defaultOff)); return; } else { @@ -2080,7 +2080,7 @@ void HhbcTranslator::emitSSwitch(const ImmVector& iv) { : LdSSwitchDestSlow, data, testVal); - m_tb->genDecRef(testVal); + gen(DecRef, testVal); auto const stack = spillStack(); gen(SyncVMRegs, m_tb->getFp(), stack); gen(JmpIndirect, dest); @@ -2340,7 +2340,7 @@ void HhbcTranslator::emitInstanceOfD(int classNameStrId) { } if (!src->isA(Type::Obj)) { push(cns(false)); - m_tb->genDecRef(src); + gen(DecRef, src); return; } @@ -2380,7 +2380,7 @@ void HhbcTranslator::emitInstanceOfD(int classNameStrId) { checkClass, cns(maybeCls && !isNormalClass)) ); - m_tb->genDecRef(src); + gen(DecRef, src); } void HhbcTranslator::emitCastArray() { @@ -2422,7 +2422,7 @@ void HhbcTranslator::emitCastArray() { void HhbcTranslator::emitCastBool() { SSATmp* src = popC(); push(m_tb->genConvToBool(src)); - m_tb->genDecRef(src); + gen(DecRef, src); } void HhbcTranslator::emitCastDouble() { @@ -2434,7 +2434,7 @@ void HhbcTranslator::emitCastDouble() { push(cns(0.0)); } else if (fromType.isArray()) { push(gen(ConvArrToDbl, src)); - m_tb->genDecRef(src); + gen(DecRef, src); } else if (fromType.isBool()) { push(gen(ConvBoolToDbl, src)); } else if (fromType.isInt()) { @@ -2459,14 +2459,14 @@ void HhbcTranslator::emitCastInt() { push(cns(0)); } else if (fromType.isArray()) { push(gen(ConvArrToInt, src)); - m_tb->genDecRef(src); + gen(DecRef, src); } else if (fromType.isBool()) { push(gen(ConvBoolToInt, src)); } else if (fromType.isDbl()) { push(gen(ConvDblToInt, src)); } else if (fromType.isString()) { push(gen(ConvStrToInt, src)); - m_tb->genDecRef(src); + gen(DecRef, src); } else if (fromType.isObj()) { exceptionBarrier(); push(gen(ConvObjToInt, src)); @@ -2495,7 +2495,7 @@ void HhbcTranslator::emitCastString() { push(cns(StringData::GetStaticString(""))); } else if (fromType.isArray()) { push(cns(StringData::GetStaticString("Array"))); - m_tb->genDecRef(src); + gen(DecRef, src); } else if (fromType.isBool()) { push(gen(ConvBoolToStr, src)); } else if (fromType.isDbl()) { @@ -2532,7 +2532,7 @@ void HhbcTranslator::emitAGetC(const StringData* clsName) { if (isSupportedAGet(topC(), clsName)) { SSATmp* src = popC(); emitAGet(src, clsName); - m_tb->genDecRef(src); + gen(DecRef, src); } else { emitInterpOne(Type::Cls, 1); } @@ -2557,7 +2557,7 @@ void HhbcTranslator::emitBindMem(SSATmp* ptr, SSATmp* src) { exitBlock->insert(++markerInst, m_irFactory.gen(DecRef, prevValue)); gen(DecRefNZOrBranch, exitBlock, prevValue); } else { - m_tb->genDecRef(prevValue); + gen(DecRef, prevValue); } } @@ -2764,7 +2764,7 @@ void HhbcTranslator::emitBinaryArith(Opcode opc) { void HhbcTranslator::emitNot() { SSATmp* src = popC(); push(m_tb->genNot(m_tb->genConvToBool(src))); - m_tb->genDecRef(src); + gen(DecRef, src); } #define BINOP(Opp) \ @@ -2850,8 +2850,8 @@ void HhbcTranslator::emitXor() { SSATmp* tr = m_tb->genConvToBool(btr); SSATmp* tl = m_tb->genConvToBool(btl); push(m_tb->genConvToBool(gen(OpXor, tl, tr))); - m_tb->genDecRef(btl); - m_tb->genDecRef(btr); + gen(DecRef, btl); + gen(DecRef, btr); } /** diff --git a/hphp/runtime/vm/translator/hopt/tracebuilder.cpp b/hphp/runtime/vm/translator/hopt/tracebuilder.cpp index d8615c95c..b394af115 100644 --- a/hphp/runtime/vm/translator/hopt/tracebuilder.cpp +++ b/hphp/runtime/vm/translator/hopt/tracebuilder.cpp @@ -73,7 +73,7 @@ TraceBuilder::~TraceBuilder() { void TraceBuilder::genSetPropCell(SSATmp* base, int64_t offset, SSATmp* value) { SSATmp* oldVal = gen(LdProp, Type::Cell, base, cns(offset)); gen(StProp, base, cns(offset), value); - genDecRef(oldVal); + gen(DecRef, oldVal); } /** @@ -99,38 +99,6 @@ bool TraceBuilder::isValueAvailable(SSATmp* tmp) const { } } -void TraceBuilder::genDecRef(SSATmp* tmp) { - if (!isRefCounted(tmp)) { - return; - } - - Type type = tmp->type(); - if (type.isBoxed()) { - // we can't really rely on the types held in the boxed values since - // aliasing stores may change them. We conservatively set the type - // of the decref to a boxed cell and rely on later optimizations to - // refine it based on alias analysis. - type = Type::BoxedCell; - } - - // refcount optimization: - // If the decref'ed value is guaranteed to be available after the decref, - // generate DecRefNZ instead of DecRef. - // We could do more accurate availability analysis. For now, we handle - // simple cases: - // 1) LdThis is always available. - // 2) A value stored in a local is always available. - IRInstruction* incRefInst = tmp->inst(); - if (incRefInst->op() == IncRef) { - if (isValueAvailable(incRefInst->getSrc(0))) { - gen(DecRefNZ, tmp); - return; - } - } - - gen(DecRef, tmp); -} - /* * Code generation support for side exits. * There are 3 types of side exits as defined by the ExitType enum: @@ -409,7 +377,7 @@ void TraceBuilder::genDecRefLoc(int id) { if (setNull) { gen(StLoc, LocalId(id), m_fpValue, genDefUninit()); } - genDecRef(val); + gen(DecRef, val); return; } @@ -442,7 +410,7 @@ void TraceBuilder::genBindLoc(uint32_t id, // Silent store: local already contains value being stored // NewValue needs to be decref'ed if (!trackedType.notCounted() && doRefCount) { - genDecRef(prevValue); + gen(DecRef, prevValue); } return; } @@ -458,7 +426,7 @@ void TraceBuilder::genBindLoc(uint32_t id, } gen(genStoreType ? StLoc : StLocNT, LocalId(id), m_fpValue, newValue); if (trackedType.maybeCounted() && doRefCount) { - genDecRef(prevValue); + gen(DecRef, prevValue); } } @@ -509,7 +477,7 @@ SSATmp* TraceBuilder::genStLoc(uint32_t id, SSATmp* retVal = newValue; if (doRefCount) { retVal = gen(IncRef, newValue); - genDecRef(prevValue); + gen(DecRef, prevValue); } return retVal; } @@ -728,7 +696,7 @@ void TraceBuilder::genDecRefStack(Type type, uint32_t stackOff) { } gen(DecRefStack, type, m_spValue, cns(int64_t(stackOff))); } else { - genDecRef(tmp); + gen(DecRef, tmp); } } @@ -1204,11 +1172,35 @@ SSATmp* TraceBuilder::preOptimizeLdCtx(IRInstruction* inst) { return nullptr; } +SSATmp* TraceBuilder::preOptimizeDecRef(IRInstruction* inst) { + /* + * Refcount optimization: + * + * If the decref'ed value is guaranteed to be available after the decref, + * generate DecRefNZ instead of DecRef. + * + * This is safe WRT copy-on-write because all the instructions that + * could cause a COW return a new SSATmp that will replace the + * tracked state that we are using to determine the value is still + * available. I.e. by the time they get to the DecRef we won't see + * it in isValueAvailable anymore and won't convert to DecRefNZ. + */ + auto const srcInst = inst->getSrc(0)->inst(); + if (srcInst->op() == IncRef) { + if (isValueAvailable(srcInst->getSrc(0))) { + inst->setOpcode(DecRefNZ); + } + } + + return nullptr; +} + SSATmp* TraceBuilder::preOptimize(IRInstruction* inst) { #define X(op) case op: return preOptimize##op(inst) switch (inst->op()) { X(LdThis); X(LdCtx); + X(DecRef); default: break; } @@ -1235,6 +1227,7 @@ SSATmp* TraceBuilder::optimizeWork(IRInstruction* inst) { indent(), preOpt->inst()->toString()); return preOpt; } + if (inst->op() == Nop) return nullptr; // copy propagation on inst source operands copyProp(inst); diff --git a/hphp/runtime/vm/translator/hopt/tracebuilder.h b/hphp/runtime/vm/translator/hopt/tracebuilder.h index c7138b576..970e9249f 100644 --- a/hphp/runtime/vm/translator/hopt/tracebuilder.h +++ b/hphp/runtime/vm/translator/hopt/tracebuilder.h @@ -116,8 +116,6 @@ public: SSATmp* genBoxLoc(uint32_t id); void genBindLoc(uint32_t id, SSATmp* ref, bool doRefCount = true); - void genCheckClsCnsDefined(SSATmp* cns, Trace* exitTrace); - SSATmp* genLdCurFuncPtr(); void genGuardLoc(uint32_t id, Type type, Trace* exitTrace); void genAssertStk(uint32_t id, Type type); void genAssertLoc(uint32_t id, @@ -139,7 +137,6 @@ public: SSATmp* genConvToBool(SSATmp* src); SSATmp* genCallBuiltin(SSATmp* func, Type type, uint32_t numArgs, SSATmp** args); - void genDecRef(SSATmp* tmp); void genDecRefStack(Type type, uint32_t stackOff); void genDecRefLoc(int id); void genDecRefThis(); @@ -314,6 +311,7 @@ public: private: SSATmp* preOptimizeLdThis(IRInstruction*); SSATmp* preOptimizeLdCtx(IRInstruction*); + SSATmp* preOptimizeDecRef(IRInstruction*); SSATmp* preOptimize(IRInstruction* inst); SSATmp* optimizeWork(IRInstruction* inst); diff --git a/hphp/runtime/vm/translator/hopt/vectortranslator.cpp b/hphp/runtime/vm/translator/hopt/vectortranslator.cpp index b7a432607..7894de23d 100644 --- a/hphp/runtime/vm/translator/hopt/vectortranslator.cpp +++ b/hphp/runtime/vm/translator/hopt/vectortranslator.cpp @@ -1283,7 +1283,7 @@ void HhbcTranslator::VectorTranslator::emitSetProp() { // The object owns a reference now SSATmp* increffed = gen(IncRef, value); gen(StMem, cellPtr, cns(0), value); - m_tb.genDecRef(oldVal); + gen(DecRef, oldVal); m_result = increffed; return; } @@ -2031,7 +2031,10 @@ void HhbcTranslator::VectorTranslator::emitMPost() { switch (input.location.space) { case Location::Stack: { ++nStack; - m_tb.genDecRef(getInput(i)); + auto input = getInput(i); + if (input->isA(Type::Gen)) { + gen(DecRef, input); + } break; } case Location::Local: