[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

882–883

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.

885–895

All of this is fantastic though. =]

903

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.

904

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

994–995

s/update/invalidate/

1132

on Function units -> on Functions

1239

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

882–883

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

1239

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

882–883

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

882–883

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

882–883

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

882–883

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

903

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

904

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.

1239

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 &InnerAM) : InnerAM(&InnerAM) {}
-    Result(Result &&Arg) : InnerAM(std::move(Arg.InnerAM)) {
-      // 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.InnerAM = nullptr;
-    }
+    explicit Result(AnalysisManagerT &AM) : AM(&AM) {}
+    Result(Result &&RHS) : AM(std::move(RHS.AM)) { RHS.AM = nullptr; }
     Result &operator=(Result &&RHS) {
-      InnerAM = RHS.InnerAM;
-      // 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.InnerAM = nullptr;
+      AM = RHS.AM;
+      RHS.AM = nullptr;
       return *this;
     }
     ~Result() {
-      // InnerAM is cleared in a moved from state where there is nothing to do.
-      if (!InnerAM)
-        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.
-      InnerAM->clear();
+      if (AM)
+        AM->clear();
     }
 
     /// \brief Accessor for the analysis manager.
-    AnalysisManagerT &getManager() { return *InnerAM; }
+    AnalysisManagerT &getManager() { return *AM; }
 
-    /// \brief Handler for invalidation of the outer IR unit.
-    ///
-    /// If this analysis itself is preserved, then we assume that the set of \c
-    /// IR units that the inner analysis manager controls hasn't changed and
-    /// thus we don't need to invalidate *all* cached data associated with any
-    /// \c IRUnitT* in the \c AnalysisManagerT.
-    ///
-    /// Regardless of whether this analysis is marked as preserved, all of the
-    /// analyses in the \c AnalysisManagerT are potentially invalidated (for
-    /// the relevant inner set of their IR units) based on the set of preserved
-    /// analyses.
+    /// \brief Handler for invalidation of the "larger" IR unit, \c IRUnitT.
     ///
-    /// Because this needs to understand the mapping from one IR unit to an
-    /// inner IR unit, this method isn't defined in the primary template.
-    /// Instead, each specialization of this template will need to provide an
-    /// explicit specialization of this method to handle that particular pair
-    /// of IR unit and inner AnalysisManagerT.
+    /// 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 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 &Inv);
 
   private:
-    AnalysisManagerT *InnerAM;
+    AnalysisManagerT *AM;
   };
 
   explicit InnerAnalysisManagerProxy(AnalysisManagerT &InnerAM)
       : InnerAM(&InnerAM) {}
 
   /// \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...> &AM,
              ExtraArgTs...) {
     return Result(*InnerAM);
   }
 
 private:
   friend AnalysisInfoMixin<
       InnerAnalysisManagerProxy<AnalysisManagerT, IRUnitT>>;
bool FunctionAnalysisManagerModuleProxy::Result::invalidate(
     Module &M, const PreservedAnalyses &PA,
     ModuleAnalysisManager::Invalidator &Inv);
 
 // Ensure the \c FunctionAnalysisManagerModuleProxy is provided as an extern
 // template.
 extern template class InnerAnalysisManagerProxy<FunctionAnalysisManager,
                                                 Module>;
 
-/// \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.
-///
-/// The invalidation provided by this proxy involves tracking when an
-/// invalidation event in the outer analysis manager needs to trigger an
-/// invalidation of a particular analysis on this IR unit.
-///
-/// Because outer analyses aren't invalidated while these IR units are being
-/// precessed, we have to register and handle these as deferred invalidation
-/// events.
+/// 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;
     }
 
     /// Register a deferred invalidation event for when the outer analysis
     /// manager processes its invalidations.

 };
 
 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.preserveSet<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 &&...) {
     auto PA = PreservedAnalyses::all();
     PA.abandon<AnalysisT>();
     return PA;
   }
 };
 
-/// \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

