[PM] Edit comments on PM Proxy and utility classes.

[jlebar]

@chandlerc, Per our conversation on IRC: I'm sending this now just for informational purposes. I will rebase after you submit your outstanding patches.

[jlebar]

Remove leftover comment.

[jlebar]

Rebase.

[chandlerc]

llvm/include/llvm/IR/PassManager.h

732–734

FWIW, I continue to prefer "outer" and "inner". There is an inherent requirement that the smaller IR units in question be *contained* by the larger units. I'd like words that help indicate that relationship which is a stronger statement than their size IMO.

735–746

All of this is fantastic though. =]

754

Why the less precise member name?

There are places where 'AM' is used and is *not* the same as the member, so it seems less ambiguous to give it some more specific name.

755

I dislike using RHS for something that is not a binary operator. A lot of this delta isn't comment-only edits...

840–841

s/update/invalidate/

954

on Function units -> on Functions

1063

This isn't common for full-file-scoped namespaces in LLVM. See the last paragraph of http://llvm.org/docs/CodingStandards.html#namespace-indentation

[mehdi_amini]

llvm/include/llvm/IR/PassManager.h

732–734

+1 with this and all the other naming remarks below.

1063

I believe it is quite common, in llvm/include there are 941 "namespace llvm" and 478 have a closing comment.

[jlebar]

llvm/include/llvm/IR/PassManager.h

732–734

I agree that containment is the relationship we ideally want to express.

However I don't think that "outer" and "inner" are the right words to express that relationship. Consider two cases:

• Every function is contained in a module, and functions together (mostly) comprise a module. We (or, I, anyway) would not say "a module is outside a function", or "a function's outer module". I would say "a function's containing module".

• Every loop is contained in a function. But loops are not the primary component of functions. In this case too I would not say "a function is outside a loop" or "a loop's outer function". I would say "a loop's containing function".

I am happy talking about inner and outer AMs, but I think we're overloading those words in a confusing way when we try to talk about inner/outer IR units.

I've thought about this since last night and unfortunately I don't have a concrete suggestion other than "bigger" / "smaller" (written in quotes, as currently in the patch, to indicate the somewhat figurative use of these words).

What if we just said

An analysis over a "larger" IR unit that provides access to an analysis manager over a "smaller" IR unit, which the larger unit must contain.

? Is that sufficient? If not I'll keep pondering this.

[chandlerc]

llvm/include/llvm/IR/PassManager.h

732–734

I see the confusion you're hitting.

For me, "smaller" and "larger" are if anything more confusing though, so I don't think they are an effective way to address the confusion. It just replaces one with another.

I also don't have any concrete suggestions yet, but currently I would leave it as-is until we do have something that isn't such a tradeoff.

I also wonder how much renaming these would help focus on the AMs being "inner" and "outer" rather than the IR units and address the confusion in that way.

[mehdi_amini]

llvm/include/llvm/IR/PassManager.h

732–734

I'm fairly sure that outer and inner are the terms for nesting of loops in the literature, so it is what seems the less confusing to me when extended for other nested IRUnits.

[jlebar]

Address review comments, and in particular change smaller/larger -> inner/outer.

llvm/include/llvm/IR/PassManager.h

732–734

OK, I've rewritten to use "inner" and "outer" -- what do you think?

754

Changed back; I don't feel strongly. In fact I reverted the comment deletion here too.

755

Changed back to Arg -- I actually have no idea why I made that change in the first place.

I'd offer to split out the non-comment changes, but in the patch I'm about to upload, they're (all?) gone anyway.

1063

I got rid of it so there's no controversy.

[jlebar]

Thank you both for the review!

[chandlerc]

LGTM, thanks!

llvm/include/llvm/IR/PassManager.h

 extern template class AnalysisManager<Module>;
 /// \brief Convenience typedef for the Module analysis manager.
 typedef AnalysisManager<Module> ModuleAnalysisManager;
 
 extern template class AnalysisManager<Function>;
 /// \brief Convenience typedef for the Function analysis manager.
 typedef AnalysisManager<Function> FunctionAnalysisManager;
 
