[mlir] Attempt to resolve edge cases in PassPipeline textual format

This commit makes the following changes:

1. Previously certain pipeline options could cause the options parser to
   get stuck in an an infinite loop. An example is:

   ```
   mlir-opt %s -verify-each=false -pass-pipeline='builtin.module(func.func(test-options-super-pass{list={list=1,2},{list=3,4}}))''
   ```

   In this example, the 'list' option of the `test-options-super-pass`
   is itself a pass options specification (this capability was added in
   #101118).

   However, while the textual format allows `ListOption<int>` to be given
   as `list=1,2,3`, it did not allow the same format for
   `ListOption<T>` when T is a subclass of `PassOptions` without extra
   enclosing `{....}`. Lack of enclosing `{...}` would cause the infinite
   looping in the parser.

   This change resolves the parser bug and also allows omitting the
   outer `{...}` for `ListOption`-of-options.

2. Previously, if you specified a default list value for your
   `ListOption`, e.g. `ListOption<int> opt{*this, "list", llvm::list_init({1,2,3})}`,
   it would be impossible to override that default value of `{1,2,3}` with
   an *empty* list on the command line, since `my-pass{list=}` was not allowed.

   This was not allowed because of ambiguous handling of lists-of-strings
   (no literal marker is currently required).

   This updates this behvior so that specifying empty lists (either
   `list={}` or just `list=` is allowed.

Author: christopherbate

mlir/include/mlir/Pass/PassOptions.h

@@ -19,6 +19,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compiler.h"
+#include "llvm/Support/Debug.h"
 #include <memory>
 
 namespace mlir {
@@ -297,10 +298,18 @@ class PassOptions : protected llvm::cl::SubCommand {
 
     /// Print the name and value of this option to the given stream.
     void print(raw_ostream &os) final {
-      // Don't print the list if empty. An empty option value can be treated as
-      // an element of the list in certain cases (e.g. ListOption<std::string>).
-      if ((**this).empty())
-        return;
+      // Don't print the list if the value is the default value. An empty
+      // option value should never be treated as an empty list.
+      if (this->isDefaultAssigned() &&
+          this->getDefault().size() == (**this).size()) {
+        unsigned i = 0;
+        for (unsigned e = (**this).size(); i < e; i++) {
+          if (!this->getDefault()[i].compare((**this)[i]))
+            break;
+        }
+        if (i == (**this).size())
+          return;
+      }
 
       os << this->ArgStr << "={";
       auto printElementFn = [&](const DataType &value) {

mlir/lib/Pass/PassRegistry.cpp

@@ -186,6 +186,27 @@ const PassPipelineInfo *mlir::PassPipelineInfo::lookup(StringRef pipelineArg) {
 // PassOptions
 //===----------------------------------------------------------------------===//
 
+static size_t findChar(StringRef str, size_t index, char c) {
+  for (size_t i = index, e = str.size(); i < e; ++i) {
+    if (str[i] == c)
+      return i;
+    // Check for various range characters.
+    if (str[i] == '{')
+      i = findChar(str, i + 1, '}');
+    else if (str[i] == '(')
+      i = findChar(str, i + 1, ')');
+    else if (str[i] == '[')
+      i = findChar(str, i + 1, ']');
+    else if (str[i] == '\"')
+      i = str.find_first_of('\"', i + 1);
+    else if (str[i] == '\'')
+      i = str.find_first_of('\'', i + 1);
+    if (i == StringRef::npos)
+      return StringRef::npos;
+  }
+  return StringRef::npos;
+}
+
 /// Extract an argument from 'options' and update it to point after the arg.
 /// Returns the cleaned argument string.
 static StringRef extractArgAndUpdateOptions(StringRef &options,
@@ -194,47 +215,37 @@ static StringRef extractArgAndUpdateOptions(StringRef &options,
   options = options.drop_front(argSize).ltrim();
 
   // Early exit if there's no escape sequence.
-  if (str.size() <= 2)
+  if (str.size() <= 1)
     return str;
 
   const auto escapePairs = {std::make_pair('\'', '\''),
-                            std::make_pair('"', '"'), std::make_pair('{', '}')};
+                            std::make_pair('"', '"')};
   for (const auto &escape : escapePairs) {
     if (str.front() == escape.first && str.back() == escape.second) {
       // Drop the escape characters and trim.
-      str = str.drop_front().drop_back().trim();
       // Don't process additional escape sequences.
-      break;
+      return str.drop_front().drop_back().trim();
     }
   }
 
+  // Arguments may be wrapped in `{...}`. Unlike the quotation markers that
+  // denote literals, we respect scoping here. The outer `{...}` should not
+  // be stripped in cases such as "arg={...},{...}", which can be used to denote
+  // lists of nested option structs.
+  if (str.front() == '{') {
+    unsigned match = findChar(str, 1, '}');
+    if (match == str.size() - 1)
+      str = str.drop_front().drop_back().trim();
+  }
+
   return str;
 }
 
 LogicalResult detail::pass_options::parseCommaSeparatedList(
     llvm::cl::Option &opt, StringRef argName, StringRef optionStr,
     function_ref<LogicalResult(StringRef)> elementParseFn) {
-  // Functor used for finding a character in a string, and skipping over
-  // various "range" characters.
-  llvm::unique_function<size_t(StringRef, size_t, char)> findChar =
-      [&](StringRef str, size_t index, char c) -> size_t {
-    for (size_t i = index, e = str.size(); i < e; ++i) {
-      if (str[i] == c)
-        return i;
-      // Check for various range characters.
-      if (str[i] == '{')
-        i = findChar(str, i + 1, '}');
-      else if (str[i] == '(')
-        i = findChar(str, i + 1, ')');
-      else if (str[i] == '[')
-        i = findChar(str, i + 1, ']');
-      else if (str[i] == '\"')
-        i = str.find_first_of('\"', i + 1);
-      else if (str[i] == '\'')
-        i = str.find_first_of('\'', i + 1);
-    }
-    return StringRef::npos;
-  };
+  if (optionStr.empty())
+    return success();
 
   size_t nextElePos = findChar(optionStr, 0, ',');
   while (nextElePos != StringRef::npos) {

mlir/test/Pass/pipeline-options-parsing.mlir

@@ -14,6 +14,19 @@
 // RUN: mlir-opt %s -verify-each=false '-test-options-super-pass-pipeline=super-list={{enum=zero list=1 string=foo},{enum=one list=2 string="bar"},{enum=two list=3 string={baz}}}' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_7 %s
 // RUN: mlir-opt %s -verify-each=false -pass-pipeline='builtin.module(func.func(test-options-super-pass{list={{enum=zero list={1} string=foo },{enum=one list={2} string=bar },{enum=two list={3} string=baz }}}))' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_7 %s
 
+
+// This test checks that lists-of-nested-options like 'option1={...},{....}' can be parsed
+// just like how 'option=1,2,3' is also allowed:
+
+// RUN: mlir-opt %s -verify-each=false -pass-pipeline='builtin.module(func.func(test-options-super-pass{list={enum=zero list={1} string=foo },{enum=one list={2} string=bar },{enum=two list={3} string=baz }}))' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_7 %s
+
+// This test checks that it is legal to specify an empty list using '{}'.
+// RUN: mlir-opt %s -verify-each=false '--test-options-super-pass=list={enum=zero list={1} string=foo},{enum=one list={} default-list= string=bar}' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_8 %s
+
+// This test checks that it is possible to specify lists of empty strings.
+// RUN: mlir-opt %s -verify-each=false '--test-options-pass=string-list="",""' '--test-options-pass=string-list=""' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_9 %s
+
+
 // CHECK_ERROR_1: missing closing '}' while processing pass options
 // CHECK_ERROR_2: no such option test-option
 // CHECK_ERROR_3: no such option invalid-option
@@ -26,4 +39,8 @@
 // CHECK_4: builtin.module(builtin.module(func.func(test-options-pass{enum=zero list={3} string= }),func.func(test-options-pass{enum=one list={1,2,3,4} string=foobar })))
 // CHECK_5: builtin.module(builtin.module(func.func(test-options-pass{enum=zero list={3} string= }),func.func(test-options-pass{enum=one list={1,2,3,4} string={foo bar baz} })))
 // CHECK_6: builtin.module(builtin.module(func.func(test-options-pass{enum=zero list={3} string= }),func.func(test-options-pass{enum=one list={1,2,3,4} string=foo"bar"baz })))
-// CHECK_7{LITERAL}: builtin.module(func.func(test-options-super-pass{list={{enum=zero list={1} string=foo },{enum=one list={2} string=bar },{enum=two list={3} string=baz }}}))
+// CHECK_7{LITERAL}: builtin.module(func.func(test-options-super-pass{list={{ enum=zero list={1} string=foo },{ enum=one list={2} string=bar },{ enum=two list={3} string=baz }}}))
+
+// CHECK_8{LITERAL}: builtin.module(func.func(test-options-super-pass{list={{ enum=zero list={1} string=foo },{default-list={} enum=one list={} string=bar }}}))
+// CHECK_9: builtin
+

mlir/test/Transforms/inlining-dump-default-pipeline.mlir

@@ -1,2 +1,2 @@
 // RUN: mlir-opt %s -pass-pipeline="builtin.module(inline)" -dump-pass-pipeline 2>&1 | FileCheck %s
-// CHECK: builtin.module(inline{default-pipeline=canonicalize inlining-threshold=4294967295 max-iterations=4 })
+// CHECK: builtin.module(inline{default-pipeline=canonicalize inlining-threshold=4294967295 max-iterations=4 op-pipelines={} })

mlir/test/lib/Pass/TestPassManager.cpp

@@ -59,8 +59,13 @@ struct TestOptionsPass
   struct Options : public PassPipelineOptions<Options> {
     ListOption<int> listOption{*this, "list",
                                llvm::cl::desc("Example list option")};
+    ListOption<int> listWithDefaultsOption{
+        *this, "default-list",
+        llvm::cl::desc("Example list option with defaults"),
+        llvm::cl::list_init<int>({10, 9, 8})};
     ListOption<std::string> stringListOption{
-        *this, "string-list", llvm::cl::desc("Example string list option")};
+        *this, "string-list", llvm::cl::desc("Example string list option"),
+        llvm::cl::list_init<std::string>({})};
     Option<std::string> stringOption{*this, "string",
                                      llvm::cl::desc("Example string option")};
     Option<Enum> enumOption{
@@ -94,7 +99,8 @@ struct TestOptionsPass
   ListOption<int> listOption{*this, "list",
                              llvm::cl::desc("Example list option")};
   ListOption<std::string> stringListOption{
-      *this, "string-list", llvm::cl::desc("Example string list option")};
+      *this, "string-list", llvm::cl::desc("Example string list option"),
+      llvm::cl::list_init<std::string>({})};
   Option<std::string> stringOption{*this, "string",
                                    llvm::cl::desc("Example string option")};
   Option<Enum> enumOption{