[llvm] r195241 - [PM] Add the preservation system to the new pass manager.

[Duncan P. N. Exon Smith]

I'm starting to review the new pass manager implementation.  I'm
reading through files in the tree, and I'm not sure how best to deal
with review comments as they come up.

Should I start a new thread for each, bring the commit back from the
dead (like I've done here), make the changes myself in tree and let
you review those, or collect my comments together locally and start
a new thread once I'm "finished" (which might not be soon)?

On 2013-Nov-20, at 03:31, Chandler Carruth <chandlerc at gmail.com> wrote:

> Author: chandlerc
> Date: Wed Nov 20 05:31:50 2013
> New Revision: 195241
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=195241&view=rev
> Log:
> [PM] Add the preservation system to the new pass manager.
> 
> This adds a new set-like type which represents a set of preserved
> analysis passes. The set is managed via the opaque PassT::ID() void*s.
> The expected convenience templates for interacting with specific passes
> are provided. It also supports a symbolic "all" state which is
> represented by an invalid pointer in the set. This state is nicely
> saturating as it comes up often. Finally, it supports intersection which
> is used when finding the set of preserved passes after N different
> transforms.
> 
> The pass API is then changed to return the preserved set rather than
> a bool. This is much more self-documenting than the previous system.
> Returning "none" is a conservatively correct solution just like
> returning "true" from todays passes and not marking any passes as
> preserved. Passes can also be dynamically preserved or not throughout
> the run of the pass, and whatever gets returned is the binding state.
> Finally, preserving "all" the passes is allowed for no-op transforms
> that simply can't harm such things.
> 
> Finally, the analysis managers are changed to instead of blindly
> invalidating all of the analyses, invalidate those which were not
> preserved. This should rig up all of the basic preservation
> functionality. This also correctly combines the preservation moving up
> from one IR-layer to the another and the preservation aggregation across
> N pass runs. Still to go is incrementally correct invalidation and
> preservation across IR layers incrementally during N pass runs. That
> will wait until we have a device for even exposing analyses across IR
> layers.
> 
> While the core of this change is obvious, I'm not happy with the current
> testing, so will improve it to cover at least some of the invalidation
> that I can test easily in a subsequent commit.
> 
> Modified:
>    llvm/trunk/include/llvm/IR/PassManager.h
>    llvm/trunk/lib/IR/PassManager.cpp
>    llvm/trunk/unittests/IR/PassManagerTest.cpp
> 
> Modified: llvm/trunk/include/llvm/IR/PassManager.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/IR/PassManager.h?rev=195241&r1=195240&r2=195241&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/IR/PassManager.h (original)
> +++ llvm/trunk/include/llvm/IR/PassManager.h Wed Nov 20 05:31:50 2013
> @@ -36,6 +36,7 @@
> //===----------------------------------------------------------------------===//
> 
> #include "llvm/ADT/DenseMap.h"
> +#include "llvm/ADT/SmallPtrSet.h"
> #include "llvm/ADT/polymorphic_ptr.h"
> #include "llvm/Support/type_traits.h"
> #include "llvm/IR/Function.h"
> @@ -48,6 +49,106 @@ namespace llvm {
> class Module;
> class Function;
> 
> +/// \brief An abstract set of preserved analyses following a transformation pass
> +/// run.
> +///
> +/// 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
> +/// 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.PreservedPassIDs.insert((void *)AllPassesID);
> +    return PA;
> +  }
> +
> +  PreservedAnalyses &operator=(PreservedAnalyses Arg) {
> +    swap(Arg);
> +    return *this;
> +  }
> +
> +  void swap(PreservedAnalyses &Arg) {
> +    PreservedPassIDs.swap(Arg.PreservedPassIDs);
> +  }
> +
> +  /// \brief Mark a particular pass as preserved, adding it to the set.
> +  template <typename PassT> void preserve() {
> +    if (!areAllPreserved())
> +      PreservedPassIDs.insert(PassT::ID());
> +  }
> +
> +  /// \brief Intersect this set with another in place.
> +  ///
> +  /// This is a mutating operation on this preserved set, removing all
> +  /// preserved passes which are not also preserved in the argument.
> +  void intersect(const PreservedAnalyses &Arg) {
> +    if (Arg.areAllPreserved())
> +      return;
> +    if (areAllPreserved()) {
> +      PreservedPassIDs = Arg.PreservedPassIDs;
> +      return;
> +    }
> +    for (SmallPtrSet<void *, 2>::const_iterator I = PreservedPassIDs.begin(),
> +                                                E = PreservedPassIDs.end();
> +         I != E; ++I)
> +      if (!Arg.PreservedPassIDs.count(*I))
> +        PreservedPassIDs.erase(*I);

Interesting that SmallPtrSet doesn't let you erase by iterator.  Is
that an API oversight, or is it intentional?

> +  }
> +
> +#if LLVM_HAS_RVALUE_REFERENCES
> +  /// \brief Intersect this set with a temporary other set in place.
> +  ///
> +  /// This is a mutating operation on this preserved set, removing all
> +  /// preserved passes which are not also preserved in the argument.
> +  void intersect(PreservedAnalyses &&Arg) {
> +    if (Arg.areAllPreserved())
> +      return;
> +    if (areAllPreserved()) {
> +      PreservedPassIDs = std::move(Arg.PreservedPassIDs);
> +      return;
> +    }
> +    for (SmallPtrSet<void *, 2>::const_iterator I = PreservedPassIDs.begin(),
> +                                                E = PreservedPassIDs.end();
> +         I != E; ++I)
> +      if (!Arg.PreservedPassIDs.count(*I))
> +        PreservedPassIDs.erase(*I);
> +  }
> +#endif
> +
> +  /// \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.
> +  bool preserved(void *PassID) const {
> +    return PreservedPassIDs.count((void *)AllPassesID) ||
> +           PreservedPassIDs.count(PassID);
> +  }

Shouldn't these functions be spelled "isPreserved()"?

> +
> +private:
> +  // Note that this must not be -1 or -2 as those are already used by the
> +  // SmallPtrSet.
> +  static const uintptr_t AllPassesID = (intptr_t)-3;
> +
> +  bool areAllPreserved() const { return PreservedPassIDs.count((void *)AllPassesID); }
> +
> +  SmallPtrSet<void *, 2> PreservedPassIDs;
> +};
> +
> +inline void swap(PreservedAnalyses &LHS, PreservedAnalyses &RHS) {
> +  LHS.swap(RHS);
> +}
> +