-/// \brief A module analysis which acts as a proxy for a function analysis
-/// manager.
+/// \brief An analysis over a "larger" IR unit that provides access to an
+/// analysis manager over a "smaller" IR unit.
 ///
-/// This primarily proxies invalidation information from the module analysis
-/// manager and module pass manager to a function analysis manager. You should
-/// never use a function analysis manager from within (transitively) a module
-/// pass manager unless your parent module pass has received a proxy result
-/// object for it.
-///
-/// Note that the proxy's result is a move-only object and represents ownership
-/// of the validity of the analyses in the \c FunctionAnalysisManager it
-/// provides.
+/// For example, you might have an
+/// InnerAnalysisManagerProxy<FunctionAnalysisManager, Module>.  This is an
+/// analysis over Modules that provides access to a Function analysis manager.
+/// (The FunctionAnalysisManager is the "inner" manager being proxied.)
+///
+/// You should never use a "smaller" analysis manager from within (transitively)
+/// a "larger" pass manager unless you have received a proxy result object for
+/// the smaller analysis manager.
+///
+/// Note that the proxy's result is a move-only RAII object.  The validity of
+/// the analyses in the "smaller" analysis manager is tied to its lifetime.
 template <typename AnalysisManagerT, typename IRUnitT, typename... ExtraArgTs>
 class InnerAnalysisManagerProxy
     : public AnalysisInfoMixin<
           InnerAnalysisManagerProxy<AnalysisManagerT, IRUnitT>> {
 public:
   class Result {
   public:
     explicit Result(AnalysisManagerT &AM) : AM(&AM) {}
-    Result(Result &&Arg) : AM(std::move(Arg.AM)) {
-      // We have to null out the analysis manager in the moved-from state
-      // because we are taking ownership of the responsibilty to clear the
-      // analysis state.
-      Arg.AM = nullptr;
-    }
+    Result(Result &&Arg) : AM(Arg.AM) { Arg.AM = nullptr; }
     Result &operator=(Result &&RHS) {
       AM = RHS.AM;
-      // We have to null out the analysis manager in the moved-from state
-      // because we are taking ownership of the responsibilty to clear the
-      // analysis state.
       RHS.AM = nullptr;
       return *this;
     }
     ~Result() {
-      // AM is cleared in a moved from state where there is nothing to do.
+      // Null AM indicates we've been moved from, so there's nothing to do.
       if (!AM)
         return;
 
       // Clear out the analysis manager if we're being destroyed -- it means we
       // didn't even see an invalidate call when we got invalidated.
       AM->clear();
     }
 
     /// \brief Accessor for the analysis manager.
     AnalysisManagerT &getManager() { return *AM; }
 
-    /// \brief Handler for invalidation of the module.
+    /// \brief Handler for invalidation of the "larger" IR unit, \c IRUnitT.
     ///
-    /// If this analysis itself is preserved, then we assume that the set of \c
-    /// Function objects in the \c Module hasn't changed and thus we don't need
-    /// to invalidate *all* cached data associated with a \c Function* in the \c
-    /// FunctionAnalysisManager.
+    /// If the proxy analysis itself is not preserved, we assume that the set of
+    /// "smaller" objects contained in IRUnit may have changed.  In this case,
+    /// we have to call \c clear() on the inner analysis manager, as it may now
+    /// have stale pointers to its "smaller" objects.
     ///
-    /// Regardless of whether this analysis is marked as preserved, all of the
-    /// analyses in the \c FunctionAnalysisManager are potentially invalidated
+    /// Regardless of whether the proxy analysis is marked as preserved, all of
+    /// the analyses in the inner analysis manager are potentially invalidated
     /// based on the set of preserved analyses.
     bool invalidate(
         IRUnitT &IR, const PreservedAnalyses &PA,
         typename AnalysisManager<IRUnitT, ExtraArgTs...>::Invalidator &) {
-      // If this proxy isn't marked as preserved, then we can't even invalidate
-      // individual function analyses, there may be an invalid set of Function
-      // objects in the cache making it impossible to incrementally preserve
-      // them. Just clear the entire manager.
+      // Clear AM if the proxy analysis is not preserved, as this indicates that
+      // the set of "smaller" IR units may have changed.
       if (!PA.preserved(InnerAnalysisManagerProxy::ID()))
         AM->clear();
 
+      // TODO: Why don't we call AM->invalidate here, somehow?
+
       // Return false to indicate that this result is still a valid proxy.
       return false;
     }
 
   private:
     AnalysisManagerT *AM;
   };
 
   explicit InnerAnalysisManagerProxy(AnalysisManagerT &AM) : AM(&AM) {}
 
   /// \brief Run the analysis pass and create our proxy result object.
   ///
-  /// This doesn't do any interesting work, it is primarily used to insert our
-  /// proxy result object into the module analysis cache so that we can proxy
-  /// invalidation to the function analysis manager.
-  ///
-  /// In debug builds, it will also assert that the analysis manager is empty
-  /// as no queries should arrive at the function analysis manager prior to
-  /// this analysis being requested.
+  /// This doesn't do any interesting work; it is primarily used to insert our
+  /// proxy result object into the "larger" analysis cache so that we can proxy
+  /// invalidation to the "smaller" analysis manager.
   Result run(IRUnitT &IR, AnalysisManager<IRUnitT, ExtraArgTs...> &,
              ExtraArgTs...) {
     return Result(*AM);
   }
 
 private:
   friend AnalysisInfoMixin<
       InnerAnalysisManagerProxy<AnalysisManagerT, IRUnitT>>;
   static AnalysisKey Key;
 
   AnalysisManagerT *AM;
 };
 
 template <typename AnalysisManagerT, typename IRUnitT, typename... ExtraArgTs>
 AnalysisKey
     InnerAnalysisManagerProxy<AnalysisManagerT, IRUnitT, ExtraArgTs...>::Key;
 
 extern template class InnerAnalysisManagerProxy<FunctionAnalysisManager,
                                                 Module>;
 /// Provide the \c FunctionAnalysisManager to \c Module proxy.
 typedef InnerAnalysisManagerProxy<FunctionAnalysisManager, Module>
     FunctionAnalysisManagerModuleProxy;
 
