[PM/AA] Teach the new pass manager to use pass-by-lambda for registering analysis passes, support pre-registering analyses, and use that to implement parsing and pre-registering a custom alias analysis pipeline.

[chandlerc]

Summary

With this its possible to configure the particular alias analysis
pipeline used by the AAManager from the commandline of opt. I've updated
the test to show this effectively in use to build a pipeline including
basic-aa as part of it.

My big question for reviewers are around the APIs that are used to
expose this functionality. Are folks happy with pass-by-lambda to do
pass registration? Are folks happy with pre-registering analyses as
a way to inject customized instances of an analysis while still using
the registry for the general case?

Other thoughts of course welcome. The next round of patches will be to
add the rest of the alias analyses into the new pass manager and wire
them up here so that they can be used from opt. This will require
extending the (somewhate limited) functionality of AAManager w.r.t.
module passes.

[bkramer]

include/llvm/Passes/PassBuilder.h

101

s/provide/&d/

lib/Passes/PassBuilder.cpp

108

What's being captured by the lambda here?

[chandlerc]

lib/Passes/PassBuilder.cpp

108

There are members of PassBuilder that may be used when constructing passes.

[pete]

So i'm not against the lambda's if they are required, but I can't really see why we need them instead of say, a function pointer?

Eg, instead of

MAM.registerPass([&] { return FunctionAnalysisManagerModuleProxy(FAM); });

do

MAM.registerPass(&FunctionAnalysisManagerModuleProxy)

Also, looks like the lambda (or function pointer), is being used to make sure we don't construct a pass twice. Why not jus make that an error to register the same pass twice? Is that something we can say is a condition of using the new pass manager?

[chandlerc]

In D17259#352996, @pete wrote:

So i'm not against the lambda's if they are required, but I can't really see why we need them instead of say, a function pointer?

Eg, instead of

MAM.registerPass([&] { return FunctionAnalysisManagerModuleProxy(FAM); });

do

MAM.registerPass(&FunctionAnalysisManagerModuleProxy)

This doesn't compile because that isn't a function. It is a constructor that builds a temporary.

The routine however *would* be fine with a function pointer if we had such a thing. The routine is being generic and accepting any callable. Callers can use a function pointer or a lambda or whatever else. A lambda has the nice property of being a maximally generic way to pass in a lazy-evaluated expression.

Also, looks like the lambda (or function pointer), is being used to make sure we don't construct a pass twice. Why not jus make that an error to register the same pass twice? Is that something we can say is a condition of using the new pass manager?

Well, the previous code did exactly that. ;] The patch description (i thought) explained why I am changing it: we need some way to explicitly handle the AAManager's registration outside of the generic registerFunctionAnalyses routine. The best idea I had for an interface was to pre-register the analyses which require custom logic, and then register the remaining ones with the generic logic in PassBuilder::register*Analyses.

The code to use this facility is also included in the patch. See the code in NewPMDriver.cpp where we call registerFunctionAnalyses. If you add back the assert that this patch removes from registerPass, it will fire because we will register AAManager twice.

[chandlerc]

Any further thoughts here? I don't want this to get bogged down, as it seems pretty simple and is in my critical path.

[reames]

Have to admit, I don't really understand the motivation for this. Having said that, I don't see anything obviously wrong, and I don't want you blocked on this, so LGTM.

lib/Passes/PassBuilder.cpp

224

Add an #undef after.

[chandlerc]

In D17259#354300, @reames wrote:

Have to admit, I don't really understand the motivation for this. Having said that, I don't see anything obviously wrong, and I don't want you blocked on this, so LGTM.

Well, I think its important that the motivation makes sense. The key thing is that we want command line tools like 'opt' to be able to customize the AAManager (potentially using the PassBuilder to do so!), and use that customized AAManager instead of whatever the default would be while still getting the default registration of every other analysis. The mechanism I ended up with was to allow callers of registerFunctionAnalyses to pre-register analyses if they desire.

lib/Passes/PassBuilder.cpp

224

The .def file should be #undefing everything it uses. I'll make sure I got that right (I think I didn't).

[pete]

Thanks for the explanation. Looks like I should mostly have read it better the first time, but I'm more or less up to speed now.

If you wanted a LGTM from me too then you have it. Wasn't sure if you wanted an ok or not after Philip's LGTM :)

Cheers
Pete

[mcrosier]

LGTM, per Pete and Philip.

[chandlerc]

Fix typo spotted by Ben.

[chandlerc]

Thanks all, landing. Please let me know Philip if there is more to explain here. I'm happy to revisit this design and other aspects of this as needed.

