Address review feedback

Author: slackito

llvm/include/llvm/SandboxIR/PassManager.h

@@ -32,11 +32,20 @@ class Value;
 /// Base class.
 template <typename ParentPass, typename ContainedPass>
 class PassManager : public ParentPass {
+public:
+  // CreatePassFunc(StringRef PassName, StringRef PassArgs).
+  using CreatePassFunc =
+      std::function<std::unique_ptr<ContainedPass>(StringRef, StringRef)>;
+
 protected:
   /// The list of passes that this pass manager will run.
   SmallVector<std::unique_ptr<ContainedPass>> Passes;
 
   PassManager(StringRef Name) : ParentPass(Name) {}
+  PassManager(StringRef Name, StringRef Pipeline, CreatePassFunc CreatePass)
+      : ParentPass(Name) {
+    setPassPipeline(Pipeline, CreatePass);
+  }
   PassManager(const PassManager &) = delete;
   PassManager(PassManager &&) = default;
   virtual ~PassManager() = default;
@@ -49,9 +58,6 @@ class PassManager : public ParentPass {
     Passes.push_back(std::move(Pass));
   }
 
-  using CreatePassFunc =
-      std::function<std::unique_ptr<ContainedPass>(StringRef, StringRef)>;
-
   /// Parses \p Pipeline as a comma-separated sequence of pass names and sets
   /// the pass pipeline, using \p CreatePass to instantiate passes by name.
   ///
@@ -86,18 +92,6 @@ class PassManager : public ParentPass {
     std::string PipelineStr = std::string(Pipeline) + EndToken;
     Pipeline = StringRef(PipelineStr);
 
-    enum {
-      ScanName,  // reading a pass name
-      ScanArgs,  // reading a list of args
-      ArgsEnded, // read the last '>' in an args list, must read delimiter next
-    } State;
-    State = ScanName;
-    int PassBeginIdx = 0;
-    int ArgsBeginIdx;
-    StringRef PassName;
-    StringRef PassArgs;
-    int NestedArgs = 0;
-
     auto AddPass = [this, CreatePass](StringRef PassName, StringRef PassArgs) {
       if (PassName.empty()) {
         errs() << "Found empty pass name.\n";
@@ -112,15 +106,25 @@ class PassManager : public ParentPass {
       }
       addPass(std::move(Pass));
     };
+
+    enum class State {
+      ScanName,  // reading a pass name
+      ScanArgs,  // reading a list of args
+      ArgsEnded, // read the last '>' in an args list, must read delimiter next
+    } CurrentState = State::ScanName;
+    int PassBeginIdx = 0;
+    int ArgsBeginIdx;
+    StringRef PassName;
+    int NestedArgs = 0;
     for (auto [Idx, C] : enumerate(Pipeline)) {
-      switch (State) {
-      case ScanName:
+      switch (CurrentState) {
+      case State::ScanName:
         if (C == BeginArgsToken) {
           // Save pass name for later and begin scanning args.
           PassName = Pipeline.slice(PassBeginIdx, Idx);
           ArgsBeginIdx = Idx + 1;
           ++NestedArgs;
-          State = ScanArgs;
+          CurrentState = State::ScanArgs;
           break;
         }
         if (C == EndArgsToken) {
@@ -134,7 +138,7 @@ class PassManager : public ParentPass {
           PassBeginIdx = Idx + 1;
         }
         break;
-      case ScanArgs:
+      case State::ScanArgs:
         // While scanning args, we only care about making sure nesting of angle
         // brackets is correct.
         if (C == BeginArgsToken) {
@@ -145,11 +149,10 @@ class PassManager : public ParentPass {
           --NestedArgs;
           if (NestedArgs == 0) {
             // Done scanning args.
-            PassArgs = Pipeline.slice(ArgsBeginIdx, Idx);
-            AddPass(PassName, PassArgs);
-            State = ArgsEnded;
+            AddPass(PassName, Pipeline.slice(ArgsBeginIdx, Idx));
+            CurrentState = State::ArgsEnded;
           } else if (NestedArgs < 0) {
-            errs() << "Unbalanced '>' in pass pipeline.\n";
+            errs() << "Unexpected '>' in pass pipeline.\n";
             exit(1);
           }
           break;
@@ -161,12 +164,12 @@ class PassManager : public ParentPass {
           exit(1);
         }
         break;
-      case ArgsEnded:
+      case State::ArgsEnded:
         // Once we're done scanning args, only a delimiter is valid. This avoids
         // accepting strings like "foo<args><more-args>" or "foo<args>bar".
         if (C == EndToken || C == PassDelimToken) {
           PassBeginIdx = Idx + 1;
-          State = ScanName;
+          CurrentState = State::ScanName;
         } else {
           errs() << "Expected delimiter or end-of-string after pass "
                     "arguments.\n";
@@ -202,12 +205,18 @@ class FunctionPassManager final
     : public PassManager<FunctionPass, FunctionPass> {
 public:
   FunctionPassManager(StringRef Name) : PassManager(Name) {}
+  FunctionPassManager(StringRef Name, StringRef Pipeline,
+                      CreatePassFunc CreatePass)
+      : PassManager(Name, Pipeline, CreatePass) {}
   bool runOnFunction(Function &F) final;
 };
 
 class RegionPassManager final : public PassManager<RegionPass, RegionPass> {
 public:
   RegionPassManager(StringRef Name) : PassManager(Name) {}
+  RegionPassManager(StringRef Name, StringRef Pipeline,
+                    CreatePassFunc CreatePass)
+      : PassManager(Name, Pipeline, CreatePass) {}
   bool runOnRegion(Region &R) final;
 };
 

llvm/lib/Transforms/Vectorize/SandboxVectorizer/Passes/BottomUpVec.cpp

@@ -16,9 +16,8 @@
 namespace llvm::sandboxir {
 
 BottomUpVec::BottomUpVec(StringRef Pipeline)
-    : FunctionPass("bottom-up-vec"), RPM("rpm") {
-  RPM.setPassPipeline(Pipeline, SandboxVectorizerPassBuilder::createRegionPass);
-}
+    : FunctionPass("bottom-up-vec"),
+      RPM("rpm", Pipeline, SandboxVectorizerPassBuilder::createRegionPass) {}
 
 // TODO: This is a temporary function that returns some seeds.
 //       Replace this with SeedCollector's function when it lands.

llvm/lib/Transforms/Vectorize/SandboxVectorizer/Passes/RegionsFromMetadata.cpp

@@ -14,9 +14,8 @@
 namespace llvm::sandboxir {
 
 RegionsFromMetadata::RegionsFromMetadata(StringRef Pipeline)
-    : FunctionPass("regions-from-metadata"), RPM("rpm") {
-  RPM.setPassPipeline(Pipeline, SandboxVectorizerPassBuilder::createRegionPass);
-}
+    : FunctionPass("regions-from-metadata"),
+      RPM("rpm", Pipeline, SandboxVectorizerPassBuilder::createRegionPass) {}
 
 bool RegionsFromMetadata::runOnFunction(Function &F) {
   SmallVector<std::unique_ptr<sandboxir::Region>> Regions =

llvm/unittests/SandboxIR/PassTest.cpp

@@ -319,16 +319,27 @@ define void @f() {
                ".*not registered.*");
   EXPECT_DEATH(FPM2.setPassPipeline(",", CreatePass), ".*empty pass name.*");
   EXPECT_DEATH(FPM2.setPassPipeline("<>", CreatePass), ".*empty pass name.*");
+  EXPECT_DEATH(FPM2.setPassPipeline("<>foo", CreatePass),
+               ".*empty pass name.*");
   EXPECT_DEATH(FPM2.setPassPipeline("foo,<>", CreatePass),
                ".*empty pass name.*");
 
   // Mismatched argument brackets.
-  EXPECT_DEATH(FPM2.setPassPipeline("foo<", CreatePass), ".*");
-  EXPECT_DEATH(FPM2.setPassPipeline("foo>", CreatePass), ".*");
-  EXPECT_DEATH(FPM2.setPassPipeline("foo<bar<>", CreatePass), ".*");
-  EXPECT_DEATH(FPM2.setPassPipeline("foo<bar<>>>", CreatePass), ".*");
-  EXPECT_DEATH(FPM2.setPassPipeline(">foo", CreatePass), ".*");
-  EXPECT_DEATH(FPM2.setPassPipeline("<>foo", CreatePass), ".*");
-  EXPECT_DEATH(FPM2.setPassPipeline("foo<args><more-args>", CreatePass), ".*");
-  EXPECT_DEATH(FPM2.setPassPipeline("foo<args>bar", CreatePass), ".*");
+  EXPECT_DEATH(FPM2.setPassPipeline("foo<", CreatePass), ".*Missing '>'.*");
+  EXPECT_DEATH(FPM2.setPassPipeline("foo<bar", CreatePass), ".*Missing '>'.*");
+  EXPECT_DEATH(FPM2.setPassPipeline("foo<bar<>", CreatePass),
+               ".*Missing '>'.*");
+  EXPECT_DEATH(FPM2.setPassPipeline("foo>", CreatePass), ".*Unexpected '>'.*");
+  EXPECT_DEATH(FPM2.setPassPipeline(">foo", CreatePass), ".*Unexpected '>'.*");
+  // Extra garbage between args and next delimiter/end-of-string.
+  EXPECT_DEATH(FPM2.setPassPipeline("foo<bar<>>>", CreatePass),
+               ".*Expected delimiter.*");
+  EXPECT_DEATH(FPM2.setPassPipeline("bar<>foo", CreatePass),
+               ".*Expected delimiter.*");
+  EXPECT_DEATH(FPM2.setPassPipeline("bar<>foo,baz", CreatePass),
+               ".*Expected delimiter.*");
+  EXPECT_DEATH(FPM2.setPassPipeline("foo<args><more-args>", CreatePass),
+               ".*Expected delimiter.*");
+  EXPECT_DEATH(FPM2.setPassPipeline("foo<args>bar", CreatePass),
+               ".*Expected delimiter.*");
 }