-/// \brief A function analysis which acts as a proxy for a module analysis
-/// manager.
+/// \brief An analysis over a "smaller" IR unit that provides access to an
+/// analysis manager over a "larger" IR unit.
 ///
-/// This primarily provides an accessor to a parent module analysis manager to
-/// function passes. Only the const interface of the module analysis manager is
-/// provided to indicate that once inside of a function analysis pass you
-/// cannot request a module analysis to actually run. Instead, the user must
-/// rely on the \c getCachedResult API.
-///
-/// This proxy *doesn't* manage the invalidation in any way. That is handled by
-/// the recursive return path of each layer of the pass manager and the
-/// returned PreservedAnalysis set.
+/// For example, you might have an
+/// OuterAnalysisManagerProxy<ModuleAnalysisManager, Function>.  This is an
+/// analysis over Functions which provides access to a Module analysis manager.
+/// (The ModuleAnalysisManager is the "outer" manager being proxied.)
+///
+/// Only the const interface of the proxied analysis manager is provided to
+/// indicate that once inside of a "smaller" analysis pass you cannot request a
+/// module analysis to actually run. Instead, the user must rely on the \c
+/// getCachedResult API.
+///
+/// This proxy doesn't manage invalidation in any way -- that is handled by the
+/// recursive return path of each layer of the pass manager.  A consequence of
+/// this is, the "larger" analyses may be stale.  We update the "larger"
+/// analyses only when we're done running passes over the "smaller" IR units.
 template <typename AnalysisManagerT, typename IRUnitT, typename... ExtraArgTs>
 class OuterAnalysisManagerProxy
     : public AnalysisInfoMixin<
           OuterAnalysisManagerProxy<AnalysisManagerT, IRUnitT>> {
 public:
   /// \brief Result proxy object for \c OuterAnalysisManagerProxy.
   class Result {
   public:
     explicit Result(const AnalysisManagerT &AM) : AM(&AM) {}
 
     const AnalysisManagerT &getManager() const { return *AM; }
 
-    /// \brief Handle invalidation by ignoring it, this pass is immutable.
+    /// \brief Handle invalidation by ignoring it; this pass is immutable.
     bool invalidate(
         IRUnitT &, const PreservedAnalyses &,
         typename AnalysisManager<IRUnitT, ExtraArgTs...>::Invalidator &) {
       return false;
     }
 
   private:
     const AnalysisManagerT *AM;

 };
 
 template <typename AnalysisManagerT, typename IRUnitT, typename... ExtraArgTs>
 AnalysisKey
     OuterAnalysisManagerProxy<AnalysisManagerT, IRUnitT, ExtraArgTs...>::Key;
 
 extern template class OuterAnalysisManagerProxy<ModuleAnalysisManager,
                                                 Function>;
-/// Provide the \c ModuleAnalysisManager to \c Fucntion proxy.
+/// Provide the \c ModuleAnalysisManager to \c Function proxy.
 typedef OuterAnalysisManagerProxy<ModuleAnalysisManager, Function>
     ModuleAnalysisManagerFunctionProxy;
 
 /// \brief Trivial adaptor that maps from a module to its functions.
 ///
 /// Designed to allow composition of a FunctionPass(Manager) and
-/// a ModulePassManager. Note that if this pass is constructed with a pointer
-/// to a \c ModuleAnalysisManager it will run the
-/// \c FunctionAnalysisManagerModuleProxy analysis prior to running the function
-/// pass over the module to enable a \c FunctionAnalysisManager to be used
-/// within this run safely.
+/// a ModulePassManager, by running the FunctionPass(Manager) over every
+/// function in the module.
 ///
 /// Function passes run within this adaptor can rely on having exclusive access
 /// to the function they are run over. They should not read or modify any other
 /// functions! Other threads or systems may be manipulating other functions in
 /// the module, and so their state should never be relied on.
 /// FIXME: Make the above true for all of LLVM's actual passes, some still
 /// violate this principle.
 ///
 /// Function passes can also read the module containing the function, but they
 /// should not modify that module outside of the use lists of various globals.
 /// For example, a function pass is not permitted to add functions to the
 /// module.
 /// FIXME: Make the above true for all of LLVM's actual passes, some still
 /// violate this principle.
+///
+/// Note that although function passes can access module analyses, module
+/// analyses are not invalidated while the function passes are running, so they
+/// may be stale.  Function analyses will not be stale.
 template <typename FunctionPassT>
 class ModuleToFunctionPassAdaptor
     : public PassInfoMixin<ModuleToFunctionPassAdaptor<FunctionPassT>> {
 public:
   explicit ModuleToFunctionPassAdaptor(FunctionPassT Pass)
       : Pass(std::move(Pass)) {}
 
   /// \brief Runs the function pass across every function in the module.
   PreservedAnalyses run(Module &M, ModuleAnalysisManager &AM) {
-    // Setup the function analysis manager from its proxy.
     FunctionAnalysisManager &FAM =
         AM.getResult<FunctionAnalysisManagerModuleProxy>(M).getManager();
 
     PreservedAnalyses PA = PreservedAnalyses::all();
     for (Function &F : M) {
       if (F.isDeclaration())
         continue;
 
       PreservedAnalyses PassPA = Pass.run(F, FAM);
 
       // We know that the function pass couldn't have invalidated any other
       // function's analyses (that's the contract of a function pass), so
       // directly handle the function analysis manager's invalidation here.
       FAM.invalidate(F, PassPA);
 
       // Then intersect the preserved set so that invalidation of module
       // analyses will eventually occur when the module pass completes.
       PA.intersect(std::move(PassPA));
     }
 
-    // By definition we preserve the proxy. We also preserve all analyses on
-    // Function units. This precludes *any* invalidation of function analyses
-    // by the proxy, but that's OK because we've taken care to invalidate
-    // analyses in the function analysis manager incrementally above.
+    // The FunctionAnalysisManagerModuleProxy is preserved because (we assume)
+    // the function passes we ran didn't add or remove any functions.
+    //
+    // We also preserve all analyses on Function units, because we did all the
+    // invalidation we needed to do above.
     PA.preserve<AllAnalysesOn<Function>>();
     PA.preserve<FunctionAnalysisManagerModuleProxy>();
     return PA;
   }
 
 private:
   FunctionPassT Pass;
 };
 
 /// \brief A function to deduce a function pass type and wrap it in the
 /// templated adaptor.
 template <typename FunctionPassT>
 ModuleToFunctionPassAdaptor<FunctionPassT>
 createModuleToFunctionPassAdaptor(FunctionPassT Pass) {
   return ModuleToFunctionPassAdaptor<FunctionPassT>(std::move(Pass));
 }
 
-/// \brief A template utility pass to force an analysis result to be available.
+/// \brief A utility pass template to force an analysis result to be available.
 ///
 /// If there are extra arguments at the pass's run level there may also be
 /// extra arguments to the analysis manager's \c getResult routine. We can't
 /// guess how to effectively map the arguments from one to the other, and so
 /// this specialization just ignores them.
 ///
 /// Specific patterns of run-method extra arguments and analysis manager extra
 /// arguments will have to be defined as appropriate specializations.
PreservedAnalyses run(IRUnitT &Arg, AnalysisManagerT &AM,
                         ExtraArgTs &&... Args) {
     (void)AM.template getResult<AnalysisT>(Arg,
                                            std::forward<ExtraArgTs>(Args)...);
 
     return PreservedAnalyses::all();
   }
 };
 
