From eab4750cd52daf70f1d923c892d561364d825976 Mon Sep 17 00:00:00 2001 From: Jan Oravec Date: Wed, 2 Oct 2019 17:46:52 -0700 Subject: [PATCH] Remove SpillFrame, merge its memory effects into CallEffects and InlineEnterEffects Summary: Merge SpillFrame's memory effects into CallEffects and InlineEnterEffects, stop emitting SpillFrame and kill it entirely. Reviewed By: paulbiss Differential Revision: D17723645 fbshipit-source-id: 7e2d61f2a42ee98f118c8ea927f38721b908f7b0 --- hphp/doc/ir.specification | 14 +-- hphp/runtime/vm/jit/alias-analysis.cpp | 1 - hphp/runtime/vm/jit/dce.cpp | 12 ++- hphp/runtime/vm/jit/extra-data.h | 16 --- hphp/runtime/vm/jit/frame-state.cpp | 17 +--- hphp/runtime/vm/jit/ir-opcode.cpp | 1 - hphp/runtime/vm/jit/irgen-builtin.cpp | 2 +- hphp/runtime/vm/jit/irgen-call.cpp | 79 +++++---------- hphp/runtime/vm/jit/irgen-inlining.cpp | 24 ++--- hphp/runtime/vm/jit/irgen.h | 3 +- hphp/runtime/vm/jit/irlower-act-rec.cpp | 4 - hphp/runtime/vm/jit/licm.cpp | 8 +- hphp/runtime/vm/jit/load-elim.cpp | 5 +- hphp/runtime/vm/jit/memory-effects.cpp | 23 +---- hphp/runtime/vm/jit/memory-effects.h | 16 --- hphp/runtime/vm/jit/refcount-opts.cpp | 38 ++----- hphp/runtime/vm/jit/store-elim.cpp | 170 ++++---------------------------- 17 files changed, 82 insertions(+), 351 deletions(-) diff --git a/hphp/doc/ir.specification b/hphp/doc/ir.specification index 248bceaa48c..01702e05c3f 100644 --- a/hphp/doc/ir.specification +++ b/hphp/doc/ir.specification @@ -1697,14 +1697,6 @@ To string conversions: 9. Call & Return -| SpillFrame, -| ND, -| S(StkPtr), -| NF - - Defines the fields for an activation record and writes them to the stack - pointed to by S0, at `offset'. - | BeginInlining, ND, S(StkPtr), NF Marks the start of an inlined function whose stack resides offset cells below @@ -1753,9 +1745,9 @@ To string conversions: | InlineReturnNoFrame, ND, NA, NF Mark the end of an inlined function for which no DefInlineFP was required. The - primary purpose of this instruction is to mark the result of a SpillFrame as - dead. InlineFrameStart is the caller FP-relative offset of the start of the - callee frame. Everything below InlineFrameStart is dead. + primary purpose of this instruction is to hint the optimization passes that + after return everything below InlineFrameStart is dead. InlineFrameStart is + the caller FP-relative offset of the start of the callee frame. | CallUnpack, | DCall, diff --git a/hphp/runtime/vm/jit/alias-analysis.cpp b/hphp/runtime/vm/jit/alias-analysis.cpp index f554117fe2e..82c645b9b2d 100644 --- a/hphp/runtime/vm/jit/alias-analysis.cpp +++ b/hphp/runtime/vm/jit/alias-analysis.cpp @@ -61,7 +61,6 @@ void visit_locations(const BlockList& blocks, Visit visit) { visit(x.kills); }, [&] (PureLoad x) { visit(x.src); }, [&] (PureStore x) { visit(x.dst); }, - [&] (PureSpillFrame x) { visit(x.stk); }, [&] (ExitEffects x) { visit(x.live); visit(x.kills); }, [&] (InlineEnterEffects x) { visit(x.inlFrame); visit(x.inlStack); diff --git a/hphp/runtime/vm/jit/dce.cpp b/hphp/runtime/vm/jit/dce.cpp index d60be2ddfd6..6524698c377 100644 --- a/hphp/runtime/vm/jit/dce.cpp +++ b/hphp/runtime/vm/jit/dce.cpp @@ -369,7 +369,6 @@ bool canDCE(IRInstruction* inst) { case AKExistsObj: case StStk: case StOutValue: - case SpillFrame: case CheckType: case CheckNullptr: case CheckTypeMem: @@ -1122,8 +1121,8 @@ void performActRecFixups(const BlockList& blocks, /* * Look for InlineReturn instructions that are the only "non-weak" use - * of a DefInlineFP. In this case we can kill both, which may allow - * removing a SpillFrame as well. + * of a DefInlineFP. In this case we can kill both, avoiding the ActRec + * spill. * * Prior to calling this routine, `uses' should contain the direct * (non-transitive) use counts of each DefInlineFP instruction. If @@ -1281,9 +1280,12 @@ void processCatchBlock(IRUnit& unit, DceState& state, Block* block, }, [&] (PureLoad x) { return process_stack(x.src); }, [&] (PureStore x) { return do_store(x.dst, &*inst); }, - [&] (PureSpillFrame x) { return process_stack(x.stk); }, [&] (ExitEffects x) { return process_stack(x.live); }, - [&] (InlineEnterEffects x) { return process_stack(x.inlStack); }, + [&] (InlineEnterEffects x) { + return + process_stack(x.inlStack) || + process_stack(x.actrec); + }, [&] (InlineExitEffects x) { return process_stack(x.inlStack); } ); } diff --git a/hphp/runtime/vm/jit/extra-data.h b/hphp/runtime/vm/jit/extra-data.h index 5e29516998e..812311caad3 100644 --- a/hphp/runtime/vm/jit/extra-data.h +++ b/hphp/runtime/vm/jit/extra-data.h @@ -704,21 +704,6 @@ struct ReqRetranslateData : IRExtraData { }; /* - * Compile-time metadata about an ActRec allocation. - */ -struct ActRecInfo : IRExtraData { - explicit ActRecInfo(IRSPRelOffset spOffset) - : spOffset(spOffset) - {} - - std::string show() const { - return folly::sformat("{}", spOffset.offset); - } - - IRSPRelOffset spOffset; -}; - -/* * DefInlineFP is present when we need to create a frame for inlining. This * instruction also carries some metadata used by IRBuilder to track state * during an inlined call. @@ -1633,7 +1618,6 @@ X(InitObjMemoSlots, ClassData); X(InstanceOfIfaceVtable, InstanceOfIfaceVtableData); X(ResolveTypeStruct, ResolveTypeStructData); X(ExtendsClass, ExtendsClassData); -X(SpillFrame, ActRecInfo); X(CheckStk, IRSPRelOffsetData); X(HintStkInner, IRSPRelOffsetData); X(StStk, IRSPRelOffsetData); diff --git a/hphp/runtime/vm/jit/frame-state.cpp b/hphp/runtime/vm/jit/frame-state.cpp index 05551e7023e..de05a28d248 100644 --- a/hphp/runtime/vm/jit/frame-state.cpp +++ b/hphp/runtime/vm/jit/frame-state.cpp @@ -547,16 +547,6 @@ void FrameStateMgr::update(const IRInstruction* inst) { ); break; - case SpillFrame: { - assertx(inst->is(SpillFrame)); - auto const extra = inst->extra(); - - for (auto i = uint32_t{0}; i < kNumActRecCells; ++i) { - setValue(stk(extra->spOffset + i), nullptr); - } - break; - } - case InterpOne: case InterpOneCF: { auto const& extra = *inst->extra(); @@ -859,7 +849,6 @@ void FrameStateMgr::updateMBase(const IRInstruction* inst) { auto const effects = memory_effects(*inst); match(effects, [&](GeneralEffects m) { handle_stores(m.stores); }, [&](PureStore m) { handle_stores(m.dst); }, - [&](PureSpillFrame m) { handle_stores(m.stk); }, [&](CallEffects x) { handle_stores(x.kills); handle_stores(x.inputs); @@ -868,7 +857,8 @@ void FrameStateMgr::updateMBase(const IRInstruction* inst) { }, [&](PureLoad /*m*/) {}, [&](ReturnEffects) {}, [&](ExitEffects) {}, [&](IrrelevantEffects) {}, - [&](InlineEnterEffects) {}, [&](InlineExitEffects) {}, + [&](InlineEnterEffects x) { handle_stores(x.actrec); }, + [&](InlineExitEffects) {}, [&](UnknownEffects) { pessimize_mbase(); }); } @@ -1127,6 +1117,9 @@ void FrameStateMgr::trackDefInlineFP(const IRInstruction* inst) { * Push a new state for the inlined callee; saving the state we'll need to * pop on return. */ + for (auto i = uint32_t{0}; i < kNumActRecCells; ++i) { + setValue(stk(extra->spOffset + i), nullptr); + } cur().bcSPOff = savedSPOff; auto stateCopy = m_stack.back(); m_stack.emplace_back(std::move(stateCopy)); diff --git a/hphp/runtime/vm/jit/ir-opcode.cpp b/hphp/runtime/vm/jit/ir-opcode.cpp index f962e998f35..ec21fb54a72 100644 --- a/hphp/runtime/vm/jit/ir-opcode.cpp +++ b/hphp/runtime/vm/jit/ir-opcode.cpp @@ -960,7 +960,6 @@ bool opcodeMayRaise(Opcode opc) { case Select: case Shl: case Shr: - case SpillFrame: case Sqrt: case StArResumeAddr: case StClosureArg: diff --git a/hphp/runtime/vm/jit/irgen-builtin.cpp b/hphp/runtime/vm/jit/irgen-builtin.cpp index d5d7aa757e6..c62cd1155ca 100644 --- a/hphp/runtime/vm/jit/irgen-builtin.cpp +++ b/hphp/runtime/vm/jit/irgen-builtin.cpp @@ -1853,7 +1853,7 @@ SSATmp* builtinCall(IRGS& env, /* * When we're inlining a NativeImpl opcode, we know this is the only opcode in * the callee method body aside from AssertRATs (bytecode invariant). So in - * order to make sure we can eliminate the SpillFrame, we do the CallBuiltin + * order to make sure we can eliminate the DefInlineFP, we do the CallBuiltin * instruction after we've left the inlined frame. * * We may need to pass some arguments to the builtin through the stack (e.g. if diff --git a/hphp/runtime/vm/jit/irgen-call.cpp b/hphp/runtime/vm/jit/irgen-call.cpp index 5860667b18a..8ba8417e767 100644 --- a/hphp/runtime/vm/jit/irgen-call.cpp +++ b/hphp/runtime/vm/jit/irgen-call.cpp @@ -178,23 +178,6 @@ void emitCallerRxChecksUnknown(IRGS& env, SSATmp* callee) { ////////////////////////////////////////////////////////////////////// -void fsetActRec( - IRGS& env, - const FCallArgs& fca -) { - auto const arOffset = - offsetFromIRSP(env, BCSPRelOffset{static_cast(fca.numInputs())}); - - gen( - env, - SpillFrame, - ActRecInfo { arOffset }, - sp(env) - ); -} - -////////////////////////////////////////////////////////////////////// - void callUnpack(IRGS& env, SSATmp* callee, const FCallArgs& fca, SSATmp* objOrClass, bool dynamicCall, bool unlikely) { assertx(fca.hasUnpack()); @@ -291,37 +274,6 @@ void handleCallReturn(IRGS& env, const Func* callee, const FCallArgs& fca, ); } -void callKnown(IRGS& env, const Func* callee, const FCallArgs& fca, - SSATmp* objOrClass, bool dynamicCall) { - assertx(callee); - if (fca.hasUnpack()) { - return callUnpack(env, cns(env, callee), fca, objOrClass, dynamicCall, - false /* unlikely */); - } - - auto const asyncEagerReturn = - fca.asyncEagerOffset != kInvalidOffset && - callee->supportsAsyncEagerReturn(); - auto const retVal = callImpl(env, cns(env, callee), fca, objOrClass, - dynamicCall, asyncEagerReturn); - handleCallReturn(env, callee, fca, retVal, asyncEagerReturn, - false /* unlikely */); -} - -void callUnknown(IRGS& env, SSATmp* callee, const FCallArgs& fca, - SSATmp* objOrClass, bool dynamicCall, bool unlikely) { - assertx(!callee->hasConstVal() || env.formingRegion); - if (fca.hasUnpack()) { - return callUnpack(env, callee, fca, objOrClass, dynamicCall, unlikely); - } - - // Okay to request async eager return even if it is not supported. - auto const asyncEagerReturn = fca.asyncEagerOffset != kInvalidOffset; - auto const retVal = callImpl(env, callee, fca, objOrClass, dynamicCall, - asyncEagerReturn); - handleCallReturn(env, nullptr, fca, retVal, asyncEagerReturn, unlikely); -} - ////////////////////////////////////////////////////////////////////// /* @@ -410,13 +362,22 @@ void prepareAndCallKnown(IRGS& env, const Func* callee, const FCallArgs& fca, if (inlined) return; } - fsetActRec(env, fca); - - // We just wrote to the stack, make sure Call opcode can set up its Catch. + // We may have updated the stack, make sure Call opcode can set up its Catch. updateMarker(env); env.irb->exceptionStackBoundary(); - callKnown(env, callee, fca, objOrClass, dynamicCall); + if (fca.hasUnpack()) { + return callUnpack(env, cns(env, callee), fca, objOrClass, dynamicCall, + false /* unlikely */); + } + + auto const asyncEagerReturn = + fca.asyncEagerOffset != kInvalidOffset && + callee->supportsAsyncEagerReturn(); + auto const retVal = callImpl(env, cns(env, callee), fca, objOrClass, + dynamicCall, asyncEagerReturn); + handleCallReturn(env, callee, fca, retVal, asyncEagerReturn, + false /* unlikely */); } void prepareAndCallUnknown(IRGS& env, SSATmp* callee, const FCallArgs& fca, @@ -435,13 +396,19 @@ void prepareAndCallUnknown(IRGS& env, SSATmp* callee, const FCallArgs& fca, } emitCallerRxChecksUnknown(env, callee); - fsetActRec(env, fca); - - // We just wrote to the stack, make sure Call opcode can set up its Catch. + // We may have updated the stack, make sure Call opcode can set up its Catch. updateMarker(env); env.irb->exceptionStackBoundary(); - callUnknown(env, callee, fca, objOrClass, dynamicCall, unlikely); + if (fca.hasUnpack()) { + return callUnpack(env, callee, fca, objOrClass, dynamicCall, unlikely); + } + + // Okay to request async eager return even if it is not supported. + auto const asyncEagerReturn = fca.asyncEagerOffset != kInvalidOffset; + auto const retVal = callImpl(env, callee, fca, objOrClass, dynamicCall, + asyncEagerReturn); + handleCallReturn(env, nullptr, fca, retVal, asyncEagerReturn, unlikely); } void prepareAndCallProfiled(IRGS& env, SSATmp* callee, const FCallArgs& fca, diff --git a/hphp/runtime/vm/jit/irgen-inlining.cpp b/hphp/runtime/vm/jit/irgen-inlining.cpp index e9dcb0a8d3c..574df1adf62 100644 --- a/hphp/runtime/vm/jit/irgen-inlining.cpp +++ b/hphp/runtime/vm/jit/irgen-inlining.cpp @@ -20,14 +20,10 @@ IR function inliner Inlining functions at the IR level can be complex, particularly when dealing with async callees. All inlined regions are setup the same way, though later optimization passes may attempt to elide elements of this inititalization, -particularly the SpillFrame and DefinlineFP. Below is an annotated version of -this setup. +particularly the DefinlineFP. Below is an annotated version of this setup. StStk ... # Stores initiated for parameters as if this was an ordinary call. - SpillFrame # Standard spill of an ActRec prior to making a call. The StoreElim - # pass will attempt to elide these spills. - # # This is where this module starts to emit instructions, the stores and spills # are generated by passing arguments and emitting FCall* instructions, prior @@ -42,9 +38,10 @@ this setup. # frame. Most importantly it marks any values on the callee # stack from before the call as dead. - DefInlineFP # This instruction sets up the inlined frame, redefining the - # fp SSATmp to refer to the callee frame. In most cases the DCE - # or partial-DCE passes are able to elide this instruction. + DefInlineFP # This instruction spills ActRec and sets up the inlined frame, + # redefining the fp SSATmp to refer to the callee frame. In most + # cases the DCE or partial-DCE passes are able to elide this + # instruction. StLoc ... # At this point the arguments are stored back to the frame # locations for their corresponding locals in the callee. N.B. @@ -148,7 +145,6 @@ Outer: | Inner: | v +-------------------+ -| SpillFrame | | ... | | BeginInlining | | DefInlineFP | @@ -265,13 +261,6 @@ void beginInlining(IRGS& env, return ctx; }(); - // The VM stack-pointer is conceptually pointing to the last - // parameter, so we need to add numInputs to get to the ActRec - IRSPRelOffset calleeAROff = spOffBCFromIRSP(env) + fca.numInputs(); - - auto const arInfo = ActRecInfo { calleeAROff }; - gen(env, SpillFrame, arInfo, sp(env)); - auto const generics = [&]() -> SSATmp* { if (!fca.hasGenerics()) return nullptr; if (target->hasReifiedGenerics()) return popC(env); @@ -291,6 +280,9 @@ void beginInlining(IRGS& env, // will be as if the arguments don't exist on the stack (even though // they do). + // The top of the stack now points to the space for ActRec. + IRSPRelOffset calleeAROff = spOffBCFromIRSP(env); + gen( env, BeginInlining, diff --git a/hphp/runtime/vm/jit/irgen.h b/hphp/runtime/vm/jit/irgen.h index e65432a9c79..341e7757698 100644 --- a/hphp/runtime/vm/jit/irgen.h +++ b/hphp/runtime/vm/jit/irgen.h @@ -210,9 +210,8 @@ uint16_t inlineDepth(const IRGS& env); * // ... normal stuff happens ... * // ... probably some StStks due to argument expressions * // FCall*: - * SpillFrame sp, ... * BeginInlining sp - * fp2 = DefInlineFP sp + * fp2 = DefInlineFP sp fp ctx * * // ... callee body ... * diff --git a/hphp/runtime/vm/jit/irlower-act-rec.cpp b/hphp/runtime/vm/jit/irlower-act-rec.cpp index 115a6332837..342faccfbfc 100644 --- a/hphp/runtime/vm/jit/irlower-act-rec.cpp +++ b/hphp/runtime/vm/jit/irlower-act-rec.cpp @@ -64,10 +64,6 @@ TRACE_SET_MOD(irlower); /////////////////////////////////////////////////////////////////////////////// -void cgSpillFrame(IRLS& env, const IRInstruction* inst) {} - -/////////////////////////////////////////////////////////////////////////////// - void cgIsFunReifiedGenericsMatched(IRLS& env, const IRInstruction* inst) { auto const dst = dstLoc(env, inst, 0).reg(); auto const callFlags = srcLoc(env, inst, 0).reg(); diff --git a/hphp/runtime/vm/jit/licm.cpp b/hphp/runtime/vm/jit/licm.cpp index 0658ca2fd23..b48d22d4538 100644 --- a/hphp/runtime/vm/jit/licm.cpp +++ b/hphp/runtime/vm/jit/licm.cpp @@ -326,9 +326,9 @@ void analyze_block(LoopEnv& env, Block* blk) { [&] (UnknownEffects) { kill(AUnknown); }, [&] (CallEffects x) { env.contains_call = true; - kill(AHeapAny); }, + kill(AHeapAny); + kill(x.actrec); }, [&] (PureStore x) { kill(x.dst); }, - [&] (PureSpillFrame x) { kill(x.stk); }, [&] (GeneralEffects x) { kill(x.stores); @@ -353,7 +353,8 @@ void analyze_block(LoopEnv& env, Block* blk) { [&] (IrrelevantEffects) {}, [&] (InlineEnterEffects x) { kill(x.inlFrame); - kill(x.inlStack); }, + kill(x.inlStack); + kill(x.actrec); }, [&] (InlineExitEffects x) { kill(x.inlFrame); kill(x.inlMeta); }, @@ -455,7 +456,6 @@ bool try_invariant_memory(LoopEnv& env, [&] (UnknownEffects) { return false; }, [&] (CallEffects) { return false; }, [&] (PureStore) { return false; }, - [&] (PureSpillFrame) { return false; }, [&] (IrrelevantEffects) { return false; }, [&] (ReturnEffects) { return false; }, [&] (ExitEffects) { return false; }, diff --git a/hphp/runtime/vm/jit/load-elim.cpp b/hphp/runtime/vm/jit/load-elim.cpp index 1b79070855d..be9d7ef22b6 100644 --- a/hphp/runtime/vm/jit/load-elim.cpp +++ b/hphp/runtime/vm/jit/load-elim.cpp @@ -574,12 +574,11 @@ Flags analyze_inst(Local& env, const IRInstruction& inst) { [&] (ReturnEffects) {}, [&] (PureStore m) { store(env, m.dst, m.value); }, - [&] (PureSpillFrame m) { store(env, m.stk, nullptr); }, - [&] (PureLoad m) { flags = load(env, inst, m.src); }, [&] (InlineEnterEffects m) { store(env, m.inlFrame, nullptr); - store(env, m.inlStack, nullptr); }, + store(env, m.inlStack, nullptr); + store(env, m.actrec, nullptr); }, [&] (InlineExitEffects m) { store(env, m.inlFrame, nullptr); store(env, m.inlMeta, nullptr); }, [&] (GeneralEffects m) { flags = handle_general_effects(env, inst, m); }, diff --git a/hphp/runtime/vm/jit/memory-effects.cpp b/hphp/runtime/vm/jit/memory-effects.cpp index 53a36438b92..cb08a0bfec4 100644 --- a/hphp/runtime/vm/jit/memory-effects.cpp +++ b/hphp/runtime/vm/jit/memory-effects.cpp @@ -728,9 +728,6 @@ MemEffects memory_effects_impl(const IRInstruction& inst) { * first instruction in the inlined call and has no effect serving only as * a marker to memory effects that the stack cells within the inlined call * are now dead. - * - * Unlike DefInlineFP it does not load the SpillFrame, which we hope to push - * off the main trace or elide entirely. */ case BeginInlining: { /* @@ -766,7 +763,7 @@ MemEffects memory_effects_impl(const IRInstruction& inst) { case SyncReturnBC: { auto const spOffset = inst.extra()->spOffset; auto const arStack = actrec(inst.src(0), spOffset); - // This instruction doesn't actually load but SpillFrame cannot be pushed + // This instruction doesn't actually load but DefInlineFP cannot be pushed // past it return may_load_store(arStack, arStack); } @@ -1589,12 +1586,6 @@ MemEffects memory_effects_impl(const IRInstruction& inst) { case LdOutAddr: return IrrelevantEffects{}; - case SpillFrame: - { - auto const spOffset = inst.extra()->spOffset; - return PureSpillFrame { actrec(inst.src(0), spOffset) }; - } - case CheckStk: return may_load_store( AStack { inst.src(0), inst.extra()->offset, 1 }, @@ -2230,7 +2221,6 @@ DEBUG_ONLY bool check_effects(const IRInstruction& inst, MemEffects me) { }, [&] (PureLoad x) { check(x.src); }, [&] (PureStore x) { check(x.dst); }, - [&] (PureSpillFrame x) { check(x.stk); }, [&] (ExitEffects x) { check(x.live); check(x.kills); }, [&] (IrrelevantEffects) {}, [&] (UnknownEffects) {}, @@ -2292,7 +2282,6 @@ MemEffects memory_effects(const IRInstruction& inst) { [&] (UnknownEffects x) { return x; }, [&] (PureLoad) { return fail(); }, [&] (PureStore) { return fail(); }, - [&] (PureSpillFrame) { return fail(); }, [&] (ExitEffects) { return fail(); }, [&] (InlineExitEffects) { return fail(); }, [&] (InlineEnterEffects) { return fail(); }, @@ -2328,11 +2317,6 @@ MemEffects canonicalize(MemEffects me) { [&] (PureStore x) -> R { return PureStore { canonicalize(x.dst), x.value, x.dep }; }, - [&] (PureSpillFrame x) -> R { - return PureSpillFrame { - canonicalize(x.stk) - }; - }, [&] (ExitEffects x) -> R { return ExitEffects { canonicalize(x.live), canonicalize(x.kills) }; }, @@ -2407,11 +2391,6 @@ std::string show(MemEffects effects) { show(x.locals) ); }, - [&] (PureSpillFrame x) { - return sformat("stFrame({})", - show(x.stk) - ); - }, [&] (PureLoad x) { return sformat("ld({})", show(x.src)); }, [&] (PureStore x) { return sformat("st({})", show(x.dst)); }, [&] (ReturnEffects x) { return sformat("return({})", show(x.kills)); }, diff --git a/hphp/runtime/vm/jit/memory-effects.h b/hphp/runtime/vm/jit/memory-effects.h index f91feca957c..e705b151f47 100644 --- a/hphp/runtime/vm/jit/memory-effects.h +++ b/hphp/runtime/vm/jit/memory-effects.h @@ -102,19 +102,6 @@ struct PureLoad { AliasClass src; }; struct PureStore { AliasClass dst; SSATmp* value; SSATmp* dep; }; /* - * Spilling pre-live ActRecs are somewhat unusual, but effectively still just - * pure stores. They store to a range of stack slots, and don't store a PHP - * value, so they get their own branch of the union. - * - * The `stk' class is the entire stack range the instruction stores to. - * - * The `stk' range should be interpreted as an exact AliasClass, not an upper - * bound: it is guaranteed to be kNumActRecCells in size---no bigger than the - * actual range of stack slots a SpillFrame instruction affects. - */ -struct PureSpillFrame { AliasClass stk; }; - -/* * Calls are somewhat special enough that they get a top-level effect. * * The `kills' set are locations that cannot be read by this instruction unless @@ -124,8 +111,6 @@ struct PureSpillFrame { AliasClass stk; }; * The `inputs' set contains stack locations the call will read as arguments. * * The `actrec' set contains stack locations the call will write ActRec to. - * As of now, they are considered to be reads, because dummy SpillFrame opcode - * pretends to write them. * * The `outputs' set contains stack locations the call will write inout * variables to. @@ -209,7 +194,6 @@ struct UnknownEffects {}; using MemEffects = boost::variant< GeneralEffects , PureLoad , PureStore - , PureSpillFrame , CallEffects , ReturnEffects , ExitEffects diff --git a/hphp/runtime/vm/jit/refcount-opts.cpp b/hphp/runtime/vm/jit/refcount-opts.cpp index 2d56a3daa26..35e0d17308c 100644 --- a/hphp/runtime/vm/jit/refcount-opts.cpp +++ b/hphp/runtime/vm/jit/refcount-opts.cpp @@ -300,13 +300,12 @@ everything alone. -- Effects of Pure Stores on Memory Support -- There are two main kinds of stores from the perspective of this module. There -are lowered stores (PureStore and PureSpillFrame) that happen within our IR -compilation unit, and don't imply reference count manipulation, and there are -stores that happen with "hhbc semantics" outside of the visibility of this -compilation unit, which imply decreffing the value that used to live in a -memory location as it's replaced with a new one. This module needs some -understanding of both types, and both of these types of stores affect memory -support, but in different ways. +are lowered stores (PureStore) that happen within our IR compilation unit, and +don't imply reference count manipulation, and there are stores that happen with +"hhbc semantics" outside of the visibility of this compilation unit, which +imply decreffing the value that used to live in a memory location as it's +replaced with a new one. This module needs some understanding of both types, +and both of these types of stores affect memory support, but in different ways. For any instruction that may do non-lowered stores outside of our unit ("stores with hhbc semantics"), if the location(s) it may be storing to could be @@ -992,13 +991,6 @@ void mrinfo_step_impl(Env& env, */ [&](PureLoad) {}, - /* - * Since there's no semantically correct way to do PureLoads from the - * locations in a PureSpillFrame unless something must have stored over - * them again first, we don't need to kill anything here. - */ - [&](PureSpillFrame /*x*/) {}, - [&](CallEffects /*x*/) { /* * Because PHP callees can side-exit (or for that matter throw from their @@ -1319,7 +1311,6 @@ bool irrelevant_inst(const IRInstruction& inst) { // object reference counts. [&] (PureLoad) { return true; }, [&] (PureStore) { return true; }, - [&] (PureSpillFrame) { return true; }, [&] (IrrelevantEffects) { return true; }, [&] (InlineEnterEffects) { return true; }, [&] (InlineExitEffects) { return true; }, @@ -2049,17 +2040,6 @@ void pure_store(Env& env, if (tmp) create_store_support(env, state, dst, tmp, add_node); } -void pure_spill_frame(Env& env, - RCState& state, - PureSpillFrame psf) { - /* - * First, the effects of PureStores on memory support. A SpillFrame will - * store over kNumActRecCells stack slots, and just like normal PureStores we - * can drop any support bits for them without reducing their lower bounds. - */ - drop_support_bits(env, state, env.ainfo.expand(psf.stk)); -} - void inline_enter_effects(Env& env, RCState& state, InlineEnterEffects& ief, @@ -2140,11 +2120,7 @@ void analyze_mem_effects(Env& env, handle_call(env, state, inst, e, add_node); }, [&] (PureStore x) { pure_store(env, state, x.dst, x.value, add_node); }, - [&] (PureLoad x) { pure_load(env, state, x.src, inst.dst(), add_node); }, - - [&] (PureSpillFrame x) { - pure_spill_frame(env, state, x); - } + [&] (PureLoad x) { pure_load(env, state, x.src, inst.dst(), add_node); } ); } diff --git a/hphp/runtime/vm/jit/store-elim.cpp b/hphp/runtime/vm/jit/store-elim.cpp index 185d502b692..23e53655b23 100644 --- a/hphp/runtime/vm/jit/store-elim.cpp +++ b/hphp/runtime/vm/jit/store-elim.cpp @@ -158,15 +158,14 @@ using PostOrderId = uint32_t; - Phi..Phi+kMaxTrackedAlocs, in which case it holds a pointer to the Block where the phi would have to be inserted, or - Bad which means that although stores are available on all - paths to this point they're not compatible in some way - (usually SpillFrame vs StStk). + paths to this point they're not compatible in some way. - Pending and Processed are used while building phis to handle cycles of phis. For the Phi case, we just need to ensure that we can differentiate Phis in the same block for different ALocations; this is just used - for the hash() and same() methods for the spillFrameMap. + for the same() method for the combine_ts(). */ struct TrackedStore { enum Kind : int16_t { @@ -238,9 +237,6 @@ struct TrackedStore { const TrackedStore& b) { return a.kind() >= b.kind(); } - size_t hash() const { - return m_ptr.tag() + intptr_t(m_ptr.ptr()); - } std::string toString() const { if (isUnseen()) return "Unseen"; if (isBad()) return "Bad"; @@ -289,18 +285,6 @@ struct StoreKeyHashCmp { using MovableStoreMap = hphp_hash_map; -struct TrackedStoreHashCmp { - size_t operator()(const TrackedStore& ts) const { - return ts.hash(); - } - bool operator()(const TrackedStore& ts1, const TrackedStore& ts2) const { - return ts1.same(ts2); - } -}; - -using SpillFrameMap = hphp_hash_map; - struct BlockState { PostOrderId id; ALocBits antIn; @@ -341,9 +325,6 @@ struct Global { */ hphp_fast_map ssa2Aloc; - // We keep an entry for each tracked SpillFrame in this map, so we - // can check its ALocBits - SpillFrameMap spillFrameMap; MovableStoreMap trackedStoreMap; // Block states are indexed by block->id(). These are only meaningful after // we do dataflow. @@ -406,30 +387,11 @@ bool srcsCanSpanCall(const IRInstruction& inst) { return true; } -const ALocBits* findSpillFrame(Global& genv, const TrackedStore& ts) { - auto it = genv.spillFrameMap.find(ts); - if (it == genv.spillFrameMap.end()) return nullptr; - return &it->second; -} - void set_movable_store(Local& env, uint32_t bit, IRInstruction& inst) { env.global.trackedStoreMap[ StoreKey { inst.block(), StoreKey::Out, bit }].set(&inst); } -void set_movable_spill_frame(Local& env, const ALocBits& bits, - IRInstruction& inst) { - TrackedStore ts { &inst }; - bitset_for_each_set( - bits, - [&](uint32_t i) { - env.global.trackedStoreMap[ - StoreKey { inst.block(), StoreKey::Out, i }] = ts; - } - ); - env.global.spillFrameMap[ts] = bits; -} - bool isDead(Local& env, int bit) { return env.antLoc[bit]; } @@ -455,14 +417,6 @@ bool removeDead(Local& env, IRInstruction& inst, bool trash) { inst.src(0) ); break; - case SpillFrame: - dbgInst = env.global.unit.gen( - DbgTrashFrame, - inst.bcctx(), - IRSPRelOffsetData { inst.extra()->spOffset }, - inst.src(0) - ); - break; case StMem: dbgInst = env.global.unit.gen(DbgTrashMem, inst.bcctx(), inst.src(0)); break; @@ -659,21 +613,23 @@ void visit(Local& env, IRInstruction& inst) { [&] (CallEffects l) { env.containsCall = true; - mayStore(env, l.outputs); - if (auto bit = pure_store_bit(env, l.outputs)) { - mustStore(env, *bit); - } else { - auto const it = - env.global.ainfo.stack_ranges.find(canonicalize(l.outputs)); - if (it != end(env.global.ainfo.stack_ranges)) { - mustStoreSet(env, it->second); + auto const store = [&](AliasClass cls) { + mayStore(env, cls); + if (auto bit = pure_store_bit(env, cls)) { + mustStore(env, *bit); + } else { + auto const it = env.global.ainfo.stack_ranges.find(canonicalize(cls)); + if (it != end(env.global.ainfo.stack_ranges)) { + mustStoreSet(env, it->second); + } } - } + }; + store(l.outputs); load(env, AHeapAny); load(env, l.locals); load(env, l.inputs); - load(env, l.actrec); + store(l.actrec); kill(env, l.kills); }, @@ -716,31 +672,6 @@ void visit(Local& env, IRInstruction& inst) { return; } mayStore(env, l.dst); - }, - - [&] (PureSpillFrame l) { - auto const it = env.global.ainfo.stack_ranges.find(canonicalize(l.stk)); - if (it != end(env.global.ainfo.stack_ranges)) { - // If all the bits corresponding to the stack range are dead, we can - // eliminate this instruction. We can also count all of them as - // redefined. - if (isDeadSet(env, it->second)) { - if (removeDead(env, inst, true)) return; - } else { - auto avlLoc = it->second & ~(env.antLoc | env.mayLoad | - env.mayStore); - if (avlLoc == it->second && - (!env.containsCall || - srcsCanSpanCall(inst))) { - set_movable_spill_frame(env, avlLoc, inst); - env.avlLoc |= avlLoc; - } - } - mayStore(env, l.stk); - mustStoreSet(env, it->second); - return; - } - mayStore(env, l.stk); } ); } @@ -840,8 +771,7 @@ void find_all_stores(Global& genv, Block* blk, uint32_t id, } IRInstruction* resolve_ts(Global& genv, Block* blk, - StoreKey::Where w, uint32_t id, - const ALocBits** sfp = nullptr); + StoreKey::Where w, uint32_t id); void resolve_cycle(Global& genv, TrackedStore& ts, Block* blk, uint32_t id) { genv.needsReflow = true; @@ -997,11 +927,8 @@ IRInstruction* resolve_flat(Global& genv, Block* blk, uint32_t id, } IRInstruction* resolve_ts(Global& genv, Block* blk, - StoreKey::Where w, uint32_t id, - const ALocBits** sfp) { + StoreKey::Where w, uint32_t id) { auto& ts = genv.trackedStoreMap[StoreKey { blk, w, id }]; - auto sf = findSpillFrame(genv, ts); - if (sfp) *sfp = sf; ITRACE(7, "resolve_ts: B{}:{} store:{} ts:{}\n", blk->id(), show(w), id, show(ts)); @@ -1032,7 +959,6 @@ IRInstruction* resolve_ts(Global& genv, Block* blk, } auto const rep = ts.instruction(); - if (sf) genv.spillFrameMap[ts] = *sf; ITRACE(7, "-> {} (resolve_ts B{}, store {})\n", rep->toString(), blk->id(), id); return rep; @@ -1073,12 +999,7 @@ void optimize_block_pre(Global& genv, Block* block, insertBits, [&](size_t i){ if (!store_insert.go()) return; - const ALocBits* sf = nullptr; - auto const cinst = resolve_ts(genv, block, StoreKey::In, i, &sf); - if (sf) { - always_assert((state.ppIn & *sf) == *sf); - insertBits &= ~*sf; - } + auto const cinst = resolve_ts(genv, block, StoreKey::In, i); FTRACE(1, " Inserting store {}: {}\n", i, cinst->toString()); auto const inst = genv.unit.clone(cinst); block->prepend(inst); @@ -1265,27 +1186,8 @@ TrackedStore combine_ts(Global& genv, uint32_t id, return Compat::Same; }; - auto sf1 = findSpillFrame(genv, s1); - auto sf2 = findSpillFrame(genv, s2); - - // t7621182 Don't merge different spillframes for now, - // because code-gen doesn't handle phi'ing an Obj and a - // Cls. - if (sf1 || sf2) { - // we already know they aren't the same - return TrackedStore::BadVal(); - } - - if (!sf1 != !sf2 || (sf1 && *sf1 != *sf2)) { - // They need to both be spill frames affecting - // the same addresses, or both not be spill frames. - return TrackedStore::BadVal(); - } - auto trackedBlock = [&]() { - auto ret = TrackedStore(succ, id); - if (sf1) genv.spillFrameMap[ret] = *sf1; - return ret; + return TrackedStore(succ, id); }; if (s1.block() || s2.block()) { @@ -1359,10 +1261,6 @@ void compute_available_stores( if (tsOut.isBad()) { state.ppOut[i] = false; - } else if (auto sf = findSpillFrame(genv, tsIn)) { - if ((propagate & *sf) != *sf) { - state.ppOut &= ~*sf; - } } } ); @@ -1408,26 +1306,13 @@ void compute_available_stores( genv.trackedStoreMap[StoreKey { blk, StoreKey::Out, i }]; auto& tsSucc = genv.trackedStoreMap[StoreKey { succ, StoreKey::In, i }]; - auto sf = findSpillFrame(genv, ts); - if (sf) { - if ((succState.ppIn & *sf) != *sf) { - changed = true; - tsSucc.setBad(); - succState.ppIn &= ~*sf; - return; - } - } auto tsNew = succ->numPreds() == 1 ? ts : recompute_ts(genv, i, succ); if (!tsNew.same(tsSucc)) { changed = true; assertx(tsNew >= tsSucc); tsSucc = tsNew; if (tsSucc.isBad()) { - if (sf) { - succState.ppIn &= ~*sf; - } else { - succState.ppIn[i] = false; - } + succState.ppIn[i] = false; } } } @@ -1485,22 +1370,7 @@ void compute_placement_possible( restrictBits &= succState.ppIn | succState.antIn; }); - auto bitsToClear = state.ppOut & ~restrictBits; - // ensure that if we clear any spill frame bits, we clear - // them all. - if (bitsToClear.any()) { - bitset_for_each_set( - bitsToClear, - [&](uint32_t i) { - auto const ts = - genv.trackedStoreMap[StoreKey { blk, StoreKey::Out, i }]; - if (auto sf = findSpillFrame(genv, ts)) { - restrictBits &= ~*sf; - } - } - ); - state.ppOut &= restrictBits; - } + state.ppOut &= restrictBits; trace(); return true; }, -- 2.11.4.GIT