[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 change makes it explicit in the ListOption construction that we
   would like to treat all ListOption as having a default value of "empty"
   unless otherwise specified (e.g. using `llvm::list_init`).

   It removes the requirement that lists are not printed if empty. Instead,
   lists are not printed if they do not have their default value.

   It is now clarified that the textual format
   `my-pass{string-list=""}` or `my-pass{string-list={}}`
   is interpreted as "empty list". This makes it imposssible to specify
   that ListOption `string-list` should be a size-1 list containing the
   empty string. However, `my-pass{string-list={"",""}}` *does* specify
   a size-2 list containing the empty string. This behavior seems preferable
   to allow for overriding non-empty defaults as described above.

Author: christopherbate

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={,}}))