[PM] Edit comments in PassManager.h.

[jlebar]

Summary

This covers most of PassManager.h, up to the introduction of inner/outer
analysis proxies.

If there's a theme to these changes, it's simplifying the language. For
example:

• PreservedAnalyses is a "set of analyses", not an "abstract set". "Abstract" doesn't have any particular meaning here.
• "Build types for the concept types" becomes "define the concept types".
• Instead of "data structures optimized for pointer-like types using the alignment-provided low bits", say "data structures that use the low bits of pointers."
• "Clear the map pointing into the results list" becomes "Delete the map entries that point into the results list."

This patch also fixes a few places where we referred to "function" and
"module" pass/analysis managers, instead of the more abstract "IRUnitT"
PM/AMs we have now.

[jlebar]

@chandlerc , should I just land these with post-commit review (after rebasing)? Or, do you have other big patches in flight that you would like me to avoid bitrotting?

[jlebar]

("These" being this and D27502.)

[chandlerc]

Minor comments below, this patch LGTM with fixes to them.

I'd appreciate doing precommit review on these patches if there is any semantic changes mostly to check and make sure you're not spotting a bug where the old comment should have been correct but wasn't due to bugs or mistakes in the code.

I'm not worried about merge conflicts. The biggest patch i've got in-progress is mostly touching LoopPassManager stuff.

llvm/include/llvm/IR/PassManager.h

133–134 ↗