include/llvm/IR/PassManager.h

typename PassT::Result *getCachedResult(IRUnitT &IR) const {
 
     typedef detail::AnalysisResultModel<IRUnitT, PassT, typename PassT::Result>
         ResultModelT;
     return &static_cast<ResultModelT *>(ResultConcept)->Result;
   }
 
   /// \brief Register an analysis pass with the manager.
   ///
-  /// This provides an initialized and set-up analysis pass to the analysis
-  /// manager. Whomever is setting up analysis passes must use this to populate
-  /// the manager with all of the analysis passes available.
-  template <typename PassT> void registerPass(PassT Pass) {
-    assert(!AnalysisPasses.count(PassT::ID()) &&
-           "Registered the same analysis pass twice!");
+  /// The argument is a callable whose result is a pass. This allows passing in
+  /// a lambda to construct the pass.
+  ///
+  /// 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.
+  template <typename PassBuilderT> bool registerPass(PassBuilderT PassBuilder) {
+    typedef decltype(PassBuilder()) PassT;
     typedef detail::AnalysisPassModel<IRUnitT, PassT> PassModelT;
-    AnalysisPasses[PassT::ID()].reset(new PassModelT(std::move(Pass)));
+
+    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.
   template <typename PassT> void invalidate(IRUnitT &IR) {
     assert(AnalysisPasses.count(PassT::ID()) &&
            "This analysis pass was not registered prior to being invalidated");

include/llvm/Passes/PassBuilder.h

 #ifndef LLVM_PASSES_PASSBUILDER_H
 #define LLVM_PASSES_PASSBUILDER_H
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Analysis/CGSCCPassManager.h"
 #include "llvm/IR/PassManager.h"
 
 namespace llvm {
+class AAManager;
 class TargetMachine;
 
 /// \brief This class provides access to building LLVM's passes.
 ///
 /// It's members provide the baseline state available to passes during their
 /// construction. The \c PassRegistry.def file specifies how to construct all
 /// of the built-in passes, and those may reference these members during
 /// construction.
 class PassBuilder {
   TargetMachine *TM;
 
 public:
   explicit PassBuilder(TargetMachine *TM = nullptr) : TM(TM) {}
 
   /// \brief Registers all available module analysis passes.
   ///
   /// This is an interface that can be used to populate a \c
   /// ModuleAnalysisManager with all registered module analyses. Callers can
-  /// still manually register any additional analyses.
+  /// still manually register any additional analyses. Callers can also
+  /// pre-register analyses and this will not override those.
   void registerModuleAnalyses(ModuleAnalysisManager &MAM);
 
   /// \brief Registers all available CGSCC analysis passes.
   ///
   /// This is an interface that can be used to populate a \c CGSCCAnalysisManager
   /// with all registered CGSCC analyses. Callers can still manually register any
-  /// additional analyses.
+  /// additional analyses. Callers can also pre-register analyses and this will
+  /// not override those.
   void registerCGSCCAnalyses(CGSCCAnalysisManager &CGAM);
 
   /// \brief Registers all available function analysis passes.
   ///
   /// This is an interface that can be used to populate a \c
   /// FunctionAnalysisManager with all registered function analyses. Callers can
-  /// still manually register any additional analyses.
+  /// still manually register any additional analyses. Callers can also
+  /// pre-register analyses and this will not override those.
   void registerFunctionAnalyses(FunctionAnalysisManager &FAM);
 
   /// \brief Parse a textual pass pipeline description into a \c ModulePassManager.
   ///
   /// The format of the textual pass pipeline description looks something like:
   ///
   ///   module(function(instcombine,sroa),dce,cgscc(inliner,function(...)),...)
   ///
public:
   /// This shortcut is especially useful for debugging and testing small pass
   /// combinations. Note that these shortcuts don't introduce any other magic. If
   /// the sequence of passes aren't all the exact same kind of pass, it will be
   /// an error. You cannot mix different levels implicitly, you must explicitly
   /// form a pass manager in which to nest passes.
   bool parsePassPipeline(ModulePassManager &MPM, StringRef PipelineText,
                          bool VerifyEachPass = true, bool DebugLogging = false);
 
+  /// Parse a textual alias analysis pipeline into the provided AA manager.
+  ///
+  /// The format of the textual AA pipeline is a comma separated list of AA
+  /// pass names:
+  ///
+  ///   basic-aa,globals-aa,...
+  ///
+  /// The AA manager is set up such that the provided alias analyses are tried
+  /// in the order specified. See the \c AAManaager documentation for details
+  /// about the logic used. This routine just provides the textual mapping
+  /// between AA names and the analyses to register with the manager.
+  ///
+  /// Returns false if the text cannot be parsed cleanly. The specific state of
+  /// the \p AA manager is unspecified if such an error is encountered and this
+  /// returns false.
+  bool parseAAPipeline(AAManager &AA, StringRef PipelineText);
+
 private:
   bool parseModulePassName(ModulePassManager &MPM, StringRef Name);
   bool parseCGSCCPassName(CGSCCPassManager &CGPM, StringRef Name);
   bool parseFunctionPassName(FunctionPassManager &FPM, StringRef Name);
+  bool parseAAPassName(AAManager &AA, StringRef Name);
   bool parseFunctionPassPipeline(FunctionPassManager &FPM,
                                  StringRef &PipelineText, bool VerifyEachPass,
                                  bool DebugLogging);
   bool parseCGSCCPassPipeline(CGSCCPassManager &CGPM, StringRef &PipelineText,
                               bool VerifyEachPass, bool DebugLogging);
   bool parseModulePassPipeline(ModulePassManager &MPM, StringRef &PipelineText,
                                bool VerifyEachPass, bool DebugLogging);
 };
 
 }
 
 #endif

lib/Passes/PassBuilder.cpp

 };
 
 char NoOpFunctionAnalysis::PassID;
 
 } // End anonymous namespace.
 
 void PassBuilder::registerModuleAnalyses(ModuleAnalysisManager &MAM) {
 #define MODULE_ANALYSIS(NAME, CREATE_PASS) \
-  MAM.registerPass(CREATE_PASS);
+  MAM.registerPass([&] { return CREATE_PASS; });
 #include "PassRegistry.def"
 }
 
 void PassBuilder::registerCGSCCAnalyses(CGSCCAnalysisManager &CGAM) {
 #define CGSCC_ANALYSIS(NAME, CREATE_PASS) \
-  CGAM.registerPass(CREATE_PASS);
+  CGAM.registerPass([&] { return CREATE_PASS; });
 #include "PassRegistry.def"
 }
 
 void PassBuilder::registerFunctionAnalyses(FunctionAnalysisManager &FAM) {
 #define FUNCTION_ANALYSIS(NAME, CREATE_PASS) \
-  FAM.registerPass(CREATE_PASS);
+  FAM.registerPass([&] { return CREATE_PASS; });
 #include "PassRegistry.def"
 }
 
 #ifndef NDEBUG
 static bool isModulePassName(StringRef Name) {
 #define MODULE_PASS(NAME, CREATE_PASS) if (Name == NAME) return true;
 #define MODULE_ANALYSIS(NAME, CREATE_PASS)                                     \
   if (Name == "require<" NAME ">" || Name == "invalidate<" NAME ">")           \
if (Name == "invalidate<" NAME ">") {                                        \
     FPM.addPass(InvalidateAnalysisPass<decltype(CREATE_PASS)>());              \
     return true;                                                               \
   }
 #include "PassRegistry.def"
 
   return false;
 }
 
+bool PassBuilder::parseAAPassName(AAManager &AA, StringRef Name) {
+#define FUNCTION_ALIAS_ANALYSIS(NAME, CREATE_PASS)                             \
+  if (Name == NAME) {                                                          \
+    AA.registerFunctionAnalysis<decltype(CREATE_PASS)>();                      \
+    return true;                                                               \
+  }
+#include "PassRegistry.def"
+
+  return false;
+}
+
 bool PassBuilder::parseFunctionPassPipeline(FunctionPassManager &FPM,
                                             StringRef &PipelineText,
                                             bool VerifyEachPass,
                                             bool DebugLogging) {
   for (;;) {
     // Parse nested pass managers by recursing.
     if (PipelineText.startswith("function(")) {
       FunctionPassManager NestedFPM(DebugLogging);
if (!parseFunctionPassPipeline(FPM, PipelineText, VerifyEachPass,
         !PipelineText.empty())
       return false;
     MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
     return true;
   }
 
   return false;
 }
+
+bool PassBuilder::parseAAPipeline(AAManager &AA, StringRef PipelineText) {
+  while (!PipelineText.empty()) {
+    StringRef Name;
+    std::tie(Name, PipelineText) = PipelineText.split(',');
+    if (!parseAAPassName(AA, Name))
+      return false;
+  }
+
+  return true;
+}

lib/Passes/PassRegistry.def

 CGSCC_PASS("no-op-cgscc", NoOpCGSCCPass())
 #undef CGSCC_PASS
 
 #ifndef FUNCTION_ANALYSIS
 #define FUNCTION_ANALYSIS(NAME, CREATE_PASS)
 #endif
 FUNCTION_ANALYSIS("aa", AAManager())
 FUNCTION_ANALYSIS("assumptions", AssumptionAnalysis())
-FUNCTION_ANALYSIS("basic-aa", BasicAA())
 FUNCTION_ANALYSIS("domtree", DominatorTreeAnalysis())
 FUNCTION_ANALYSIS("loops", LoopAnalysis())
 FUNCTION_ANALYSIS("no-op-function", NoOpFunctionAnalysis())
 FUNCTION_ANALYSIS("scalar-evolution", ScalarEvolutionAnalysis())
 FUNCTION_ANALYSIS("targetlibinfo", TargetLibraryAnalysis())
 FUNCTION_ANALYSIS("targetir",
                   TM ? TM->getTargetIRAnalysis() : TargetIRAnalysis())
+
+#ifndef FUNCTION_ALIAS_ANALYSIS
+#define FUNCTION_ALIAS_ANALYSIS(NAME, CREATE_PASS)                             \
+  FUNCTION_ANALYSIS(NAME, CREATE_PASS)
+#endif
+FUNCTION_ALIAS_ANALYSIS("basic-aa", BasicAA())
+#undef FUNCTION_ALIAS_ANALYSIS
 #undef FUNCTION_ANALYSIS
 
 #ifndef FUNCTION_PASS
 #define FUNCTION_PASS(NAME, CREATE_PASS)
 #endif
 FUNCTION_PASS("adce", ADCEPass())
 FUNCTION_PASS("early-cse", EarlyCSEPass())
 FUNCTION_PASS("instcombine", InstCombinePass())

test/Other/new-pass-manager.ll

 ; RUN:     -passes='require<domtree>' \
 ; RUN:     | FileCheck %s --check-prefix=CHECK-DT
 ; CHECK-DT: Starting pass manager
 ; CHECK-DT: Running pass: RequireAnalysisPass
 ; CHECK-DT: Running analysis: DominatorTreeAnalysis
 ; CHECK-DT: Finished pass manager
 
 ; RUN: opt -disable-output -disable-verify -debug-pass-manager %s 2>&1 \
-; RUN:     -passes='require<aa>' \
-; RUN:     | FileCheck %s --check-prefix=CHECK-AA
-; CHECK-AA: Starting pass manager
-; CHECK-AA: Running pass: RequireAnalysisPass
-; CHECK-AA: Running analysis: AAManager
-; CHECK-AA: Finished pass manager
-
-; RUN: opt -disable-output -disable-verify -debug-pass-manager %s 2>&1 \
 ; RUN:     -passes='require<basic-aa>' \
 ; RUN:     | FileCheck %s --check-prefix=CHECK-BASIC-AA
 ; CHECK-BASIC-AA: Starting pass manager
 ; CHECK-BASIC-AA: Running pass: RequireAnalysisPass
 ; CHECK-BASIC-AA: Running analysis: BasicAA
 ; CHECK-BASIC-AA: Finished pass manager
 
+; RUN: opt -disable-output -disable-verify -debug-pass-manager %s 2>&1 \
+; RUN:     -passes='require<aa>' -aa-pipeline='basic-aa' \
+; RUN:     | FileCheck %s --check-prefix=CHECK-AA
+; CHECK-AA: Starting pass manager
+; CHECK-AA: Running pass: RequireAnalysisPass
+; CHECK-AA: Running analysis: AAManager
+; CHECK-AA: Running analysis: BasicAA
+; CHECK-AA: Finished pass manager
+
 define void @foo() {
   ret void
 }
 
 declare void @bar()

tools/opt/NewPMDriver.cpp

 ///
 /// This file is just a split of the code that logically belongs in opt.cpp but
 /// that includes the new pass manager headers.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "NewPMDriver.h"
 #include "llvm/ADT/StringRef.h"
+#include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/CGSCCPassManager.h"
 #include "llvm/Bitcode/BitcodeWriterPass.h"
 #include "llvm/IR/Dominators.h"
 #include "llvm/IR/IRPrintingPasses.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/Passes/PassBuilder.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Target/TargetMachine.h"
 
 using namespace llvm;
 using namespace opt_tool;
 
 static cl::opt<bool>
     DebugPM("debug-pass-manager", cl::Hidden,
             cl::desc("Print pass management debugging information"));
 
+// This flag specifies a textual description of the alias analysis pipeline to
+// use when querying for aliasing information. It only works in concert with
+// the "passes" flag above.
+static cl::opt<std::string>
+    AAPipeline("aa-pipeline",
+               cl::desc("A textual description of the alias analysis "
+                        "pipeline for handling managed aliasing queries"),
+               cl::Hidden);
+
 bool llvm::runPassPipeline(StringRef Arg0, LLVMContext &Context, Module &M,
                            TargetMachine *TM, tool_output_file *Out,
                            StringRef PassPipeline, OutputKind OK,
                            VerifierKind VK,
                            bool ShouldPreserveAssemblyUseListOrder,
                            bool ShouldPreserveBitcodeUseListOrder) {
   PassBuilder PB(TM);
 
+  // Specially handle the alias analysis manager so that we can register
+  // a custom pipeline of AA passes with it.
+  AAManager AA;
+  if (!PB.parseAAPipeline(AA, AAPipeline)) {
+    errs() << Arg0 << ": unable to parse AA pipeline description.\n";
+    return false;
+  }
+
   FunctionAnalysisManager FAM(DebugPM);
   CGSCCAnalysisManager CGAM(DebugPM);
   ModuleAnalysisManager MAM(DebugPM);
 
+  // Register the AA manager first so that our version is the one used.
+  FAM.registerPass([&] { return std::move(AA); });
+
   // Register all the basic analyses with the managers.
   PB.registerModuleAnalyses(MAM);
   PB.registerCGSCCAnalyses(CGAM);
   PB.registerFunctionAnalyses(FAM);
 
   // Cross register the analysis managers through their proxies.
-  MAM.registerPass(FunctionAnalysisManagerModuleProxy(FAM));
-  MAM.registerPass(CGSCCAnalysisManagerModuleProxy(CGAM));
-  CGAM.registerPass(FunctionAnalysisManagerCGSCCProxy(FAM));
-  CGAM.registerPass(ModuleAnalysisManagerCGSCCProxy(MAM));
-  FAM.registerPass(CGSCCAnalysisManagerFunctionProxy(CGAM));
-  FAM.registerPass(ModuleAnalysisManagerFunctionProxy(MAM));
+  MAM.registerPass([&] { return FunctionAnalysisManagerModuleProxy(FAM); });
+  MAM.registerPass([&] { return CGSCCAnalysisManagerModuleProxy(CGAM); });
+  CGAM.registerPass([&] { return FunctionAnalysisManagerCGSCCProxy(FAM); });
+  CGAM.registerPass([&] { return ModuleAnalysisManagerCGSCCProxy(MAM); });
+  FAM.registerPass([&] { return CGSCCAnalysisManagerFunctionProxy(CGAM); });
+  FAM.registerPass([&] { return ModuleAnalysisManagerFunctionProxy(MAM); });
 
   ModulePassManager MPM(DebugPM);
   if (VK > VK_NoVerifier)
     MPM.addPass(VerifierPass());
 
   if (!PB.parsePassPipeline(MPM, PassPipeline, VK == VK_VerifyEachPass,
                             DebugPM)) {
     errs() << Arg0 << ": unable to parse pass pipeline description.\n";

unittests/IR/PassManagerTest.cpp

TEST_F(PassManagerTest, BasicPreservedAnalyses) {
   PA1.intersect(PA4);
   EXPECT_TRUE(PA1.preserved<TestFunctionAnalysis>());
   EXPECT_FALSE(PA1.preserved<TestModuleAnalysis>());
 }
 
 TEST_F(PassManagerTest, Basic) {
   FunctionAnalysisManager FAM;
   int FunctionAnalysisRuns = 0;
-  FAM.registerPass(TestFunctionAnalysis(FunctionAnalysisRuns));
+  FAM.registerPass([&] { return TestFunctionAnalysis(FunctionAnalysisRuns); });
 
   ModuleAnalysisManager MAM;
   int ModuleAnalysisRuns = 0;
-  MAM.registerPass(TestModuleAnalysis(ModuleAnalysisRuns));
-  MAM.registerPass(FunctionAnalysisManagerModuleProxy(FAM));
-  FAM.registerPass(ModuleAnalysisManagerFunctionProxy(MAM));
+  MAM.registerPass([&] { return TestModuleAnalysis(ModuleAnalysisRuns); });
+  MAM.registerPass([&] { return FunctionAnalysisManagerModuleProxy(FAM); });
+  FAM.registerPass([&] { return ModuleAnalysisManagerFunctionProxy(MAM); });
 
   ModulePassManager MPM;
 
   // Count the runs over a Function.
   int FunctionPassRunCount1 = 0;
   int AnalyzedInstrCount1 = 0;
   int AnalyzedFunctionCount1 = 0;
   {