-/// \brief A template utility pass to force an analysis result to be
-/// invalidated.
-///
-/// This is a no-op pass which simply forces a specific analysis result to be
-/// invalidated when it is run.
+/// \brief A no-op pass template which simply forces a specific analysis result
+/// to be invalidated.
 template <typename AnalysisT>
 struct InvalidateAnalysisPass
     : PassInfoMixin<InvalidateAnalysisPass<AnalysisT>> {
   /// \brief Run this pass over some unit of IR.
   ///
-  /// This pass can be run over any unit of IR and use any analysis manager
+  /// This pass can be run over any unit of IR and use any analysis manager,
   /// provided they satisfy the basic API requirements. When this pass is
   /// created, these methods can be instantiated to satisfy whatever the
   /// context requires.
   template <typename IRUnitT, typename AnalysisManagerT, typename... ExtraArgTs>
   PreservedAnalyses run(IRUnitT &Arg, AnalysisManagerT &AM, ExtraArgTs &&...) {
-    // We have to directly invalidate the analysis result as we can't
+    // We have to directly invalidate the analysis result, as we can't
     // enumerate all other analyses and use the preserved set to control it.
     AM.template invalidate<AnalysisT>(Arg);
 
     return PreservedAnalyses::all();
   }
 };
 
-/// \brief A utility pass that does nothing but preserves no analyses.
+/// \brief A utility pass that does nothing, but preserves no analyses.
 ///
-/// As a consequence fo not preserving any analyses, this pass will force all
-/// analysis passes to be re-run to produce fresh results if any are needed.
+/// Because this preserves no analyses, any analysis passes queried after this
+/// pass runs will recompute fresh results.
 struct InvalidateAllAnalysesPass : PassInfoMixin<InvalidateAllAnalysesPass> {
   /// \brief Run this pass over some unit of IR.
   template <typename IRUnitT, typename AnalysisManagerT, typename... ExtraArgTs>
   PreservedAnalyses run(IRUnitT &, AnalysisManagerT &, ExtraArgTs &&...) {
     return PreservedAnalyses::none();
   }
 };
 
private:
   PassT P;
 };
 
 template <typename PassT>
 RepeatedPass<PassT> createRepeatedPass(int Count, PassT P) {
   return RepeatedPass<PassT>(Count, std::move(P));
 }
 
-}
+} // namespace llvm
 
 #endif