Document TraceBuilder/HhbcTranslator/Simplifier @override-unit-failures

Adds comments explaining the role of each of these modules.
The comments explain the roles as I am planning to make them, not as
they actually are, so they are currently partially lies.  (I will move
EvalStack into TraceBuilder, and get the remaining "dubious" gen
routines out to HhbcTranslator on top of this diff.)

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

@@ -1327,7 +1327,7 @@ void HhbcTranslator::emitClsCnsD(int32_t cnsNameStrId, int32_t clsNameStrId) {
     // TODO: 2068502 pick one of these two implementations and remove the other.
     Trace* exitTrace = getExitSlowTrace();
     SSATmp* cns = gen(LdClsCns, Type::Cell, cnsNameTmp, clsNameTmp);
-    gen(CheckInit, m_tb->getFirstBlock(exitTrace), cns);
+    gen(CheckInit, exitTrace, cns);
     push(cns);
   } else {
     // if-then-else
@@ -2713,7 +2713,7 @@ void HhbcTranslator::emitCGet(const StringData* name,
                 ? nullptr : getExitSlowTrace();
   SSATmp* ptr = (this->*emitLdAddr)(name,
                                     exitOnFailure
-                                      ? m_tb->getFirstBlock(getExitSlowTrace())
+                                      ? getExitSlowTrace()->front()
                                       : nullptr);
   if (!isInferedType) ptr = gen(UnboxPtr, ptr);
   pushIncRef(gen(LdMem, resultType, exit, ptr, cns(0)));

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

@@ -35,6 +35,8 @@ namespace VM {
 namespace Transl { struct PropInfo; }
 namespace JIT {
 
+//////////////////////////////////////////////////////////////////////
+
 struct EvalStack {
   void push(SSATmp* tmp) {
     m_vector.push_back(tmp);
@@ -78,30 +80,26 @@ private:
   std::vector<SSATmp*> m_vector;
 };
 
-class TypeGuard {
- public:
-  enum Kind {
-    Local,
-    Stack,
-    Iter
-  };
-
-  TypeGuard(Kind kind, uint32_t index, Type type)
-    : m_kind(kind)
-    , m_index(index)
-    , m_type(type)
-  {}
-
-  Kind      getKind()  const { return m_kind;  }
-  uint32_t  getIndex() const { return m_index; }
-  Type      type()  const { return m_type;  }
-
- private:
-  Kind      m_kind;
-  uint32_t  m_index;
-  Type      m_type;
-};
+//////////////////////////////////////////////////////////////////////
 
+/*
+ * This module is responsible for determining high-level HHBC->IR
+ * compilation strategy.
+ *
+ * For each bytecode Foo in HHBC, there is a function in this class
+ * called emitFoo which handles translating it into HHIR.
+ *
+ * Additionally, while transating bytecode, this module manages a
+ * virtual execution stack modelling the state of the stack since the
+ * last time we emitted an IR instruction that materialized it
+ * (e.g. SpillStack or SpillFrame).
+ *
+ * HhbcTranslator is where we make optimiations that relate to overall
+ * knowledge of the runtime and HHBC.  For example, decisions like
+ * whether to use IR instructions that have constant Class*'s (for a
+ * AttrUnique class) instead of loading a Class* from TargetCache are
+ * made at this level.
+ */
 struct HhbcTranslator {
   HhbcTranslator(IRFactory& irFactory,
                  Offset bcStartOffset,
@@ -618,6 +616,29 @@ private:
     const Func* func;
   };
 
+  struct TypeGuard {
+    enum Kind {
+      Local,
+      Stack,
+      Iter
+    };
+
+    TypeGuard(Kind kind, uint32_t index, Type type)
+      : m_kind(kind)
+      , m_index(index)
+      , m_type(type)
+    {}
+
+    Kind      getKind()  const { return m_kind;  }
+    uint32_t  getIndex() const { return m_index; }
+    Type      getType()  const { return m_type;  }
+
+  private:
+    Kind      m_kind;
+    uint32_t  m_index;
+    Type      m_type;
+  };
+
 private:
   IRFactory&        m_irFactory;
   std::unique_ptr<TraceBuilder>
@@ -657,6 +678,8 @@ private:
   Trace* const      m_exitGuardFailureTrace;
 };
 
+//////////////////////////////////////////////////////////////////////
+
 }}} // namespace HPHP::VM::JIT
 
 #endif

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

@@ -113,7 +113,7 @@ void optimizeTrace(Trace* trace, TraceBuilder* traceBuilder) {
   if (RuntimeOption::EvalHHIRExtraOptPass
       && (RuntimeOption::EvalHHIRCse
           || RuntimeOption::EvalHHIRSimplification)) {
-    traceBuilder->optimizeTrace();
+    traceBuilder->reoptimize();
     finishPass("after CSE/Simplification");
     // Cleanup any dead code left around by CSE/Simplification
     // Ideally, this would be controlled by a flag returned

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

@@ -20,12 +20,32 @@
 #include "runtime/vm/translator/hopt/cse.h"
 #include "runtime/vm/translator/hopt/ir.h"
 
-namespace HPHP {
-namespace VM {
-namespace JIT {
+namespace HPHP { namespace VM { namespace JIT {
+
+//////////////////////////////////////////////////////////////////////
 
 class TraceBuilder;
 
+//////////////////////////////////////////////////////////////////////
+
+/*
+ * Module that handles state-independent optimizations.
+ *
+ * Specifically, the optimizations in this module should be those that
+ * we can do based only on chasing the use-def chain.  Instructions
+ * can be modified in place or replaced with new instructions as
+ * needed.
+ *
+ * The Simplifier recursively invokes TraceBuilder, which can call
+ * back into it.  It's used both during our initial gen-time
+ * optimizations and in the TraceBuilder::reoptimize pass.
+ *
+ * The line of separation between these two modules is essentially
+ * about who needs to know about tracked state.  If an optimization is
+ * completely stateless (e.g. strength reduction, constant folding,
+ * etc) it goes in here, otherwise it goes in TraceBuilder or some
+ * other pass.
+ */
 struct Simplifier {
   explicit Simplifier(TraceBuilder* t) : m_tb(t) {}
 
@@ -106,6 +126,8 @@ private:
   TraceBuilder* const m_tb;
 };
 
+//////////////////////////////////////////////////////////////////////
+
 /*
  * Propagate very simple copies on the given instruction.
  * Specifically, Movs, and also IncRefs of non-refcounted types.
@@ -114,6 +136,8 @@ private:
  */
 void copyProp(IRInstruction*);
 
+//////////////////////////////////////////////////////////////////////
+
 }}}
 
 #endif

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

@@ -433,7 +433,7 @@ SSATmp* TraceBuilder::genStLoc(uint32_t id,
   if (doRefCount) {
     assert(exit);
     Type innerType = trackedType.innerType();
-    prevValue = gen(LdRef, innerType, getFirstBlock(exit), prevRef);
+    prevValue = gen(LdRef, innerType, exit, prevRef);
   }
   // stref [prevRef] = t1
   Opcode opc = genStoreType ? StRef : StRefNT;
@@ -1289,8 +1289,8 @@ void CSEHash::filter(Block* block, IdomVector& idoms) {
 }
 
 /*
- * optimizeTrace runs another pass of CSE and simplification on an
- * already-built trace, like this:
+ * reoptimize() runs a trace through a second pass of TraceBuilder
+ * optimizations, like this:
  *
  *   reset state.
  *   move all blocks to a temporary list.
@@ -1310,7 +1310,7 @@ void CSEHash::filter(Block* block, IdomVector& idoms) {
  *     if the last conditional branch was turned into a jump, remove the
  *     fall-through edge to the next block.
  */
-void TraceBuilder::optimizeTrace() {
+void TraceBuilder::reoptimize() {
   m_enableCse = RuntimeOption::EvalHHIRCse;
   m_enableSimplification = RuntimeOption::EvalHHIRSimplification;
   if (!m_enableCse && !m_enableSimplification) return;

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

@@ -26,17 +26,71 @@
 
 #include "folly/ScopeGuard.h"
 
-namespace HPHP {
-namespace VM {
-namespace JIT {
+namespace HPHP { namespace VM { namespace JIT {
 
-class TraceBuilder {
-public:
+//////////////////////////////////////////////////////////////////////
+
+/*
+ * This module provides the basic utilities for generating the IR
+ * instructions in a trace, emitting control flow, tracking the state
+ * of locals, and managing how state should merge at control flow join
+ * points.  It also performs some optimizations while generating IR,
+ * and may be reinvoked for a second optimization pass.
+ *
+ *
+ * The types of state tracked by TraceBuilder include:
+ *
+ *   - value availability
+ *
+ *      Used for value propagation and tracking which values can be
+ *      CSE'd (value numbering below).
+ *
+ *   - local types and values
+ *
+ *      We track the current view of these types as we link in new
+ *      instructions that mutate these.  The state of the stack is
+ *      encoded in the IR via the StkPtr chain instead.
+ *
+ *   - current frame and stack pointers
+ *
+ *   - the current function and bytecode offset
+ *
+ *
+ * This module is also responsible for organizing a few types of
+ * gen-time optimizations:
+ *
+ *   - preOptimize pass
+ *
+ *      Before an instruction is linked into the trace, TraceBuilder
+ *      internally runs preOptimize() on it, which can do some
+ *      tracelet-state related modifications to the instruction.  For
+ *      example, it can eliminate redundant guards or weaken DecRef
+ *      instructions that cannot go to zero to DecRefNZ.
+ *
+ *   - value numbering
+ *
+ *      After preOptimize, instructions that support it are hashed and
+ *      looked up in the CSEHash for this trace.  If we find an
+ *      available expression for the same value, instead of linking a
+ *      new instruction into the trace we will just add a use to the
+ *      previous SSATmp.
+ *
+ *   - simplification pass
+ *
+ *      After the preOptimize pass, TraceBuilder calls out to
+ *      Simplifier to perform state-independent optimizations, like
+ *      copy propagation and strength reduction.  (See simplifier.h.)
+ *
+ *
+ * After all the instructions are linked into the trace, this module
+ * can also be used to perform a second round of the above two
+ * optimizations via the reoptimize() entry point.
+ */
+struct TraceBuilder {
   TraceBuilder(Offset initialBcOffset,
                uint32_t initialSpOffsetFromFp,
                IRFactory&,
                const Func* func);
-
   ~TraceBuilder();
 
   void beginInlining(const Func* target,
@@ -51,6 +105,13 @@ public:
   void setEnableCse(bool val)            { m_enableCse = val; }
   void setEnableSimplification(bool val) { m_enableSimplification = val; }
 
+
+  Trace* getTrace() const { return m_trace.get(); }
+  IRFactory* getIrFactory() { return &m_irFactory; }
+  int32_t getSpOffset() { return m_spOffset; }
+  SSATmp* getSp() const { return m_spValue; }
+  SSATmp* getFp() const { return m_fpValue; }
+
   Trace* makeExitTrace(uint32_t bcOff) {
     return m_trace->addExitTrace(makeTrace(m_curFunc->getValFunc(),
                                            bcOff));
@@ -61,15 +122,17 @@ public:
   void setThisAvailable() {
     m_thisIsAvailable = true;
   }
-  void dropLocalRefsInnerTypes();
-
-  // Run one more pass of simplification on this builder's trace.
-  void optimizeTrace();
 
   /*
-   * Create an IRInstruction attached to this Trace, and allocate a
-   * destination SSATmp for it.  Uses the same argument list format as
-   * IRFactory::gen.
+   * Run another pass of TraceBuilder-managed optimizations on this
+   * trace.
+   */
+  void reoptimize();
+
+  /*
+   * Create an IRInstruction attached to the current main Trace, and
+   * allocate a destination SSATmp for it.  Uses the same argument
+   * list format as IRFactory::gen.
    */
   template<class... Args>
   SSATmp* gen(Args&&... args) {
@@ -79,6 +142,10 @@ public:
     );
   }
 
+  /*
+   * Create an IRInstruction, similar to gen(), except link it into
+   * the Trace t instead of the current main trace.
+   */
   template<class... Args>
   IRInstruction* genFor(Trace* t, Args... args) {
     auto instr = m_irFactory.gen(args...);
@@ -86,6 +153,11 @@ public:
     return instr;
   }
 
+  //////////////////////////////////////////////////////////////////////
+  // locals
+
+  Type getLocalType(unsigned id) const;
+
   SSATmp* genLdLoc(uint32_t id);
   SSATmp* genLdLocAddr(uint32_t id);
 
@@ -105,31 +177,16 @@ public:
                    bool doRefCount,
                    bool genStoreType,
                    Trace* exit);
-  void    genSetPropCell(SSATmp* base, int64_t offset, SSATmp* value);
 
   SSATmp* genBoxLoc(uint32_t id);
   void    genBindLoc(uint32_t id, SSATmp* ref, bool doRefCount = true);
 
-  void    genAssertStk(uint32_t id, Type type);
-
-  // TODO(#2058865): we should have a real not opcode
-  SSATmp* genNot(SSATmp* src);
-
-  SSATmp* genDefUninit();
-  SSATmp* genDefInitNull();
-  SSATmp* genDefNull();
-  SSATmp* genPtrToInitNull();
-  SSATmp* genPtrToUninit();
-  SSATmp* genDefNone();
-
-  SSATmp* genCmp(Opcode opc, SSATmp* src1, SSATmp* src2);
-  SSATmp* genCastStk(uint32_t id, Type type);
-  SSATmp* genConvToBool(SSATmp* src);
-  SSATmp* genCallBuiltin(SSATmp* func, Type type,
-                         uint32_t numArgs, SSATmp** args);
-  void    genDecRefStack(Type type, uint32_t stackOff);
   void    genDecRefLoc(int id);
-  void    genDecRefThis();
+
+  //////////////////////////////////////////////////////////////////////
+  // stack
+
+  void    genAssertStk(uint32_t id, Type type);
   SSATmp* genSpillStack(uint32_t stackAdjustment,
                         uint32_t numOpnds,
                         SSATmp** opnds);
@@ -139,38 +196,17 @@ public:
     return genLdStackAddr(m_spValue, offset);
   }
 
-  Trace* getExitSlowTrace(uint32_t bcOff,
-                          int32_t stackDeficit,
-                          uint32_t numOpnds,
-                          SSATmp** opnds);
+  void    genDecRefStack(Type type, uint32_t stackOff);
 
-  /*
-   * Generates a trace exit that can be the target of a conditional
-   * or unconditional control flow instruction from the main trace.
-   *
-   * Lifetime of the returned pointer is managed by the trace this
-   * TraceBuilder is generating.
-   */
-  typedef std::function<void(IRFactory*, Trace*)> ExitTraceCallback;
-  Trace* genExitTrace(uint32_t bcOff,
-                      int32_t  stackDeficit,
-                      uint32_t numOpnds,
-                      SSATmp* const* opnds,
-                      TraceExitType::ExitType,
-                      uint32_t notTakenBcOff = 0,
-                      ExitTraceCallback beforeExit = ExitTraceCallback());
+  //////////////////////////////////////////////////////////////////////
+  // constants
 
-  /*
-   * Generates a target exit trace for GuardFailure exits.
-   *
-   * Lifetime of the returned pointer is managed by the trace this
-   * TraceBuilder is generating.
-   */
-  Trace* genExitGuardFailure(uint32_t off);
-
-  // generates the ExitTrace instruction at the end of a trace
-  void genTraceEnd(uint32_t nextPc,
-                   TraceExitType::ExitType exitType = TraceExitType::Normal);
+  SSATmp* genDefUninit();
+  SSATmp* genDefInitNull();
+  SSATmp* genDefNull();
+  SSATmp* genPtrToInitNull();
+  SSATmp* genPtrToUninit();
+  SSATmp* genDefNone();
 
   template<typename T>
   SSATmp* cns(T val) {
@@ -191,39 +227,31 @@ public:
     return gen(LdConst, typeForConst(val), ConstData(val));
   }
 
-  Trace* getTrace() const { return m_trace.get(); }
-  IRFactory* getIrFactory() { return &m_irFactory; }
-  int32_t getSpOffset() { return m_spOffset; }
-  SSATmp* getSp() const { return m_spValue; }
-  SSATmp* getFp() const { return m_fpValue; }
+  //////////////////////////////////////////////////////////////////////
+  // dubious
 
-  Type getLocalType(unsigned id) const;
+  void    genSetPropCell(SSATmp* base, int64_t offset, SSATmp* value);
 
-  Block* getFirstBlock(Trace* trace) {
-    return trace ? trace->front() : nullptr;
-  }
+  // TODO(#2058865): we should have a real not opcode
+  SSATmp* genNot(SSATmp* src);
+
+  SSATmp* genCmp(Opcode opc, SSATmp* src1, SSATmp* src2);
+  SSATmp* genCastStk(uint32_t id, Type type);
+  SSATmp* genConvToBool(SSATmp* src);
+  SSATmp* genCallBuiltin(SSATmp* func, Type type,
+                         uint32_t numArgs, SSATmp** args);
+  void    genDecRefThis();
+
+  //////////////////////////////////////////////////////////////////////
+  // control flow
+
+  typedef std::function<void(IRFactory*, Trace*)> ExitTraceCallback;
 
   // hint the execution frequency of the current block
   void hint(Block::Hint h) const {
     m_trace->back()->setHint(h);
   }
 
-  struct DisableCseGuard {
-    explicit DisableCseGuard(TraceBuilder& tb)
-      : m_tb(tb)
-      , m_oldEnable(tb.m_enableCse)
-    {
-        m_tb.m_enableCse = false;
-    }
-    ~DisableCseGuard() {
-      m_tb.m_enableCse = m_oldEnable;
-    }
-
-   private:
-    TraceBuilder& m_tb;
-    bool m_oldEnable;
-  };
-
   /*
    * cond() generates if-then-else blocks within a trace.  The caller
    * supplies lambdas to create the branch, next-body, and taken-body.
@@ -298,6 +326,71 @@ public:
     appendBlock(done_block);
   }
 
+  Trace* getExitSlowTrace(uint32_t bcOff,
+                          int32_t stackDeficit,
+                          uint32_t numOpnds,
+                          SSATmp** opnds);
+
+  /*
+   * Generates a trace exit that can be the target of a conditional
+   * or unconditional control flow instruction from the main trace.
+   *
+   * Lifetime of the returned pointer is managed by the trace this
+   * TraceBuilder is generating.
+   */
+  Trace* genExitTrace(uint32_t bcOff,
+                      int32_t  stackDeficit,
+                      uint32_t numOpnds,
+                      SSATmp* const* opnds,
+                      TraceExitType::ExitType,
+                      uint32_t notTakenBcOff = 0,
+                      ExitTraceCallback beforeExit = ExitTraceCallback());
+
+  /*
+   * Generates a target exit trace for GuardFailure exits.
+   *
+   * Lifetime of the returned pointer is managed by the trace this
+   * TraceBuilder is generating.
+   */
+  Trace* genExitGuardFailure(uint32_t off);
+
+  // generates the ExitTrace instruction at the end of a trace
+  void genTraceEnd(uint32_t nextPc,
+                   TraceExitType::ExitType exitType = TraceExitType::Normal);
+
+private:
+  // RAII disable of CSE; only restores if it used to be on.  Used for
+  // control flow, where we currently don't allow CSE.
+  struct DisableCseGuard {
+    explicit DisableCseGuard(TraceBuilder& tb)
+      : m_tb(tb)
+      , m_oldEnable(tb.m_enableCse)
+    {
+        m_tb.m_enableCse = false;
+    }
+    ~DisableCseGuard() {
+      m_tb.m_enableCse = m_oldEnable;
+    }
+
+   private:
+    TraceBuilder& m_tb;
+    bool m_oldEnable;
+  };
+
+  // Saved state information associated with the start of a block, or
+  // for the caller of an inlined function.
+  struct State {
+    SSATmp* spValue;
+    SSATmp* fpValue;
+    SSATmp* curFunc;
+    int32_t spOffset;
+    bool thisAvailable;
+    std::vector<SSATmp*> localValues;
+    std::vector<Type> localTypes;
+    SSATmp* refCountedMemValue;
+    std::vector<SSATmp*> callerAvailableValues; // unordered list
+  };
+
 private:
   SSATmp*   preOptimizeGuardLoc(IRInstruction*);
   SSATmp*   preOptimizeAssertLoc(IRInstruction*);
@@ -329,33 +422,20 @@ private:
   void      updateLocalRefValues(SSATmp* oldRef, SSATmp* newRef);
   void      updateTrackedState(IRInstruction* inst);
   void      clearTrackedState();
+  void      dropLocalRefsInnerTypes();
 
   Trace* makeTrace(const Func* func, uint32_t bcOff) {
     return new Trace(m_irFactory.defBlock(func), bcOff);
   }
 
-  // Saved state information associated with the start of a block, or
-  // for the caller of an inlined function.
-  struct State {
-    SSATmp* spValue;
-    SSATmp* fpValue;
-    SSATmp* curFunc;
-    int32_t spOffset;
-    bool thisAvailable;
-    std::vector<SSATmp*> localValues;
-    std::vector<Type> localTypes;
-    SSATmp* refCountedMemValue;
-    std::vector<SSATmp*> callerAvailableValues; // unordered list
-  };
+private:
   std::unique_ptr<State> createState() const;
   void saveState(Block*);
   void mergeState(State* s1);
   void useState(std::unique_ptr<State> state);
   void useState(Block*);
 
-  /*
-   * Fields
-   */
+private:
   IRFactory& m_irFactory;
   Simplifier m_simplifier;
 
@@ -425,6 +505,8 @@ private:
   std::vector<std::unique_ptr<State>> m_inlineSavedStates;
 };
 
+//////////////////////////////////////////////////////////////////////
+
 }}}
 
 #endif