llvm/lib/Analysis/LoopPassManager.cpp

 template class AnalysisManager<Loop>;
 template class InnerAnalysisManagerProxy<LoopAnalysisManager, Function>;
 template class OuterAnalysisManagerProxy<FunctionAnalysisManager, Loop>;
 
 template <>
 bool LoopAnalysisManagerFunctionProxy::Result::invalidate(
     Function &F, const PreservedAnalyses &PA,
     FunctionAnalysisManager::Invalidator &Inv) {
-  // If this proxy isn't marked as preserved, the set of Function objects in
-  // the module may have changed. We therefore can't call
-  // InnerAM->invalidate(), because any pointers to Functions it has may be
-  // stale.
+  // If this proxy isn't marked as preserved, the set of Function objects in the
+  // module may have changed. We therefore can't call AM->invalidate(), because
+  // any pointers to Functions it has may be stale.
   auto PAC = PA.getChecker<LoopAnalysisManagerFunctionProxy>();
   if (!PAC.preserved() && !PAC.preservedSet<AllAnalysesOn<Loop>>())
-    InnerAM->clear();
+    AM->clear();
 
   // FIXME: Proper suppor for invalidation isn't yet implemented for the LPM.
 
   // Return false to indicate that this result is still a valid proxy.
   return false;
 }
 }
 

llvm/lib/IR/PassManager.cpp

bool FunctionAnalysisManagerModuleProxy::Result::invalidate(
   //
   // Note that in order to preserve this proxy, a module pass must ensure that
   // the FAM has been completely updated to handle the deletion of functions.
   // Specifically, any FAM-cached results for those functions need to have been
   // forcibly cleared. When preserved, this proxy will only invalidate results
   // cached on functions *still in the module* at the end of the module pass.
   auto PAC = PA.getChecker<FunctionAnalysisManagerModuleProxy>();
   if (!PAC.preserved() && !PAC.preservedSet<AllAnalysesOn<Module>>()) {
-    InnerAM->clear();
+    AM->clear();
     return true;
   }
 
   // Directly check if the relevant set is preserved.
   bool AreFunctionAnalysesPreserved =
       PA.allAnalysesInSetPreserved<AllAnalysesOn<Function>>();
 
   // Now walk all the functions to see if any inner analysis invalidation is
   // necessary.
   for (Function &F : M) {
     Optional<PreservedAnalyses> FunctionPA;
 
     // Check to see whether the preserved set needs to be pruned based on
     // module-level analysis invalidation that triggers deferred invalidation
     // registered with the outer analysis manager proxy for this function.
     if (auto *OuterProxy =
-            InnerAM->getCachedResult<ModuleAnalysisManagerFunctionProxy>(F))
+            AM->getCachedResult<ModuleAnalysisManagerFunctionProxy>(F))
       for (const auto &OuterInvalidationPair :
            OuterProxy->getOuterInvalidations()) {
         AnalysisKey *OuterAnalysisID = OuterInvalidationPair.first;
         const auto &InnerAnalysisIDs = OuterInvalidationPair.second;
         if (Inv.invalidate(OuterAnalysisID, M, PA)) {
           if (!FunctionPA)
             FunctionPA = PA;
           for (AnalysisKey *InnerAnalysisID : InnerAnalysisIDs)
             FunctionPA->abandon(InnerAnalysisID);
         }
       }
 
     // Check if we needed a custom PA set, and if so we'll need to run the
     // inner invalidation.
     if (FunctionPA) {
-      InnerAM->invalidate(F, *FunctionPA);
+      AM->invalidate(F, *FunctionPA);
       continue;
     }
 
     // Otherwise we only need to do invalidation if the original PA set didn't
     // preserve all function analyses.
     if (!AreFunctionAnalysesPreserved)
-      InnerAM->invalidate(F, PA);
+      AM->invalidate(F, PA);
   }
 
   // Return false to indicate that this result is still a valid proxy.
   return false;
 }
 }
 
 AnalysisSetKey PreservedAnalyses::AllAnalysesKey;