Simplify BeginInlining sequence

Summary:
- We always side-exit when performing an InlineCall so the complicated tracking we do here is no longer necessary
 - `BeginInlining` doesn't really mark the start of an inlined region, it defines the frame pointer via an lea so rename it `DefCalleeFP`
 - `EndInlining` is really more logically paired with `EnterInlineFrame`, it has memory effects that end in the inlined region, so rename it `LeaveInlineFrame`
 - Defining the inline frame pointer can sometimes make merging diamond control flow induced by `CheckType` difficult so hoist it to the start of the unit. Since we constant propagate `lea` instructions this shouldn't matter.

Reviewed By: jano

Differential Revision: D72073596

fbshipit-source-id: 9d1cb1f455a008918db3d0b7d71b3f04b065387d

Author: paulbiss

hphp/doc/ir.specification

@@ -1614,18 +1614,17 @@ To string conversions:
 
 8. Call & Return
 
-| BeginInlining<func, offset>, D(FramePtr), S(StkPtr), NF
+| DefCalleeFP<func, offset>, D(FramePtr), S(StkPtr) S(FramePtr), NF
 
   Defines a new frame pointer for an ActRec at callBCOff for use in an inlined
-  region. In resumed contexts the new frame is computed relative to S0 as S1 is
-  not a stack location.
+  region. The frame is defined relative to S0, and S1 is the parent frame.
 
 | EnterInlineFrame, ND, S(FramePtr), NF
 
   Marks the start of an inlined function whose stack resides offset cells below
   the SP.
 
-| EndInlining, ND, S(FramePtr), NF
+| LeaveInlineFrame, ND, S(FramePtr), NF
 
   Marks the end of an inlined function. S0 is no longer a valid frame location.
 
@@ -1664,7 +1663,7 @@ This is achieved by calling the inlineSideExit stub and using registers to pass
 all relevant ActRec fields. The stub finishes initialization of the ActRec and
 jumps to S3.
 
-It could be viewed as performing a work similar to a combination of EndInlining,
+It could be viewed as performing a work similar to a combination of LeaveInlineFrame,
 ReqBindJmp and CallFuncEntry.
 
 | EnterFrame, D(FramePtr), S(FramePtr) S(FramePtr) S(Int) S(Int), NF

hphp/runtime/base/backtrace.cpp

