[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

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

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

[jlebar]

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

llvm/include/llvm/IR/PassManager.h

 /// A special type used by analysis passes to provide an address that
 /// 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 {};
 
-/// \brief An abstract set of preserved analyses following a transformation pass
-/// run.
+/// \brief A set of analyses that are preserved following a run of a
+/// transformation pass.
 ///
 /// When a transformation pass is run, it can return a set of analyses whose
 /// results were preserved by that transformation. The default set is "none",
 /// and preserving analyses must be done explicitly.
 ///
-/// There is also an explicit all state which can be used (for example) when
+/// There is also an explicit 'all' state which can be used (for example) when
 /// the IR is not mutated at all.
 class PreservedAnalyses {
 public:
   /// \brief Convenience factory function for the empty preserved set.
   static PreservedAnalyses none() { return PreservedAnalyses(); }
 
   /// \brief Construct a special preserved set that preserves all passes.
   static PreservedAnalyses all() {
     PreservedAnalyses PA;
     PA.PreservedAnalysisIDs.insert(&AllAnalysesKey);
     return PA;
   }
 
   /// \brief Mark a particular pass as preserved, adding it to the set.
   template <typename PassT> void preserve() { preserve(PassT::ID()); }
 
-  /// \brief Mark an abstract ID as preserved, adding it to the set.
+  /// \brief Given an analysis's ID, mark the analysis as preserved, adding it
+  /// to the set.
   void preserve(AnalysisKey *ID) {
     if (!areAllPreserved())
       PreservedAnalysisIDs.insert(ID);
   }
 
   /// \brief Intersect this set with another in place.
   ///
   /// This is a mutating operation on this preserved set, removing all
for (auto ID : PreservedAnalysisIDs)
         PreservedAnalysisIDs.erase(ID);
   }
 
   /// \brief Query whether a pass is marked as preserved by this set.
   template <typename PassT> bool preserved() const {
     return preserved(PassT::ID());
   }
 
-  /// \brief Query whether an abstract pass ID is marked as preserved by this
-  /// set.
+  /// \brief Given a pointer to its AnalysisKey, query whether a pass is marked
+  /// as preserved by this set.
   bool preserved(AnalysisKey *ID) const {
     return PreservedAnalysisIDs.count(&AllAnalysesKey) ||
            PreservedAnalysisIDs.count(ID);
   }
 
-  /// \brief Query whether all of the analyses in the set are preserved.
+  /// \brief Query whether all of the analyses in a given set are preserved.
   bool preserved(const PreservedAnalyses& Arg) {
     if (Arg.areAllPreserved())
       return areAllPreserved();
     for (auto ID : Arg.PreservedAnalysisIDs)
       if (!preserved(ID))
         return false;
     return true;
   }
 
   /// \brief Test whether all passes are preserved.
   ///
-  /// This is used primarily to optimize for the case of no changes which will
-  /// common in many scenarios.
+  /// This is used primarily to optimize for the common case of a transformation
+  /// which makes no changes to the IR.
   bool areAllPreserved() const {
     return PreservedAnalysisIDs.count(&AllAnalysesKey);
   }
 
 private:
   // A special key used to indicate all analyses.
   static AnalysisKey AllAnalysesKey;
 
   SmallPtrSet<AnalysisKey *, 2> PreservedAnalysisIDs;
 };
 
 // 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.
 ///
-/// 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
+/// Each analysis operates over a particular unit of IR, and it is useful to be
+/// able to talk about preserving all analyses that operate over e.g. functions.
+/// This class 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.
 ///
 /// 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 AnalysisKey *ID() { return &SetKey; }
 
 private:
   static AnalysisKey SetKey;
 };
 
 template <typename IRUnitT> AnalysisKey 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 DebugLogging is true, we'll print messages as each pass is run.
   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.
   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) {
       AnalysisKey *ID = PassT::ID();
 
       // If we've already visited this pass, return true if it was invalidated
       // and false otherwise.
       auto IMapI = IsResultInvalidated.find(ID);
       if (IMapI != IsResultInvalidated.end())
         return IMapI->second;
 
       // Otherwise look up the result object.
       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!");
 
       typedef detail::AnalysisResultModel<IRUnitT, PassT,
                                           typename PassT::Result,
                                           PreservedAnalyses, Invalidator>
           ResultModelT;
       auto &ResultModel = static_cast<ResultModelT &>(*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, ResultModel.invalidate(IR, PA, *this)});
       (void)Inserted;
       assert(Inserted && "Should not have already inserted this ID, likely "
                          "indicates a dependency cycle!");
       return IMapI->second;
     }
 
   private:
     friend class AnalysisManager;
 
     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 DebugLogging is true, the manager will log its progress to
+  /// \c 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) {
     // Short circuit for common cases of all analyses being preserved.
     if (PA.areAllPreserved() || PA.preserved<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>;