(On Diff #80142)

Above you use "Given an analysis's ID", here you use "Given a pointer to its AnalysisKey". I mildly prefer the former wording to the latter, but I'm more interested in consistency.

448–449 ↗

(On Diff #80142)

Make the DebugLogging comments consistent? And use \p DebugLogging?

[jlebar]

Thank you for the review! I've rebased now, will submit soon.

llvm/trunk/include/llvm/IR/PassManager.h

 /// identifies that particular analysis pass type.
 ///
 /// Analysis passes should have a static data member of this type and derive
 /// from the \c AnalysisInfoMixin to get a static ID method used to identify
 /// the analysis in the pass management infrastructure.
 struct alignas(8) AnalysisKey {};
 
 /// A special type used to provide an address that identifies a set of related
-/// analyses.
+/// analyses.  These sets are primarily used below to mark sets of analyses as
+/// preserved.
 ///
-/// These sets are primarily used below to mark sets of analyses as preserved.
-/// An example would be analyses depending only on the CFG of a function.
-/// A transformation can mark that it is preserving the CFG of a function and
-/// then analyses can check for this rather than each transform having to fully
-/// enumerate every analysis preserved.
+/// For example, a transformation can indicate that it preserves the CFG of a
+/// function by preserving the appropriate AnalysisSetKey.  An analysis that
+/// depends only on the CFG can then check if that AnalysisSetKey is preserved;
+/// if it is, the analysis knows that it itself is preserved.
 struct alignas(8) AnalysisSetKey {};
 
-/// Class for tracking what analyses are preserved after a transformation pass
-/// runs over some unit of IR.
+/// A set of analyses that are preserved following a run of a transformation
+/// pass.
 ///
-/// Transformation passes build and return these objects when run over the IR
-/// to communicate which analyses remain valid afterward. For most passes this
-/// is fairly simple: if they don't change anything all analyses are preserved,
+/// Transformation passes build and return these objects to communicate which
+/// analyses are still valid after the transformation. For most passes this is
+/// fairly simple: if they don't change anything all analyses are preserved,
 /// otherwise only a short list of analyses that have been explicitly updated
 /// are preserved.
 ///
-/// This class also provides the ability to mark abstract *sets* of analyses as
-/// preserved. These sets allow passes to indicate that they preserve broad
-/// aspects of the IR (such as its CFG) and analyses to opt in to that being
-/// sufficient without the passes having to fully enumerate such analyses.
+/// This class also lets transformation passes mark abstract *sets* of analyses
+/// as preserved. A transformation that (say) does not alter the CFG can
+/// indicate such by marking a particular AnalysisSetKey as preserved, and
+/// then analyses can query whether that AnalysisSetKey is preserved.
 ///
-/// Finally, this class can represent "abandoning" an analysis, which marks it
-/// as not-preserved even if it would be covered by some abstract set of
-/// analyses.
+/// Finally, this class can represent an "abandoned" analysis, which is
+/// not preserved even if it would be covered by some abstract set of analyses.
 ///
 /// Given a `PreservedAnalyses` object, an analysis will typically want to
 /// figure out whether it is preserved. In the example below, MyAnalysisType is
 /// preserved if it's not abandoned, and (a) it's explicitly marked as
 /// preserved, (b), the set AllAnalysesOn<MyIRUnit> is preserved, or (c) both
 /// AnalysisSetA and AnalysisSetB are preserved.
 ///
 /// ```
static PreservedAnalyses all() {
     PreservedAnalyses PA;
     PA.PreservedIDs.insert(&AllAnalysesKey);
     return PA;
   }
 
   /// Mark an analysis as preserved.
   template <typename AnalysisT> void preserve() { preserve(AnalysisT::ID()); }
 
-  /// Mark an analysis as preserved using its ID.
+  /// \brief Given an analysis's ID, mark the analysis as preserved, adding it
+  /// to the set.
   void preserve(AnalysisKey *ID) {
     // Clear this ID from the explicit not-preserved set if present.
     NotPreservedAnalysisIDs.erase(ID);
 
     // If we're not already preserving all analyses (other than those in
     // NotPreservedAnalysisIDs).
     if (!areAllPreserved())
       PreservedIDs.insert(ID);
class PreservedAnalysisChecker {
 
     /// A PreservedAnalysisChecker is tied to a particular Analysis because
     /// `preserved()` and `preservedSet()` both return false if the Analysis
     /// was abandoned.
     PreservedAnalysisChecker(const PreservedAnalyses &PA, AnalysisKey *ID)
         : PA(PA), ID(ID), IsAbandoned(PA.NotPreservedAnalysisIDs.count(ID)) {}
 
   public:
-    /// Returns true if the checker's analysis was not abandoned and the
-    /// analysis is either is explicitly preserved or all analyses are
-    /// preserved.
+    /// Returns true if the checker's analysis was not abandoned and either
+    ///  - the analysis is explicitly preserved or
+    ///  - all analyses are preserved.
     bool preserved() {
       return !IsAbandoned && (PA.PreservedIDs.count(&AllAnalysesKey) ||
                               PA.PreservedIDs.count(ID));
     }
 
-    /// Returns true if the checker's analysis was not abandoned and either the
-    /// provided set type is either explicitly preserved or all analyses are
-    /// preserved.
+    /// Returns true if the checker's analysis was not abandoned and either
+    ///  - \p AnalysisSetT is explicitly preserved or
+    ///  - all analyses are preserved.
     template <typename AnalysisSetT> bool preservedSet() {
       AnalysisSetKey *SetID = AnalysisSetT::ID();
       return !IsAbandoned && (PA.PreservedIDs.count(&AllAnalysesKey) ||
                               PA.PreservedIDs.count(SetID));
     }
   };
 
   /// Build a checker for this `PreservedAnalyses` and the specified analysis
public:
   /// You can use the returned object to query whether an analysis was
   /// preserved. See the example in the comment on `PreservedAnalysis`.
   PreservedAnalysisChecker getChecker(AnalysisKey *ID) const {
     return PreservedAnalysisChecker(*this, ID);
   }
 
   /// Test whether all analyses are preserved (and none are abandoned).
   ///
-  /// This lets analyses optimize for the common case where a transformation
-  /// made no changes to the IR.
+  /// This is used primarily to optimize for the common case of a transformation
+  /// which makes no changes to the IR.
   bool areAllPreserved() const {
     return NotPreservedAnalysisIDs.empty() &&
            PreservedIDs.count(&AllAnalysesKey);
   }
 
   /// Directly test whether a set of analyses is preserved.
   ///
   /// This is only true when no analyses have been explicitly abandoned.

 };
 
 // Forward declare the analysis manager template.
 template <typename IRUnitT, typename... ExtraArgTs> class AnalysisManager;
 
 /// A CRTP mix-in to automatically provide informational APIs needed for
 /// passes.
 ///
-/// This provides some boiler plate for types that are passes.
+/// This provides some boilerplate for types that are passes.
 template <typename DerivedT> struct PassInfoMixin {
-  /// Returns the name of the derived pass type.
+  /// Gets the name of the pass we are mixed into.
   static StringRef name() {
     StringRef Name = getTypeName<DerivedT>();
     if (Name.startswith("llvm::"))
       Name = Name.drop_front(strlen("llvm::"));
     return Name;
   }
 };
 
-/// A CRTP mix-in to automatically provide informational APIs needed for
-/// analysis passes.
+/// A CRTP mix-in that provides informational APIs needed for analysis passes.
 ///
-/// This provides some boiler plate for types that are analysis passes. It
-/// automatically mixes in \c PassInfoMixin and adds informational APIs
-/// specifically used for analyses.
+/// This provides some boilerplate for types that are analysis passes. It
+/// automatically mixes in \c PassInfoMixin.
 template <typename DerivedT>
 struct AnalysisInfoMixin : PassInfoMixin<DerivedT> {
   /// Returns an opaque, unique ID for this analysis type.
   ///
-  /// This ID is a pointer type that is guaranteed to be 8-byte aligned and
-  /// thus suitable for use in sets, maps, and other data structures optimized
-  /// for pointer-like types using the alignment-provided low bits.
+  /// This ID is a pointer type that is guaranteed to be 8-byte aligned and thus
+  /// suitable for use in sets, maps, and other data structures that use the low
+  /// bits of pointers.
   ///
   /// Note that this requires the derived type provide a static \c AnalysisKey
   /// member called \c Key.
   ///
-  /// FIXME: The only reason the derived type needs to provide this rather than
-  /// this mixin providing it is due to broken implementations which cannot
-  /// correctly unique a templated static so that they have the same addresses
-  /// for each instantiation and are definitively emitted once for each
-  /// instantiation. The only currently known platform with this limitation are
-  /// Windows DLL builds, specifically building each part of LLVM as a DLL. If
-  /// we ever remove that build configuration, this mixin can provide the
-  /// static key as well.
+  /// FIXME: The only reason the mixin type itself can't declare the Key value
+  /// is that some compilers cannot correctly unique a templated static variable
+  /// so it has the same addresses in each instantiation. The only currently
+  /// known platform with this limitation is Windows DLL builds, specifically
+  /// building each part of LLVM as a DLL. If we ever remove that build
+  /// configuration, this mixin can provide the static key as well.
   static AnalysisKey *ID() { return &DerivedT::Key; }
 };
 
-/// A class template to provide analysis sets for IR units.
+/// This templated class represents "all analyses that operate over <a
+/// particular IR unit>" (e.g. a Function or a Module) in instances of
+/// PreservedAnalysis.
 ///
-/// Analyses operate on units of IR. It is useful to be able to talk about
-/// preservation of all analyses for a given unit of IR as a set. This class
-/// template can be used with the \c PreservedAnalyses API for that purpose and
-/// the \c AnalysisManager will automatically check and use this set to skip
-/// invalidation events.
+/// This lets a transformation say e.g. "I preserved all function analyses".
 ///
 /// Note that you must provide an explicit instantiation declaration and
 /// definition for this template in order to get the correct behavior on
 /// Windows. Otherwise, the address of SetKey will not be stable.
 template <typename IRUnitT>
 class AllAnalysesOn {
 public:
   static AnalysisSetKey *ID() { return &SetKey; }
 
 private:
   static AnalysisSetKey SetKey;
 };
 
 template <typename IRUnitT> AnalysisSetKey AllAnalysesOn<IRUnitT>::SetKey;
 
 extern template class AllAnalysesOn<Module>;
 extern template class AllAnalysesOn<Function>;
 
-/// \brief Manages a sequence of passes over units of IR.
+/// \brief Manages a sequence of passes over a particular unit of IR.
 ///
-/// A pass manager contains a sequence of passes to run over units of IR. It is
-/// itself a valid pass over that unit of IR, and when over some given IR will
-/// run each pass in sequence. This is the primary and most basic building
-/// block of a pass pipeline.
-///
-/// If it is run with an \c AnalysisManager<IRUnitT> argument, it will propagate
-/// that analysis manager to each pass it runs, as well as calling the analysis
-/// manager's invalidation routine with the PreservedAnalyses of each pass it
-/// runs.
+/// A pass manager contains a sequence of passes to run over a particular unit
+/// of IR (e.g. Functions, Modules). It is itself a valid pass over that unit of
+/// IR, and when run over some given IR will run each of its contained passes in
+/// sequence. Pass managers are the primary and most basic building block of a
+/// pass pipeline.
+///
+/// When you run a pass manager, you provide an \c AnalysisManager<IRUnitT>
+/// argument. The pass manager will propagate that analysis manager to each
+/// pass it runs, and will call the analysis manager's invalidation routine with
+/// the PreservedAnalyses of each pass it runs.
 template <typename IRUnitT,
           typename AnalysisManagerT = AnalysisManager<IRUnitT>,
           typename... ExtraArgTs>
 class PassManager : public PassInfoMixin<
                         PassManager<IRUnitT, AnalysisManagerT, ExtraArgTs...>> {
 public:
   /// \brief Construct a pass manager.
   ///
-  /// It can be passed a flag to get debug logging as the passes are run.
+  /// If \p DebugLogging is true, we'll log our progress to llvm::dbgs().
   explicit PassManager(bool DebugLogging = false) : DebugLogging(DebugLogging) {}
 
   // FIXME: These are equivalent to the default move constructor/move
   // assignment. However, using = default triggers linker errors due to the
   // explicit instantiations below. Find away to use the default and remove the
   // duplicated code here.
   PassManager(PassManager &&Arg)
       : Passes(std::move(Arg.Passes)),
         DebugLogging(std::move(Arg.DebugLogging)) {}
+
   PassManager &operator=(PassManager &&RHS) {
     Passes = std::move(RHS.Passes);
     DebugLogging = std::move(RHS.DebugLogging);
     return *this;
   }
 
-  /// \brief Run all of the passes in this manager over the IR.
+  /// \brief Run all of the passes in this manager over the given unit of IR.
+  /// ExtraArgs are passed to each pass.
   PreservedAnalyses run(IRUnitT &IR, AnalysisManagerT &AM,
                         ExtraArgTs... ExtraArgs) {
     PreservedAnalyses PA = PreservedAnalyses::all();
 
     if (DebugLogging)
       dbgs() << "Starting " << getTypeName<IRUnitT>() << " pass manager run.\n";
 
     for (unsigned Idx = 0, Size = Passes.size(); Idx != Size; ++Idx) {
       if (DebugLogging)
         dbgs() << "Running pass: " << Passes[Idx]->name() << " on "
                << IR.getName() << "\n";
 
       PreservedAnalyses PassPA = Passes[Idx]->run(IR, AM, ExtraArgs...);
 
       // Update the analysis manager as each pass runs and potentially
       // invalidates analyses.
       AM.invalidate(IR, PassPA);
 
-      // Finally, we intersect the preserved analyses to compute the aggregate
+      // Finally, intersect the preserved analyses to compute the aggregate
       // preserved set for this pass manager.
       PA.intersect(std::move(PassPA));
 
       // FIXME: Historically, the pass managers all called the LLVM context's
       // yield function here. We don't have a generic way to acquire the
       // context and it isn't yet clear what the right pattern is for yielding
       // in the new pass manager so it is currently omitted.
       //IR.getContext().yield();

 extern template class PassManager<Module>;
 /// \brief Convenience typedef for a pass manager over modules.
 typedef PassManager<Module> ModulePassManager;
 
 extern template class PassManager<Function>;
 /// \brief Convenience typedef for a pass manager over functions.
 typedef PassManager<Function> FunctionPassManager;
 
-/// \brief A generic analysis pass manager with lazy running and caching of
+/// \brief A container for analyses that lazily runs them and caches their
 /// results.
 ///
-/// This analysis manager can be used for any IR unit where the address of the
-/// IR unit sufficies as its identity. It manages the cache for a unit of IR via
-/// the address of each unit of IR cached.
+/// This class can manage analyses for any IR unit where the address of the IR
+/// unit sufficies as its identity.
 template <typename IRUnitT, typename... ExtraArgTs> class AnalysisManager {
 public:
   class Invalidator;
 
 private:
-  // Now that we've defined our invalidator, we can build types for the concept
-  // types.
+  // Now that we've defined our invalidator, we can define the concept types.
   typedef detail::AnalysisResultConcept<IRUnitT, PreservedAnalyses, Invalidator>
       ResultConceptT;
   typedef detail::AnalysisPassConcept<IRUnitT, PreservedAnalyses, Invalidator,
                                       ExtraArgTs...>
       PassConceptT;
 
-  /// \brief List of function analysis pass IDs and associated concept pointers.
+  /// \brief List of analysis pass IDs and associated concept pointers.
   ///
   /// Requires iterators to be valid across appending new entries and arbitrary
-  /// erases. Provides the analysis ID to enable finding iterators to a given entry
-  /// in maps below, and provides the storage for the actual result concept.
+  /// erases. Provides the analysis ID to enable finding iterators to a given
+  /// entry in maps below, and provides the storage for the actual result
+  /// concept.
   typedef std::list<std::pair<AnalysisKey *, std::unique_ptr<ResultConceptT>>>
       AnalysisResultListT;
 
   /// \brief Map type from IRUnitT pointer to our custom list type.
   typedef DenseMap<IRUnitT *, AnalysisResultListT> AnalysisResultListMapT;
 
   /// \brief Map type from a pair of analysis ID and IRUnitT pointer to an
-  /// iterator into a particular result list which is where the actual result
-  /// is stored.
+  /// iterator into a particular result list (which is where the actual analysis
+  /// result is stored).
   typedef DenseMap<std::pair<AnalysisKey *, IRUnitT *>,
                    typename AnalysisResultListT::iterator>
       AnalysisResultMapT;
 
 public:
   /// API to communicate dependencies between analyses during invalidation.
   ///
   /// When an analysis result embeds handles to other analysis results, it
   /// needs to be invalidated both when its own information isn't preserved and
-  /// if any of those embedded analysis results end up invalidated. We pass in
-  /// an \c Invalidator object from the analysis manager in order to let the
-  /// analysis results themselves define the dependency graph on the fly. This
-  /// avoids building an explicit data structure representation of the
+  /// when any of its embedded analysis results end up invalidated. We pass an
+  /// \c Invalidator object as an argument to \c invalidate() in order to let
+  /// the analysis results themselves define the dependency graph on the fly.
+  /// This lets us avoid building building an explicit representation of the
   /// dependencies between analysis results.
   class Invalidator {
   public:
     /// Trigger the invalidation of some other analysis pass if not already
-    /// handled and return whether it will in fact be invalidated.
+    /// handled and return whether it was in fact invalidated.
     ///
     /// This is expected to be called from within a given analysis result's \c
     /// invalidate method to trigger a depth-first walk of all inter-analysis
     /// dependencies. The same \p IR unit and \p PA passed to that result's \c
     /// invalidate method should in turn be provided to this routine.
     ///
-    /// The first time this is called for a given analysis pass, it will
-    /// trigger the corresponding result's \c invalidate method to be called.
-    /// Subsequent calls will use a cache of the results of that initial call.
-    /// It is an error to form cyclic dependencies between analysis results.
+    /// The first time this is called for a given analysis pass, it will call
+    /// the corresponding result's \c invalidate method.  Subsequent calls will
+    /// use a cache of the results of that initial call.  It is an error to form
+    /// cyclic dependencies between analysis results.
     ///
-    /// This returns true if the given analysis pass's result is invalid and
-    /// any dependecies on it will become invalid as a result.
+    /// This returns true if the given analysis's result is invalid. Any
+    /// dependecies on it will become invalid as a result.
     template <typename PassT>
     bool invalidate(IRUnitT &IR, const PreservedAnalyses &PA) {
       typedef detail::AnalysisResultModel<IRUnitT, PassT,
                                           typename PassT::Result,
                                           PreservedAnalyses, Invalidator>
           ResultModelT;
       return invalidateImpl<ResultModelT>(PassT::ID(), IR, PA);
     }
bool invalidateImpl(AnalysisKey *ID, IRUnitT &IR,
       auto RI = Results.find({ID, &IR});
       assert(RI != Results.end() &&
              "Trying to invalidate a dependent result that isn't in the "
              "manager's cache is always an error, likely due to a stale result "
              "handle!");
 
       auto &Result = static_cast<ResultT &>(*RI->second->second);
 
-      // Insert into the map whether the result should be invalidated and
-      // return that. Note that we cannot re-use IMapI and must do a fresh
-      // insert here as calling the invalidate routine could (recursively)
-      // insert things into the map making any iterator or reference invalid.
+      // Insert into the map whether the result should be invalidated and return
+      // that. Note that we cannot reuse IMapI and must do a fresh insert here,
+      // as calling invalidate could (recursively) insert things into the map,
+      // making any iterator or reference invalid.
       bool Inserted;
       std::tie(IMapI, Inserted) =
           IsResultInvalidated.insert({ID, Result.invalidate(IR, PA, *this)});
       (void)Inserted;
       assert(Inserted && "Should not have already inserted this ID, likely "
                          "indicates a dependency cycle!");
       return IMapI->second;
     }
 
     Invalidator(SmallDenseMap<AnalysisKey *, bool, 8> &IsResultInvalidated,
                 const AnalysisResultMapT &Results)
         : IsResultInvalidated(IsResultInvalidated), Results(Results) {}
 
     SmallDenseMap<AnalysisKey *, bool, 8> &IsResultInvalidated;
     const AnalysisResultMapT &Results;
   };
 
   /// \brief Construct an empty analysis manager.
   ///
-  /// A flag can be passed to indicate that the manager should perform debug
-  /// logging.
+  /// If \p DebugLogging is true, we'll log our progress to llvm::dbgs().
   AnalysisManager(bool DebugLogging = false) : DebugLogging(DebugLogging) {}
   AnalysisManager(AnalysisManager &&) = default;
   AnalysisManager &operator=(AnalysisManager &&) = default;
 
   /// \brief Returns true if the analysis manager has an empty results cache.
   bool empty() const {
     assert(AnalysisResults.empty() == AnalysisResultLists.empty() &&
            "The storage and index of analysis results disagree on how many "
            "there are!");
     return AnalysisResults.empty();
   }
 
-  /// \brief Clear any results for a single unit of IR.
+  /// \brief Clear any cached analysis results for a single unit of IR.
   ///
-  /// This doesn't invalidate but directly clears the results. It is useful
-  /// when the IR is being removed and we want to clear out all the memory
-  /// pinned for it.
+  /// This doesn't invalidate, but instead simply deletes, the relevant results.
+  /// It is useful when the IR is being removed and we want to clear out all the
+  /// memory pinned for it.
   void clear(IRUnitT &IR) {
     if (DebugLogging)
       dbgs() << "Clearing all analysis results for: " << IR.getName() << "\n";
 
     auto ResultsListI = AnalysisResultLists.find(&IR);
     if (ResultsListI == AnalysisResultLists.end())
       return;
-    // Clear the map pointing into the results list.
+    // Delete the map entries that point into the results list.
     for (auto &IDAndResult : ResultsListI->second)
       AnalysisResults.erase({IDAndResult.first, &IR});
 
     // And actually destroy and erase the results associated with this IR.
     AnalysisResultLists.erase(ResultsListI);
   }
 
-  /// \brief Clear the analysis result cache.
+  /// \brief Clear all analysis results cached by this AnalysisManager.
   ///
-  /// This routine allows cleaning up when the set of IR units itself has
-  /// potentially changed, and thus we can't even look up a a result and
-  /// invalidate it directly. Notably, this does *not* call invalidate
-  /// functions as there is nothing to be done for them.
+  /// Like \c clear(IRUnitT&), this doesn't invalidate the results; it simply
+  /// deletes them.  This lets you clean up the AnalysisManager when the set of
+  /// IR units itself has potentially changed, and thus we can't even look up a
+  /// a result and invalidate/clear it directly.
   void clear() {
     AnalysisResults.clear();
     AnalysisResultLists.clear();
   }
 
-  /// \brief Get the result of an analysis pass for this module.
+  /// \brief Get the result of an analysis pass for a given IR unit.
   ///
-  /// If there is not a valid cached result in the manager already, this will
-  /// re-run the analysis to produce a valid result.
+  /// Runs the analysis if a cached result is not available.
   template <typename PassT>
   typename PassT::Result &getResult(IRUnitT &IR, ExtraArgTs... ExtraArgs) {
     assert(AnalysisPasses.count(PassT::ID()) &&
            "This analysis pass was not registered prior to being queried");
     ResultConceptT &ResultConcept =
         getResultImpl(PassT::ID(), IR, ExtraArgs...);
     typedef detail::AnalysisResultModel<IRUnitT, PassT, typename PassT::Result,
                                         PreservedAnalyses, Invalidator>
         ResultModelT;
     return static_cast<ResultModelT &>(ResultConcept).Result;
   }
 
-  /// \brief Get the cached result of an analysis pass for this module.
+  /// \brief Get the cached result of an analysis pass for a given IR unit.
   ///
   /// This method never runs the analysis.
   ///
   /// \returns null if there is no cached result.
   template <typename PassT>
   typename PassT::Result *getCachedResult(IRUnitT &IR) const {
     assert(AnalysisPasses.count(PassT::ID()) &&
            "This analysis pass was not registered prior to being queried");
 
     ResultConceptT *ResultConcept = getCachedResultImpl(PassT::ID(), IR);
     if (!ResultConcept)
       return nullptr;
 
     typedef detail::AnalysisResultModel<IRUnitT, PassT, typename PassT::Result,
                                         PreservedAnalyses, Invalidator>
         ResultModelT;
     return &static_cast<ResultModelT *>(ResultConcept)->Result;
   }
 
   /// \brief Register an analysis pass with the manager.
   ///
-  /// The argument is a callable whose result is a pass. This allows passing in
-  /// a lambda to construct the pass.
+  /// The parameter is a callable whose result is an analysis pass. This allows
+  /// passing in a lambda to construct the analysis.
   ///
-  /// The pass type registered is the result type of calling the argument. If
-  /// that pass has already been registered, then the argument will not be
-  /// called and this function will return false. Otherwise, the pass type
-  /// becomes registered, with the instance provided by calling the argument
-  /// once, and this function returns true.
-  ///
-  /// While this returns whether or not the pass type was already registered,
-  /// there in't an independent way to query that as that would be prone to
-  /// risky use when *querying* the analysis manager. Instead, the only
-  /// supported use case is avoiding duplicate registry of an analysis. This
-  /// interface also lends itself to minimizing the number of times we have to
-  /// do lookups for analyses or construct complex passes only to throw them
-  /// away.
+  /// The analysis type to register is the type returned by calling the \c
+  /// PassBuilder argument. If that type has already been registered, then the
+  /// argument will not be called and this function will return false.
+  /// Otherwise, we register the analysis returned by calling \c PassBuilder(),
+  /// and this function returns true.
+  ///
+  /// (Note: Although the return value of this function indicates whether or not
+  /// an analysis was previously registered, there intentionally isn't a way to
+  /// query this directly.  Instead, you should just register all the analyses
+  /// you might want and let this class run them lazily.  This idiom lets us
+  /// minimize the number of times we have to look up analyses in our
+  /// hashtable.)
   template <typename PassBuilderT>
   bool registerPass(PassBuilderT &&PassBuilder) {
     typedef decltype(PassBuilder()) PassT;
     typedef detail::AnalysisPassModel<IRUnitT, PassT, PreservedAnalyses,
                                       Invalidator, ExtraArgTs...>
         PassModelT;
 
     auto &PassPtr = AnalysisPasses[PassT::ID()];
     if (PassPtr)
       // Already registered this pass type!
       return false;
 
     // Construct a new model around the instance returned by the builder.
     PassPtr.reset(new PassModelT(PassBuilder()));
     return true;
   }
 
   /// \brief Invalidate a specific analysis pass for an IR module.
   ///
-  /// Note that the analysis result can disregard invalidation.
+  /// Note that the analysis result can disregard invalidation, if it determines
+  /// it is in fact still valid.
   template <typename PassT> void invalidate(IRUnitT &IR) {
     assert(AnalysisPasses.count(PassT::ID()) &&
            "This analysis pass was not registered prior to being invalidated");
     invalidateImpl(PassT::ID(), IR);
   }
 
-  /// \brief Invalidate analyses cached for an IR unit.
+  /// \brief Invalidate cached analyses for an IR unit.
   ///
   /// Walk through all of the analyses pertaining to this unit of IR and
-  /// invalidate them unless they are preserved by the PreservedAnalyses set.
+  /// invalidate them, unless they are preserved by the PreservedAnalyses set.
   void invalidate(IRUnitT &IR, const PreservedAnalyses &PA) {
     // We're done if all analyses on this IR unit are preserved.
     if (PA.allAnalysesInSetPreserved<AllAnalysesOn<IRUnitT>>())
       return;
 
     if (DebugLogging)
       dbgs() << "Invalidating all non-preserved analyses for: " << IR.getName()
              << "\n";
 
-    // Track whether each pass's result is invalidated. Memoize the results
-    // using the IsResultInvalidated map.
+    // Track whether each analysis's result is invalidated in
+    // IsResultInvalidated.
     SmallDenseMap<AnalysisKey *, bool, 8> IsResultInvalidated;
     Invalidator Inv(IsResultInvalidated, AnalysisResults);
     AnalysisResultListT &ResultsList = AnalysisResultLists[&IR];
     for (auto &AnalysisResultPair : ResultsList) {
       // This is basically the same thing as Invalidator::invalidate, but we
       // can't call it here because we're operating on the type-erased result.
       // Moreover if we instead called invalidate() directly, it would do an
       // unnecessary look up in ResultsList.
       AnalysisKey *ID = AnalysisResultPair.first;
       auto &Result = *AnalysisResultPair.second;
 
       auto IMapI = IsResultInvalidated.find(ID);
       if (IMapI != IsResultInvalidated.end())
         // This result was already handled via the Invalidator.
         continue;
 
       // Try to invalidate the result, giving it the Invalidator so it can
       // recursively query for any dependencies it has and record the result.
-      // Note that we cannot re-use 'IMapI' here or pre-insert the ID as the
-      // invalidate method may insert things into the map as well, invalidating
-      // any iterator or pointer.
+      // Note that we cannot reuse 'IMapI' here or pre-insert the ID, as
+      // Result.invalidate may insert things into the map, invalidating our
+      // iterator.
       bool Inserted =
           IsResultInvalidated.insert({ID, Result.invalidate(IR, PA, Inv)})
               .second;
       (void)Inserted;
       assert(Inserted && "Should never have already inserted this ID, likely "
                          "indicates a cycle!");
     }
 
private:
   /// Provides linear time removal of all analysis results for a function and
   /// the ultimate storage for a particular cached analysis result.
   AnalysisResultListMapT AnalysisResultLists;
 
   /// \brief Map from an analysis ID and function to a particular cached
   /// analysis result.
   AnalysisResultMapT AnalysisResults;
 
-  /// \brief A flag indicating whether debug logging is enabled.
+  /// \brief Indicates whether we log to \c llvm::dbgs().
   bool DebugLogging;
 };
 
 extern template class AnalysisManager<Module>;
 /// \brief Convenience typedef for the Module analysis manager.
 typedef AnalysisManager<Module> ModuleAnalysisManager;
 
 extern template class AnalysisManager<Function>;