[Mlir-commits] [mlir] [mlir] Attempt to resolve edge cases in PassPipeline textual format (PR #118877)
Christopher Bate
llvmlistbot at llvm.org
Thu Dec 5 14:59:28 PST 2024
https://github.com/christopherbate updated https://github.com/llvm/llvm-project/pull/118877
>From a892f8d6989afc9d07307865b808cdd29546feea Mon Sep 17 00:00:00 2001
From: Christopher Bate <cbate at nvidia.com>
Date: Thu, 5 Dec 2024 22:37:26 +0000
Subject: [PATCH] [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
https://github.com/llvm/llvm-project/issues/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.
---
mlir/include/mlir/Pass/PassOptions.h | 23 ++++++--
mlir/lib/Pass/PassRegistry.cpp | 61 ++++++++++++--------
mlir/test/Pass/pipeline-options-parsing.mlir | 24 ++++++++
mlir/test/lib/Pass/TestPassManager.cpp | 4 +-
4 files changed, 82 insertions(+), 30 deletions(-)
diff --git a/mlir/include/mlir/Pass/PassOptions.h b/mlir/include/mlir/Pass/PassOptions.h
index a5a3f1c1c19652..b5a9c25e3baca5 100644
--- a/mlir/include/mlir/Pass/PassOptions.h
+++ b/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) {
diff --git a/mlir/lib/Pass/PassRegistry.cpp b/mlir/lib/Pass/PassRegistry.cpp
index fe842755958418..167abdc4dddab2 100644
--- a/mlir/lib/Pass/PassRegistry.cpp
+++ b/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) {
diff --git a/mlir/test/Pass/pipeline-options-parsing.mlir b/mlir/test/Pass/pipeline-options-parsing.mlir
index b6c2b688b7cfb3..9c111683bde57a 100644
--- a/mlir/test/Pass/pipeline-options-parsing.mlir
+++ b/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,11 @@
// 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={,}}))
+
+
+func.func @func() {
+ return
+}
\ No newline at end of file
diff --git a/mlir/test/lib/Pass/TestPassManager.cpp b/mlir/test/lib/Pass/TestPassManager.cpp
index 7afe2109f04db3..a99e4cecdc88e5 100644
--- a/mlir/test/lib/Pass/TestPassManager.cpp
+++ b/mlir/test/lib/Pass/TestPassManager.cpp
@@ -85,7 +85,9 @@ struct TestOptionsPass
enumOption = options.enumOption;
}
- void runOnOperation() final {}
+ void runOnOperation() final {
+ llvm::errs() << "string-list has size " << stringListOption.size() << "\n";
+ }
StringRef getArgument() const final { return "test-options-pass"; }
StringRef getDescription() const final {
return "Test options parsing capabilities";
More information about the Mlir-commits
mailing list