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

mlir/include/mlir/Pass/PassOptions.h

@@ -253,6 +253,11 @@ class PassOptions : protected llvm::cl::SubCommand {
       assert(!(this->getMiscFlags() & llvm::cl::MiscFlags::CommaSeparated) &&
              "ListOption is implicitly comma separated, specifying "
              "CommaSeparated is extraneous");
+
+      // Make the default explicitly "empty" if no default was given.
+      if (!this->isDefaultAssigned())
+        this->setInitialValues({});
+
       parent.options.push_back(this);
       elementParser.initialize();
     }
@@ -296,11 +301,21 @@ class PassOptions : protected llvm::cl::SubCommand {
     const llvm::cl::Option *getOption() const final { return this; }
 
     /// Print the name and value of this option to the given stream.
+    /// Note that there is currently a limitation with regards to
+    /// `ListOption<string>`: parsing 'option=""` will result in `option` being
+    /// set to the empty list, not to a size-1 list containing an empty string.
     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.
+      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,30 @@ const PassPipelineInfo *mlir::PassPipelineInfo::lookup(StringRef pipelineArg) {
 // PassOptions
 //===----------------------------------------------------------------------===//
 
+/// Attempt to find the next occurance of character 'c' in the string starting
+/// from the `index`-th position , omitting any occurances that appear within
+/// intervening ranges or literals.
+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 +218,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,22 @@
 // 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={} string=bar}' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_8 %s
+
+// It is not possible to specify a size-1 list of empty string.
+// It is possible to specify a size > 1 list of empty strings.
+// RUN: mlir-opt %s -verify-each=false '--pass-pipeline=builtin.module(func.func(test-options-pass{string-list={""}}))' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_9 %s
+// RUN: mlir-opt %s -verify-each=false '--pass-pipeline=builtin.module(func.func(test-options-pass{string-list={,}}))' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_10 %s
+// RUN: mlir-opt %s -verify-each=false '--pass-pipeline=builtin.module(func.func(test-options-pass{string-list={"",}}))' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_10 %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
@@ -27,3 +43,6 @@
 // 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_8{LITERAL}: builtin.module(func.func(test-options-super-pass{list={{enum=zero list={1} string=foo },{enum=one string=bar }}}))
+// CHECK_9: builtin.module(func.func(test-options-pass{enum=zero  string= string-list={}}))
+// CHECK_10: builtin.module(func.func(test-options-pass{enum=zero  string= string-list={,}}))