@@ -134,10 +134,7 @@ TypedValue* BTFrame::local(int n) const {
 
   if (m_frameId != kRootIFrameID) {
     auto const ifr = jit::getInlineFrame(m_frameId);
-    auto const pubSB = Stack::anyFrameStackBase(m_fp);
-    auto const rootSB = m_pubFrameId != kRootIFrameID
-      ? pubSB + jit::getInlineFrame(m_pubFrameId).sbToRootSbOff
-      : pubSB;
+    auto const rootSB = Stack::anyFrameStackBase(m_fp);
     auto const mySB = rootSB - ifr.sbToRootSbOff;
     auto const fp = (ActRec*)(mySB + ifr.func->numSlotsInFrame());
     return frame_local(fp, n);
@@ -224,10 +221,9 @@ BTFrame initBTFrameAt(jit::CTCA ip, BTFrame frm) {
 
   if (auto const stk = jit::inlineStackAt(ip)) {
     FTRACE_MOD(Trace::fixup, 3,
-               "IStack fp={} ip={} frame={} pubFrame={} callOff={}\n",
-               frm.fpInternal(), ip, stk->frame, stk->pubFrame, stk->callOff);
-    return BTFrame::iframe(
-      frm.fpInternal(), stk->callOff, stk->frame, stk->pubFrame);
+               "IStack fp={} ip={} frame={} callOff={}\n",
+               frm.fpInternal(), ip, stk->frame, stk->callOff);
+    return BTFrame::iframe(frm.fpInternal(), stk->callOff, stk->frame);
   }
 
   if (frm.bcOff() != kInvalidOffset) return frm;
@@ -252,13 +248,12 @@ BTFrame getPrevActRec(
                fp, frm.frameIdInternal(), ifr.parent,
                ifr.func->fullName()->data(), ifr.callOff, ifr.sbToRootSbOff);
 
-    if (ifr.parent == frm.pubFrameIdInternal()) {
+    if (ifr.parent == kRootIFrameID) {
       // Reached published frame, continue following the FP chain.
       return BTFrame::regular(fp, ifr.callOff);
     }
 
-    return BTFrame::iframe(
-      fp, ifr.callOff, ifr.parent, frm.pubFrameIdInternal());
+    return BTFrame::iframe(fp, ifr.callOff, ifr.parent);
   }
 
   // Handle AsyncFunctionWaitHandle tail frame.

hphp/runtime/base/backtrace.h

@@ -289,7 +289,6 @@ struct BacktraceArgs {
  *             ____________________                            |  inl2->base())
  *            |                    |                           |
  *            |  frame: inl2       |                           |
- *            |  pubFrame: caller  |                           |
  *            |  callOff ----------+---------------------------+
  *            |____________________|
  *
@@ -317,7 +316,6 @@ struct IFrame {
 
 struct IStack {
   IFrameID frame;     // leaf frame in this stack
-  IFrameID pubFrame;  // the last published frame to which vmfp() points to
   Offset callOff;
 
   template<class SerDe> void serde(SerDe& sd) {
@@ -346,12 +344,10 @@ struct BTFrame {
 
   // Inlined frame that does not have a materialized ActRec. IFrame backed by
   // `frameID` contains the metadata necessary to reconstruct the frame.
-  // The nearest published frame `fp` is represented by `pubFrameId`.
-  static BTFrame iframe(ActRec* fp, Offset bcOff,
-                        IFrameID frameId, IFrameID pubFrameId) {
+  static BTFrame iframe(ActRec* fp, Offset bcOff, IFrameID frameId) {
     assertx(fp != nullptr);
-    assertx(frameId != kRootIFrameID && frameId != pubFrameId);
-    return BTFrame{fp, bcOff, frameId, pubFrameId};
+    assertx(frameId != kRootIFrameID);
+    return BTFrame{fp, bcOff, frameId};
   }
 
   static BTFrame afwhTailFrame(ActRec* fp, Offset bcOff,
@@ -362,7 +358,7 @@ struct BTFrame {
   }
 
   BTFrame withFP(ActRec* fp) const {
-    return BTFrame{fp, m_bcOff, m_frameId, m_pubFrameId};
+    return BTFrame{fp, m_bcOff, m_frameId};
   }
 
   operator bool() const { return m_fp != nullptr; }
@@ -378,10 +374,6 @@ struct BTFrame {
   // instead use the helpers above.
   ActRec* fpInternal() const { return m_fp; }
   IFrameID frameIdInternal() const { return m_frameId; }
-  IFrameID pubFrameIdInternal() const {
-    assertx(m_frameId != kRootIFrameID);
-    return m_pubFrameId;
-  }
   int8_t afwhTailFrameIdxInternal() const {
     assertx(m_frameId == kRootIFrameID);
     return m_afwhTailFrameIdx;
@@ -391,21 +383,18 @@ struct BTFrame {
 
  private:
   BTFrame() = default;
-  BTFrame(ActRec* fp, Offset bcOff, IFrameID frameId, IFrameID pubFrameId)
-    : m_fp(fp), m_bcOff(bcOff), m_frameId(frameId), m_pubFrameId(pubFrameId) {}
+  BTFrame(ActRec* fp, Offset bcOff, IFrameID frameId)
+    : m_fp(fp), m_bcOff(bcOff), m_frameId(frameId) {}
   BTFrame(ActRec* fp, Offset bcOff, int8_t afwhTailFrameIdx)
     : m_fp(fp), m_bcOff(bcOff), m_frameId(kRootIFrameID)
     , m_afwhTailFrameIdx(afwhTailFrameIdx) {}
 
   ActRec* m_fp{nullptr};
   Offset m_bcOff{kInvalidOffset};
   IFrameID m_frameId{kRootIFrameID};
-  union {
-    // Used if m_frameId != kRootIFrameID
-    IFrameID m_pubFrameId;
-    // Used if m_frameId == kRootIFrameID, this is afwh tail frame if >= 0
-    int8_t m_afwhTailFrameIdx;
-  };
+
+  // Used if m_frameId == kRootIFrameID, this is afwh tail frame if >= 0
+  int8_t m_afwhTailFrameIdx;
 };
 
 Array createBacktrace(const BacktraceArgs& backtraceArgs);

hphp/runtime/test/alias-class.cpp

@@ -480,13 +480,13 @@ TEST(AliasClass, FrameUnion) {
   IRUnit unit{test_context};
   auto const bcctx = BCContext { BCMarker::Dummy(), 0 };
   auto const dsData = DefStackData { SBInvOffset { 0 }, SBInvOffset { 0 } };
-  auto const biData = BeginInliningData {
+  auto const biData = DefCalleeFPData {
     IRSPRelOffset { 0 }, nullptr, 1, SrcKey {}, IRSPRelOffset { 0 },
     SBInvOffset { 0 }, 0
   };
   auto const SP = unit.gen(DefRegSP, bcctx, dsData)->dst();
   auto const FP1 = unit.gen(DefFP, bcctx, DefFPData { std::nullopt })->dst();
-  auto const FP2 = unit.gen(BeginInlining, bcctx, biData, SP)->dst();
+  auto const FP2 = unit.gen(DefCalleeFP, bcctx, biData, SP)->dst();
 
   AliasClass const local1 = ALocal { FP1, 0 };
   AliasClass const localR = ALocal { FP1, AliasIdSet::IdRange(0, 2) };

hphp/runtime/vm/jit/analysis.cpp

@@ -147,16 +147,16 @@ SSATmp* least_common_ancestor(SSATmp* s1, SSATmp* s2) {
 
 const Func* funcFromFP(const SSATmp* fp) {
   auto const inst = canonical(fp)->inst();
-  if (inst->is(BeginInlining)) return inst->extra<BeginInlining>()->func;
+  if (inst->is(DefCalleeFP)) return inst->extra<DefCalleeFP>()->func;
   always_assert(inst->is(DefFP, DefFuncEntryFP, EnterFrame));
   return inst->marker().func();
 }
 
 uint32_t frameDepthIndex(const SSATmp* fp) {
   always_assert(fp->isA(TFramePtr));
   fp = canonical(fp);
-  if (fp->inst()->is(BeginInlining)) {
-    auto const extra = fp->inst()->extra<BeginInlining>();
+  if (fp->inst()->is(DefCalleeFP)) {
+    auto const extra = fp->inst()->extra<DefCalleeFP>();
     return extra->depth;
   }
   return 0;
@@ -165,7 +165,7 @@ uint32_t frameDepthIndex(const SSATmp* fp) {
 Optional<IRSPRelOffset> offsetOfFrame(const SSATmp *fp) {
   assertx(fp->isA(TFramePtr));
   auto const inst = canonical(fp)->inst();
-  if (inst->is(BeginInlining)) return inst->extra<BeginInlining>()->spOffset;
+  if (inst->is(DefCalleeFP)) return inst->extra<DefCalleeFP>()->spOffset;
   if (inst->is(DefFP)) return inst->extra<DefFP>()->offset;
   if (inst->is(EnterFrame)) return IRSPRelOffset { 0 };
   always_assert(false);

hphp/runtime/vm/jit/bc-marker.cpp

@@ -53,12 +53,12 @@ SBInvOffset BCMarker::fixupSBOff() const {
   assertx(valid());
   if (fp() == fixupFP()) return SBInvOffset{0};
   auto const begin_inlining = fp()->inst();
-  assertx(begin_inlining->is(BeginInlining));
-  auto const fpSBOffset = begin_inlining->extra<BeginInlining>()->sbOffset;
+  assertx(begin_inlining->is(DefCalleeFP));
+  auto const fpSBOffset = begin_inlining->extra<DefCalleeFP>()->sbOffset;
 
   auto const fixupFPStackBaseOffset = [&] {
-    if (fixupFP()->inst()->is(BeginInlining)) {
-      return fixupFP()->inst()->extra<BeginInlining>()->sbOffset;
+    if (fixupFP()->inst()->is(DefCalleeFP)) {
+      return fixupFP()->inst()->extra<DefCalleeFP>()->sbOffset;
     }
     assertx(fixupFP()->inst()->is(DefFP, EnterFrame));
     auto const defSP = fp()->inst()->src(0)->inst();
@@ -74,7 +74,7 @@ SrcKey BCMarker::fixupSk() const {
 
   auto curFP = fp();
   do {
-    assertx(curFP->inst()->is(BeginInlining));
+    assertx(curFP->inst()->is(DefCalleeFP));
     // Walking the FP chain created from the marker is suspect, but we aren't
     // using it to materialize the fp SSATmp, or offsets based on the begin
     // inlinings.  We are only materializing srckey info.

hphp/runtime/vm/jit/bc-marker.h

@@ -123,7 +123,7 @@ struct BCMarker {
 
   // Normally, the fixup SrcKey is the same as the SrcKey for the marker,
   // however, when inlining has dropped an inner frame the fixup SrcKey will
-  // be inside the parent frame (corresponding to the BeginInlining of the first
+  // be inside the parent frame (corresponding to the DefCalleeFP of the first
   // unpublished frame) so that its offset is relative to the live ActRec.
   SrcKey fixupSk() const;
 

hphp/runtime/vm/jit/cg-meta.cpp

@@ -86,11 +86,9 @@ void insertStacks(
   IFrameID start, const std::vector<std::pair<TCA,IStack>>& stacks
 ) {
   for (auto& stk : stacks) {
-    assertx(stk.second.frame != stk.second.pubFrame);
     auto off = stackAddrToOffset(stk.first);
     auto val = stk.second;
     val.frame += start;
-    if (val.pubFrame != kRootIFrameID) val.pubFrame += start;
 
     if (auto pos = s_inlineStacks.find(off)) {
       *pos = val;

hphp/runtime/vm/jit/dce.cpp

@@ -705,8 +705,8 @@ bool canDCE(const IRInstruction& inst) {
   case StMROProp:
   case CheckMROProp:
   case FinishMemberOp:
-  case BeginInlining:
-  case EndInlining:
+  case DefCalleeFP:
+  case LeaveInlineFrame:
   case EnterInlineFrame:
   case SetOpTV:
   case OutlineSetOp:
@@ -987,8 +987,8 @@ void processCatchBlock(IRUnit& unit, DceState& state, Block* block,
     // If the catch block occurs within an inlined frame the outer stack
     // locations (those above the inlined frame) are not dead and cannot be
     // elided as we may not throw through the outer callers.
-    if (block->back().src(0)->inst()->is(BeginInlining)) {
-      auto const extra = block->back().src(0)->inst()->extra<BeginInlining>();
+    if (block->back().src(0)->inst()->is(DefCalleeFP)) {
+      auto const extra = block->back().src(0)->inst()->extra<DefCalleeFP>();
       auto const spOff = extra->spOffset;
       assertx(stackTop <= spOff);
       if (spOff < stackTop + numTrackedSlots) {

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

@@ -2545,8 +2545,8 @@ struct ProfileCallTargetData : IRExtraData {
   rds::Handle handle;
 };
 
-struct BeginInliningData : IRExtraData {
-  BeginInliningData(IRSPRelOffset offset, const Func* func, unsigned depth,
+struct DefCalleeFPData : IRExtraData {
+  DefCalleeFPData(IRSPRelOffset offset, const Func* func, unsigned depth,
                     SrcKey returnSk, IRSPRelOffset sbOffset,
                     SBInvOffset returnSPOff, int cost)
     : spOffset(offset)
@@ -2575,7 +2575,7 @@ struct BeginInliningData : IRExtraData {
     );
   }
 
-  bool equals(const BeginInliningData& o) const {
+  bool equals(const DefCalleeFPData& o) const {
     return
       spOffset == o.spOffset && func == o.func && depth == o.depth &&
       returnSk == o.returnSk && sbOffset == o.sbOffset &&
@@ -3117,7 +3117,7 @@ X(DefRegSP,                     DefStackData);
 X(LdStk,                        IRSPRelOffsetData);
 X(LdStkAddr,                    IRSPRelOffsetData);
 X(StFrameMeta,                  StFrameMetaData);
-X(BeginInlining,                BeginInliningData);
+X(DefCalleeFP,                  DefCalleeFPData);
 X(ReqBindJmp,                   ReqBindJmpData);
 X(ReqInterpBBNoTranslate,       ReqBindJmpData);
 X(ReqRetranslate,               IRSPRelOffsetData);