[PM] Significantly refactor the pass pipeline parsing to be easier to reason about and less error prone.

[chandlerc]

Summary

The core idea is to fully parse the text without trying to identify
passes or structure. This is done with a single state machine. There
were various bugs in the logic around this previously that were repeated
and scattered across the code. Having a single routine makes it much
easier to fix and get correct. For example, this routine doesn't suffer
from PR28577.

Then the actual pass construction is handled using *much* easier to read
code and simple loops, with particular pass manager construction sunk to
live with other pass construction. This is especially nice as the pass
managers *are* in fact passes.

Finally, the "implicit" pass manager synthesis is done much more simply
by forming "pre-parsed" structures rather than having to duplicate tons
of logic.

One of the bugs fixed by this was evident in the tests where we accepted
a pipeline that wasn't really well formed. Another bug is PR28577 for
which I have added a test case.

The code is less efficient than the previous code but I'm really hoping
that's not a priority. ;]

Depends on D22723

[silvas]

lib/Passes/PassBuilder.cpp

561 ↗

(On Diff #65219)

Do you need error-checking here to verify that InnerPipeline is empty? Just from staring at the code it seems like gvn(foo,bar) will just be silently accepted with the (foo,bar) ignored.

I think down the road it actually would be useful to allow passes to take "arguments". E.g. inline(150) or instcombine(expensivecombines=true). That way we can run two inliners in the same pipeline but with different thresholds.
The instcombine case is actually interesting because currently there is no way to specify an instcombine pass that does not do the "expensive" combines. This means that this textual pipeline description cannot actually describe the O2 pipeline correctly (this is an issue with the old PM too, of course, so it isn't a high priority to fix).

[chandlerc]

lib/Passes/PassBuilder.cpp

561 ↗

(On Diff #65219)

I think we should check that the inner pipeline is empty as you suggest.

I agree that we'll want passes to take arguments. We actually already have that with requires and such.

The design I've been going with is that ()s denote *pipelines*, and <>s denote *arguments*. So, imagining a pass that repeats a pipeline a certain number of times (which I have a review out for but will update based on the new parsing momentarily) we will have "repeat<N>(gvn,whatever)". Does that make sense?

[chandlerc]

Add error checking for providing an inner pipeline to a normal pass by handling
the inner pipeline parsing separately. Seems less brittle going forward too.

[silvas]

LGTM.

lib/Passes/PassBuilder.cpp

574 ↗

(On Diff #66603)

Thanks for the clarification. That makes sense to me. Can you make sure that ends up in a comment somewhere (if it isn't already)?

602 ↗

(On Diff #66603)

LoopPassManager &FPM looks a bit funky. Might as well make it LPM if changing the line.

[chandlerc]

Thanks for reviewing! Will land shortly.

lib/Passes/PassBuilder.cpp

574 ↗

(On Diff #66603)

Yea, I'll land a follow-up commit that documents some of this design stuff. Let me know if it doesn't go far enough.

602 ↗

(On Diff #66603)

This is precisely one of the things I want to fix and mentioned in IRC. Changing it in this patch adds a bunch of other lines of diff sadly. I'll do it in a follow-up.

llvm/trunk/include/llvm/Passes/PassBuilder.h

 /// Interfaces for registering analysis passes, producing common pass manager
 /// configurations, and parsing of pass pipelines.
 ///
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_PASSES_PASSBUILDER_H
 #define LLVM_PASSES_PASSBUILDER_H
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/Analysis/CGSCCPassManager.h"
 #include "llvm/Analysis/LoopPassManager.h"
 #include "llvm/IR/PassManager.h"
+#include <vector>
 
 namespace llvm {
 class StringRef;
 class AAManager;
 class TargetMachine;
 
 /// \brief This class provides access to building LLVM's passes.
 ///
public:
   /// 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 DebugLogging);
-  bool parseCGSCCPassName(CGSCCPassManager &CGPM, StringRef Name);
-  bool parseFunctionPassName(FunctionPassManager &FPM, StringRef Name);
-  bool parseLoopPassName(LoopPassManager &LPM, StringRef Name);
+  /// A struct to capture parsed pass pipeline names.
+  struct PipelineElement {
+    StringRef Name;
+    std::vector<PipelineElement> InnerPipeline;
+  };
+
+  static Optional<std::vector<PipelineElement>>
+  parsePipelineText(StringRef Text);
+
+  bool parseModulePass(ModulePassManager &MPM, const PipelineElement &E,
+                       bool VerifyEachPass, bool DebugLogging);
+  bool parseCGSCCPass(CGSCCPassManager &CGPM, const PipelineElement &E,
+                      bool VerifyEachPass, bool DebugLogging);
+  bool parseFunctionPass(FunctionPassManager &FPM, const PipelineElement &E,
+                     bool VerifyEachPass, bool DebugLogging);
+  bool parseLoopPass(LoopPassManager &LPM, const PipelineElement &E,
+                     bool VerifyEachPass, bool DebugLogging);
   bool parseAAPassName(AAManager &AA, StringRef Name);
-  bool parseLoopPassPipeline(LoopPassManager &LPM, StringRef &PipelineText,
+
+  bool parseLoopPassPipeline(LoopPassManager &LPM,
+                             ArrayRef<PipelineElement> Pipeline,
                              bool VerifyEachPass, bool DebugLogging);
   bool parseFunctionPassPipeline(FunctionPassManager &FPM,
-                                 StringRef &PipelineText, bool VerifyEachPass,
-                                 bool DebugLogging);
-  bool parseCGSCCPassPipeline(CGSCCPassManager &CGPM, StringRef &PipelineText,
+                                 ArrayRef<PipelineElement> Pipeline,
+                                 bool VerifyEachPass, bool DebugLogging);
+  bool parseCGSCCPassPipeline(CGSCCPassManager &CGPM,
+                              ArrayRef<PipelineElement> Pipeline,
                               bool VerifyEachPass, bool DebugLogging);
-  bool parseModulePassPipeline(ModulePassManager &MPM, StringRef &PipelineText,
+  bool parseModulePassPipeline(ModulePassManager &MPM,
+                               ArrayRef<PipelineElement> Pipeline,
                                bool VerifyEachPass, bool DebugLogging);
 };
 }
 
 #endif

llvm/trunk/lib/Passes/PassBuilder.cpp

 }
 
 #ifndef NDEBUG
 static bool isModulePassName(StringRef Name) {
   // Manually handle aliases for pre-configured pipeline fragments.
   if (Name.startswith("default") || Name.startswith("lto"))
     return DefaultAliasRegex.match(Name);
 
+  // Explicitly handle pass manager names.
+  if (Name == "module")
+    return true;
+  if (Name == "cgscc")
+    return true;
+  if (Name == "function")
+    return true;
+
 #define MODULE_PASS(NAME, CREATE_PASS)                                         \
   if (Name == NAME)                                                            \
     return true;
 #define MODULE_ANALYSIS(NAME, CREATE_PASS)                                     \
   if (Name == "require<" NAME ">" || Name == "invalidate<" NAME ">")           \
     return true;
 #include "PassRegistry.def"
 
   return false;
 }
 #endif
 
 static bool isCGSCCPassName(StringRef Name) {
+  // Explicitly handle pass manager names.
+  if (Name == "cgscc")
+    return true;
+  if (Name == "function")
+    return true;
+
 #define CGSCC_PASS(NAME, CREATE_PASS)                                          \
   if (Name == NAME)                                                            \
     return true;
 #define CGSCC_ANALYSIS(NAME, CREATE_PASS)                                      \
   if (Name == "require<" NAME ">" || Name == "invalidate<" NAME ">")           \
     return true;
 #include "PassRegistry.def"
 
   return false;
 }
 
 static bool isFunctionPassName(StringRef Name) {
+  // Explicitly handle pass manager names.
+  if (Name == "function")
+    return true;
+  if (Name == "loop")
+    return true;
+
 #define FUNCTION_PASS(NAME, CREATE_PASS)                                       \
   if (Name == NAME)                                                            \
     return true;
 #define FUNCTION_ANALYSIS(NAME, CREATE_PASS)                                   \
   if (Name == "require<" NAME ">" || Name == "invalidate<" NAME ">")           \
     return true;
 #include "PassRegistry.def"
 
   return false;
 }
 
 static bool isLoopPassName(StringRef Name) {
+  // Explicitly handle pass manager names.
+  if (Name == "loop")
+    return true;
+
 #define LOOP_PASS(NAME, CREATE_PASS)                                           \
   if (Name == NAME)                                                            \
     return true;
 #define LOOP_ANALYSIS(NAME, CREATE_PASS)                                       \
   if (Name == "require<" NAME ">" || Name == "invalidate<" NAME ">")           \
     return true;
 #include "PassRegistry.def"
 
   return false;
 }
 
-bool PassBuilder::parseModulePassName(ModulePassManager &MPM, StringRef Name,
-                                      bool DebugLogging) {
+Optional<std::vector<PassBuilder::PipelineElement>>
+PassBuilder::parsePipelineText(StringRef Text) {
+  std::vector<PipelineElement> ResultPipeline;
+
+  SmallVector<std::vector<PipelineElement> *, 4> PipelineStack = {
+      &ResultPipeline};
+  for (;;) {
+    std::vector<PipelineElement> &Pipeline = *PipelineStack.back();
+    size_t Pos = Text.find_first_of(",()");
+    Pipeline.push_back({Text.substr(0, Pos), {}});
+
+    // If we have a single terminating name, we're done.
+    if (Pos == Text.npos)
+      break;
+
+    char Sep = Text[Pos];
+    Text = Text.substr(Pos + 1);
+    if (Sep == ',')
+      // Just a name ending in a comma, continue.
+      continue;
+
+    if (Sep == '(') {
+      // Push the inner pipeline onto the stack to continue processing.
+      PipelineStack.push_back(&Pipeline.back().InnerPipeline);
+      continue;
+    }
+
+    assert(Sep == ')' && "Bogus separator!");
+    // When handling the close parenthesis, we greedily consume them to avoid
+    // empty strings in the pipeline.
+    do {
+      // If we try to pop the outer pipeline we have unbalanced parentheses.
+      if (PipelineStack.size() == 1)
+        return None;
+
+      PipelineStack.pop_back();
+    } while (Text.consume_front(")"));
+
+    // Check if we've finished parsing.
+    if (Text.empty())
+      break;
+
+    // Otherwise, the end of an inner pipeline always has to be followed by
+    // a comma, and then we can continue.
+    if (!Text.consume_front(","))
+      return None;
+  }
+
+  if (PipelineStack.size() > 1)
+    // Unbalanced paretheses.
+    return None;
+
+  assert(PipelineStack.back() == &ResultPipeline &&
+         "Wrong pipeline at the bottom of the stack!");
+  return {std::move(ResultPipeline)};
+}
+
+bool PassBuilder::parseModulePass(ModulePassManager &MPM,
+                                  const PipelineElement &E, bool VerifyEachPass,
+  auto &Name = E.Name;
+  auto &InnerPipeline = E.InnerPipeline;
+
+  // First handle complex passes like the pass managers which carry pipelines.
+  if (!InnerPipeline.empty()) {
+    if (Name == "module") {
+      ModulePassManager NestedMPM(DebugLogging);
+      if (!parseModulePassPipeline(NestedMPM, InnerPipeline, VerifyEachPass,
+                                   DebugLogging))
+        return false;
+      MPM.addPass(std::move(NestedMPM));
+      return true;
+    }
+    if (Name == "cgscc") {
+      CGSCCPassManager CGPM(DebugLogging);
+      if (!parseCGSCCPassPipeline(CGPM, InnerPipeline, VerifyEachPass,
+                                  DebugLogging))
+        return false;
+      MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM),
+                                                          DebugLogging));
+      return true;
+    }
+    if (Name == "function") {
+      FunctionPassManager FPM(DebugLogging);
+      if (!parseFunctionPassPipeline(FPM, InnerPipeline, VerifyEachPass,
+                                     DebugLogging))
+        return false;
+      MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
+      return true;
+    }
+    // Normal passes can't have pipelines.
+    return false;
+  }
+
   // Manually handle aliases for pre-configured pipeline fragments.
   if (Name.startswith("default") || Name.startswith("lto")) {
     SmallVector<StringRef, 3> Matches;
     if (!DefaultAliasRegex.match(Name, &Matches))
       return false;
     assert(Matches.size() == 3 && "Must capture two matched strings!");
 
     OptimizationLevel L = StringSwitch<OptimizationLevel>(Matches[2])
if (Matches[1] == "default") {
       addLTOPreLinkDefaultPipeline(MPM, L, DebugLogging);
     } else {
       assert(Matches[1] == "lto" && "Not one of the matched options!");
       addLTODefaultPipeline(MPM, L, DebugLogging);
     }
     return true;
   }
 
+  // Finally expand the basic registered passes from the .inc file.
 #define MODULE_PASS(NAME, CREATE_PASS)                                         \
   if (Name == NAME) {                                                          \
     MPM.addPass(CREATE_PASS);                                                  \
     return true;                                                               \
   }
 #define MODULE_ANALYSIS(NAME, CREATE_PASS)                                     \
   if (Name == "require<" NAME ">") {                                           \
     MPM.addPass(RequireAnalysisPass<                                           \
                 std::remove_reference<decltype(CREATE_PASS)>::type>());        \
     return true;                                                               \
   }                                                                            \
   if (Name == "invalidate<" NAME ">") {                                        \
     MPM.addPass(InvalidateAnalysisPass<                                        \
                 std::remove_reference<decltype(CREATE_PASS)>::type>());        \
     return true;                                                               \
   }
 #include "PassRegistry.def"
 
   return false;
 }
 
-bool PassBuilder::parseCGSCCPassName(CGSCCPassManager &CGPM, StringRef Name) {
+bool PassBuilder::parseCGSCCPass(CGSCCPassManager &CGPM,
+                                 const PipelineElement &E, bool VerifyEachPass,
+                                 bool DebugLogging) {
+  auto &Name = E.Name;
+  auto &InnerPipeline = E.InnerPipeline;
+
+  // First handle complex passes like the pass managers which carry pipelines.
+  if (!InnerPipeline.empty()) {
+    if (Name == "cgscc") {
+      CGSCCPassManager NestedCGPM(DebugLogging);
+      if (!parseCGSCCPassPipeline(NestedCGPM, InnerPipeline, VerifyEachPass,
+                                  DebugLogging))
+        return false;
+      // Add the nested pass manager with the appropriate adaptor.
+      CGPM.addPass(std::move(NestedCGPM));
+      return true;
+    }
+    if (Name == "function") {
+      FunctionPassManager FPM(DebugLogging);
+      if (!parseFunctionPassPipeline(FPM, InnerPipeline, VerifyEachPass,
+                                     DebugLogging))
+        return false;
+      // Add the nested pass manager with the appropriate adaptor.
+      CGPM.addPass(
+          createCGSCCToFunctionPassAdaptor(std::move(FPM), DebugLogging));
+      return true;
+    }
+    // Normal passes can't have pipelines.
+    return false;
+  }
+
+  // Now expand the basic registered passes from the .inc file.
 #define CGSCC_PASS(NAME, CREATE_PASS)                                          \
   if (Name == NAME) {                                                          \
     CGPM.addPass(CREATE_PASS);                                                 \
     return true;                                                               \
   }
 #define CGSCC_ANALYSIS(NAME, CREATE_PASS)                                      \
   if (Name == "require<" NAME ">") {                                           \
     CGPM.addPass(RequireAnalysisPass<                                          \
                  std::remove_reference<decltype(CREATE_PASS)>::type>());       \
     return true;                                                               \
   }                                                                            \
   if (Name == "invalidate<" NAME ">") {                                        \
     CGPM.addPass(InvalidateAnalysisPass<                                       \
                  std::remove_reference<decltype(CREATE_PASS)>::type>());       \
     return true;                                                               \
   }
 #include "PassRegistry.def"
 
   return false;
 }
 
-bool PassBuilder::parseFunctionPassName(FunctionPassManager &FPM,
-                                        StringRef Name) {
+bool PassBuilder::parseFunctionPass(FunctionPassManager &FPM,
+                                    const PipelineElement &E,
+                                    bool VerifyEachPass, bool DebugLogging) {
+  auto &Name = E.Name;
+  auto &InnerPipeline = E.InnerPipeline;
+
+  // First handle complex passes like the pass managers which carry pipelines.
+  if (!InnerPipeline.empty()) {
+    if (Name == "function") {
+      FunctionPassManager NestedFPM(DebugLogging);
+      if (!parseFunctionPassPipeline(NestedFPM, InnerPipeline, VerifyEachPass,
+                                     DebugLogging))
+        return false;
+      // Add the nested pass manager with the appropriate adaptor.
+      FPM.addPass(std::move(NestedFPM));
+      return true;
+    }
+    if (Name == "loop") {
+      LoopPassManager LPM(DebugLogging);
+      if (!parseLoopPassPipeline(LPM, InnerPipeline, VerifyEachPass,
+                                 DebugLogging))
+        return false;
+      // Add the nested pass manager with the appropriate adaptor.
+      FPM.addPass(createFunctionToLoopPassAdaptor(std::move(LPM)));
+      return true;
+    }
+    // Normal passes can't have pipelines.
+    return false;
+  }
+
+  // Now expand the basic registered passes from the .inc file.
 #define FUNCTION_PASS(NAME, CREATE_PASS)                                       \
   if (Name == NAME) {                                                          \
     FPM.addPass(CREATE_PASS);                                                  \
     return true;                                                               \
   }
 #define FUNCTION_ANALYSIS(NAME, CREATE_PASS)                                   \
   if (Name == "require<" NAME ">") {                                           \
     FPM.addPass(RequireAnalysisPass<                                           \
                 std::remove_reference<decltype(CREATE_PASS)>::type>());        \
     return true;                                                               \
   }                                                                            \
   if (Name == "invalidate<" NAME ">") {                                        \
     FPM.addPass(InvalidateAnalysisPass<                                        \
                 std::remove_reference<decltype(CREATE_PASS)>::type>());        \
     return true;                                                               \
   }
 #include "PassRegistry.def"
 
   return false;
 }
 
-bool PassBuilder::parseLoopPassName(LoopPassManager &FPM, StringRef Name) {
+bool PassBuilder::parseLoopPass(LoopPassManager &FPM, const PipelineElement &E,
+                                bool VerifyEachPass, bool DebugLogging) {
+  auto &Name = E.Name;
+  auto &InnerPipeline = E.InnerPipeline;
+
+  // First handle complex passes like the pass managers which carry pipelines.
+  if (!InnerPipeline.empty()) {
+    if (Name == "loop") {
+      LoopPassManager NestedLPM(DebugLogging);
+      if (!parseLoopPassPipeline(NestedLPM, InnerPipeline, VerifyEachPass,
+                                 DebugLogging))
+        return false;
+      // Add the nested pass manager with the appropriate adaptor.
+      FPM.addPass(std::move(NestedLPM));
+      return true;
+    }
+    // Normal passes can't have pipelines.
+    return false;
+  }
+
+  // Now expand the basic registered passes from the .inc file.
 #define LOOP_PASS(NAME, CREATE_PASS)                                           \
   if (Name == NAME) {                                                          \
     FPM.addPass(CREATE_PASS);                                                  \
     return true;                                                               \
   }
 #define LOOP_ANALYSIS(NAME, CREATE_PASS)                                       \
   if (Name == "require<" NAME ">") {                                           \
     FPM.addPass(RequireAnalysisPass<                                           \
if (Name == NAME) {                                                          \
     return true;                                                               \
   }
 #include "PassRegistry.def"
 
   return false;
 }
 
 bool PassBuilder::parseLoopPassPipeline(LoopPassManager &LPM,
-                                        StringRef &PipelineText,
+                                        ArrayRef<PipelineElement> Pipeline,
                                         bool VerifyEachPass,
                                         bool DebugLogging) {
-  for (;;) {
-    // Parse nested pass managers by recursing.
-    if (PipelineText.startswith("loop(")) {
-      LoopPassManager NestedLPM(DebugLogging);
-
-      // Parse the inner pipeline inte the nested manager.
-      PipelineText = PipelineText.substr(strlen("loop("));
-      if (!parseLoopPassPipeline(NestedLPM, PipelineText, VerifyEachPass,
-                                 DebugLogging) ||
-          PipelineText.empty())
-        return false;
-      assert(PipelineText[0] == ')');
-      PipelineText = PipelineText.substr(1);
-
-      // Add the nested pass manager with the appropriate adaptor.
-      LPM.addPass(std::move(NestedLPM));
-    } else {
-      // Otherwise try to parse a pass name.
-      size_t End = PipelineText.find_first_of(",)");
-      if (!parseLoopPassName(LPM, PipelineText.substr(0, End)))
-        return false;
-      // TODO: Ideally, we would run a LoopVerifierPass() here in the
-      // VerifyEachPass case, but we don't have such a verifier yet.
-
-      PipelineText = PipelineText.substr(End);
-    }
-
-    if (PipelineText.empty() || PipelineText[0] == ')')
-      return true;
-
-    assert(PipelineText[0] == ',');
-    PipelineText = PipelineText.substr(1);
-  }
+  for (const auto &Element : Pipeline) {
+    if (!parseLoopPass(LPM, Element, VerifyEachPass, DebugLogging))
+    // FIXME: No verifier support for Loop passes!
 }
 
 bool PassBuilder::parseFunctionPassPipeline(FunctionPassManager &FPM,
-                                            StringRef &PipelineText,
+                                            ArrayRef<PipelineElement> Pipeline,
                                             bool VerifyEachPass,
                                             bool DebugLogging) {
-  for (;;) {
-    // Parse nested pass managers by recursing.
-    if (PipelineText.startswith("function(")) {
-      FunctionPassManager NestedFPM(DebugLogging);
-
-      // Parse the inner pipeline inte the nested manager.
-      PipelineText = PipelineText.substr(strlen("function("));
-      if (!parseFunctionPassPipeline(NestedFPM, PipelineText, VerifyEachPass,
-                                     DebugLogging) ||
-          PipelineText.empty())
-        return false;
-      assert(PipelineText[0] == ')');
-      PipelineText = PipelineText.substr(1);
-
-      // Add the nested pass manager with the appropriate adaptor.
-      FPM.addPass(std::move(NestedFPM));
-    } else if (PipelineText.startswith("loop(")) {
-      LoopPassManager NestedLPM(DebugLogging);
-
-      // Parse the inner pipeline inte the nested manager.
-      PipelineText = PipelineText.substr(strlen("loop("));
-      if (!parseLoopPassPipeline(NestedLPM, PipelineText, VerifyEachPass,
-                                 DebugLogging) ||
-          PipelineText.empty())
-        return false;
-      assert(PipelineText[0] == ')');
-      PipelineText = PipelineText.substr(1);
-
-      // Add the nested pass manager with the appropriate adaptor.
-      FPM.addPass(createFunctionToLoopPassAdaptor(std::move(NestedLPM)));
-    } else {
-      // Otherwise try to parse a pass name.
-      size_t End = PipelineText.find_first_of(",)");
-      if (!parseFunctionPassName(FPM, PipelineText.substr(0, End)))
-        return false;
-      if (VerifyEachPass)
-        FPM.addPass(VerifierPass());
-
-      PipelineText = PipelineText.substr(End);
-    }
-
-    if (PipelineText.empty() || PipelineText[0] == ')')
-      return true;
-
-    assert(PipelineText[0] == ',');
-    PipelineText = PipelineText.substr(1);
-  }
+  for (const auto &Element : Pipeline) {
+    if (!parseFunctionPass(FPM, Element, VerifyEachPass, DebugLogging))
 }
 
 bool PassBuilder::parseCGSCCPassPipeline(CGSCCPassManager &CGPM,
-                                         StringRef &PipelineText,
+                                         ArrayRef<PipelineElement> Pipeline,
                                          bool VerifyEachPass,
                                          bool DebugLogging) {
-  for (;;) {
-    // Parse nested pass managers by recursing.
-    if (PipelineText.startswith("cgscc(")) {
-      CGSCCPassManager NestedCGPM(DebugLogging);
-
-      // Parse the inner pipeline into the nested manager.
-      PipelineText = PipelineText.substr(strlen("cgscc("));
-      if (!parseCGSCCPassPipeline(NestedCGPM, PipelineText, VerifyEachPass,
-                                  DebugLogging) ||
-          PipelineText.empty())
-        return false;
-      assert(PipelineText[0] == ')');
-      PipelineText = PipelineText.substr(1);
-
-      // Add the nested pass manager with the appropriate adaptor.
-      CGPM.addPass(std::move(NestedCGPM));
-    } else if (PipelineText.startswith("function(")) {
-      FunctionPassManager NestedFPM(DebugLogging);
-
-      // Parse the inner pipeline inte the nested manager.
-      PipelineText = PipelineText.substr(strlen("function("));
-      if (!parseFunctionPassPipeline(NestedFPM, PipelineText, VerifyEachPass,
-                                     DebugLogging) ||
-          PipelineText.empty())
-        return false;
-      assert(PipelineText[0] == ')');
-      PipelineText = PipelineText.substr(1);
-
-      // Add the nested pass manager with the appropriate adaptor.
-      CGPM.addPass(
-          createCGSCCToFunctionPassAdaptor(std::move(NestedFPM), DebugLogging));
-    } else {
-      // Otherwise try to parse a pass name.
-      size_t End = PipelineText.find_first_of(",)");
-      if (!parseCGSCCPassName(CGPM, PipelineText.substr(0, End)))
-        return false;
-      // FIXME: No verifier support for CGSCC passes!
-
-      PipelineText = PipelineText.substr(End);
-    }
-
-    if (PipelineText.empty() || PipelineText[0] == ')')
-      return true;
-
-    assert(PipelineText[0] == ',');
-    PipelineText = PipelineText.substr(1);
-  }
+  for (const auto &Element : Pipeline) {
+    if (!parseCGSCCPass(CGPM, Element, VerifyEachPass, DebugLogging))
 }
 
 void PassBuilder::crossRegisterProxies(LoopAnalysisManager &LAM,
                                        FunctionAnalysisManager &FAM,
                                        CGSCCAnalysisManager &CGAM,
                                        ModuleAnalysisManager &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); });
   FAM.registerPass([&] { return LoopAnalysisManagerFunctionProxy(LAM); });
   LAM.registerPass([&] { return FunctionAnalysisManagerLoopProxy(FAM); });
 }
 
 bool PassBuilder::parseModulePassPipeline(ModulePassManager &MPM,
-                                          StringRef &PipelineText,
+                                          ArrayRef<PipelineElement> Pipeline,
                                           bool VerifyEachPass,
                                           bool DebugLogging) {
-  for (;;) {
-    // Parse nested pass managers by recursing.
-    if (PipelineText.startswith("module(")) {
-      ModulePassManager NestedMPM(DebugLogging);
-
-      // Parse the inner pipeline into the nested manager.
-      PipelineText = PipelineText.substr(strlen("module("));
-      if (!parseModulePassPipeline(NestedMPM, PipelineText, VerifyEachPass,
-                                   DebugLogging) ||
-          PipelineText.empty())
-        return false;
-      assert(PipelineText[0] == ')');
-      PipelineText = PipelineText.substr(1);
-
-      // Now add the nested manager as a module pass.
-      MPM.addPass(std::move(NestedMPM));
-    } else if (PipelineText.startswith("cgscc(")) {
-      CGSCCPassManager NestedCGPM(DebugLogging);
-
-      // Parse the inner pipeline inte the nested manager.
-      PipelineText = PipelineText.substr(strlen("cgscc("));
-      if (!parseCGSCCPassPipeline(NestedCGPM, PipelineText, VerifyEachPass,
-                                  DebugLogging) ||
-          PipelineText.empty())
-        return false;
-      assert(PipelineText[0] == ')');
-      PipelineText = PipelineText.substr(1);
-
-      // Add the nested pass manager with the appropriate adaptor.
-      MPM.addPass(createModuleToPostOrderCGSCCPassAdaptor(std::move(NestedCGPM),
-                                                          DebugLogging));
-    } else if (PipelineText.startswith("function(")) {
-      FunctionPassManager NestedFPM(DebugLogging);
-
-      // Parse the inner pipeline inte the nested manager.
-      PipelineText = PipelineText.substr(strlen("function("));
-      if (!parseFunctionPassPipeline(NestedFPM, PipelineText, VerifyEachPass,
-                                     DebugLogging) ||
-          PipelineText.empty())
-        return false;
-      assert(PipelineText[0] == ')');
-      PipelineText = PipelineText.substr(1);
-
-      // Add the nested pass manager with the appropriate adaptor.
-      MPM.addPass(createModuleToFunctionPassAdaptor(std::move(NestedFPM)));
-    } else {
-      // Otherwise try to parse a pass name.
-      size_t End = PipelineText.find_first_of(",)");
-      if (!parseModulePassName(MPM, PipelineText.substr(0, End), DebugLogging))
-        return false;
-      if (VerifyEachPass)
-        MPM.addPass(VerifierPass());
-
-      PipelineText = PipelineText.substr(End);
-    }
-
-    if (PipelineText.empty() || PipelineText[0] == ')')
-      return true;
-
-    assert(PipelineText[0] == ',');
-    PipelineText = PipelineText.substr(1);
-  }
+  for (const auto &Element : Pipeline) {
+    if (!parseModulePass(MPM, Element, VerifyEachPass, DebugLogging))
 }
 
 // Primary pass pipeline description parsing routine.
 // FIXME: Should this routine accept a TargetMachine or require the caller to
 // pre-populate the analysis managers with target-specific stuff?
 bool PassBuilder::parsePassPipeline(ModulePassManager &MPM,
                                     StringRef PipelineText, bool VerifyEachPass,
                                     bool DebugLogging) {
-  // By default, try to parse the pipeline as-if it were within an implicit
-  // 'module(...)' pass pipeline. If this will parse at all, it needs to
-  // consume the entire string.
-  if (parseModulePassPipeline(MPM, PipelineText, VerifyEachPass, DebugLogging))
-    return PipelineText.empty();
-
-  // This isn't parsable as a module pipeline, look for the end of a pass name
-  // and directly drop down to that layer.
-  StringRef FirstName =
-      PipelineText.substr(0, PipelineText.find_first_of(",)"));
-  assert(!isModulePassName(FirstName) &&
-         "Already handled all module pipeline options.");
-
-  // If this looks like a CGSCC pass, parse the whole thing as a CGSCC
-  // pipeline.
-  if (PipelineText.startswith("cgscc(") || isCGSCCPassName(FirstName)) {
-    CGSCCPassManager CGPM(DebugLogging);
-    if (!parseCGSCCPassPipeline(CGPM, PipelineText, VerifyEachPass,
-                                DebugLogging) ||
-        !PipelineText.empty())
-      return false;
-    MPM.addPass(
-        createModuleToPostOrderCGSCCPassAdaptor(std::move(CGPM), DebugLogging));
-    return true;
-  }
+  auto Pipeline = parsePipelineText(PipelineText);
+  if (!Pipeline || Pipeline->empty())
 
-  // Similarly, if this looks like a Function pass, parse the whole thing as
-  // a Function pipelien.
-  if (PipelineText.startswith("function(") || isFunctionPassName(FirstName)) {
-    FunctionPassManager FPM(DebugLogging);
-    if (!parseFunctionPassPipeline(FPM, PipelineText, VerifyEachPass,
-                                   DebugLogging) ||
-        !PipelineText.empty())
-      return false;
-    MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
-    return true;
-  }
+  // If the first name isn't at the module layer, wrap the pipeline up
+  // automatically.
+  StringRef FirstName = Pipeline->front().Name;
 
-  // If this looks like a Loop pass, parse the whole thing as a Loop pipeline.
-  if (PipelineText.startswith("loop(") || isLoopPassName(FirstName)) {
-    LoopPassManager LPM(DebugLogging);
-    if (!parseLoopPassPipeline(LPM, PipelineText, VerifyEachPass,
-                               DebugLogging) ||
-        !PipelineText.empty())
+  if (!isModulePassName(FirstName)) {
+    if (isCGSCCPassName(FirstName))
+      Pipeline = {{"cgscc", std::move(*Pipeline)}};
+    else if (isFunctionPassName(FirstName))
+      Pipeline = {{"function", std::move(*Pipeline)}};
+    else if (isLoopPassName(FirstName))
+      Pipeline = {{"function", {{"loop", std::move(*Pipeline)}}}};
+    else
+      // Unknown pass name!
       return false;
-    FunctionPassManager FPM(DebugLogging);
-    FPM.addPass(createFunctionToLoopPassAdaptor(std::move(LPM)));
-    MPM.addPass(createModuleToFunctionPassAdaptor(std::move(FPM)));
-    return true;
   }
 
-  return false;
+  return parseModulePassPipeline(MPM, *Pipeline, VerifyEachPass, DebugLogging);
 }
 
 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;
 }

llvm/trunk/test/Other/pass-pipeline-parsing.ll

 ; CHECK-NESTED-FP-LP: Starting llvm::Function pass manager run
 ; CHECK-NESTED-FP-LP: Running pass: FunctionToLoopPassAdaptor
 ; CHECK-NESTED-FP-LP: Starting llvm::Loop pass manager run
 ; CHECK-NESTED-FP-LP: Running pass: NoOpLoopPass
 ; CHECK-NESTED-FP-LP: Finished llvm::Loop pass manager run
 ; CHECK-NESTED-FP-LP: Finished llvm::Function pass manager run
 ; CHECK-NESTED-FP-LP: Finished llvm::Module pass manager run
 
+; RUN: not opt -disable-output -debug-pass-manager \
+; RUN:     -passes='function(no-op-function)function(no-op-function)' %s 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=CHECK-MISSING-COMMA1
+; CHECK-MISSING-COMMA1: unable to parse pass pipeline description
+
+; RUN: not opt -disable-output -debug-pass-manager \
+; RUN:     -passes='function()' %s 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=CHECK-EMPTY-INNER-PIPELINE
+; CHECK-EMPTY-INNER-PIPELINE: unable to parse pass pipeline description
+
+; RUN: not opt -disable-output -debug-pass-manager \
+; RUN:     -passes='no-op-module(no-op-module,whatever)' %s 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=CHECK-PIPELINE-ON-MODULE-PASS
+; CHECK-PIPELINE-ON-MODULE-PASS: unable to parse pass pipeline description
+
+; RUN: not opt -disable-output -debug-pass-manager \
+; RUN:     -passes='no-op-cgscc(no-op-cgscc,whatever)' %s 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=CHECK-PIPELINE-ON-CGSCC-PASS
+; CHECK-PIPELINE-ON-CGSCC-PASS: unable to parse pass pipeline description
+
+; RUN: not opt -disable-output -debug-pass-manager \
+; RUN:     -passes='no-op-function(no-op-function,whatever)' %s 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=CHECK-PIPELINE-ON-FUNCTION-PASS
+; CHECK-PIPELINE-ON-FUNCTION-PASS: unable to parse pass pipeline description
+
+; RUN: not opt -disable-output -debug-pass-manager \
+; RUN:     -passes='no-op-loop(no-op-loop,whatever)' %s 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=CHECK-PIPELINE-ON-LOOP-PASS
+; CHECK-PIPELINE-ON-LOOP-PASS: unable to parse pass pipeline description
+
+; RUN: not opt -disable-output -debug-pass-manager \
+; RUN:     -passes='no-op-function()' %s 2>&1 \
+; RUN:     | FileCheck %s --check-prefix=CHECK-EMPTY-PIPELINE-ON-PASS
+; CHECK-EMPTY-PIPELINE-ON-PASS: unable to parse pass pipeline description
 
 define void @f() {
 entry:
  br label %loop
 loop:
  br label %loop
 }

llvm/trunk/test/Transforms/LoopStrengthReduce/quadradic-exit-value.ll

 ; RUN: opt < %s -analyze -iv-users | FileCheck %s
-; RUN: opt -passes='function(require<scalar-evolution>),print<ivusers>' -S < %s 2>&1| FileCheck %s
+; RUN: opt -passes='function(require<scalar-evolution>,loop(print<ivusers>))' -S < %s 2>&1| FileCheck %s
 
 ; Provide legal integer types.
 target datalayout = "n8:16:32:64"
 
 ; The value of %r is dependent on a polynomial iteration expression.
 ;
 ; CHECK-LABEL: IV Users for loop %foo.loop
 ; CHECK: {1,+,3,+,2}<%foo.loop>