[mlir] Attempt to resolve edge cases in PassPipeline textual format #118877
Conversation
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-core Author: Christopher Bate (christopherbate) ChangesThis commit makes the following changes:
Full diff: https://github.com/llvm/llvm-project/pull/118877.diff 5 Files Affected:
diff --git a/mlir/include/mlir/Pass/PassOptions.h b/mlir/include/mlir/Pass/PassOptions.h
index a5a3f1c1c19652..3312f1c11b0bad 100644
--- a/mlir/include/mlir/Pass/PassOptions.h
+++ b/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) {
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..c23c3a2e3d0d0c 100644
--- a/mlir/test/Pass/pipeline-options-parsing.mlir
+++ b/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
+
diff --git a/mlir/test/Transforms/inlining-dump-default-pipeline.mlir b/mlir/test/Transforms/inlining-dump-default-pipeline.mlir
index 4f8638054206e8..102310400e9d0b 100644
--- a/mlir/test/Transforms/inlining-dump-default-pipeline.mlir
+++ b/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={} })
diff --git a/mlir/test/lib/Pass/TestPassManager.cpp b/mlir/test/lib/Pass/TestPassManager.cpp
index 7afe2109f04db3..a350a2f0931be7 100644
--- a/mlir/test/lib/Pass/TestPassManager.cpp
+++ b/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{
|
|
I updated the behavior, description, and tests to explicity demonstrate that constructing a |
fdecfbb to
a892f8d
Compare
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
llvm#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.
a892f8d to
04d231a
Compare
|
This PR seems to change the behavior of a string option with no default, when passing the empty string |
|
Wouldn't this be: |
Cf. google@ccba176#diff-6f8c01b4d228f97d2a800c5b5deca07240dd1203a0063e775b6a9f8e49cc9b9c for the change that introduced this, due to a failure caused by upstream changes. llvm/llvm-project#118877 (comment) for the suggested correct form.
Cf. google@ccba176#diff-6f8c01b4d228f97d2a800c5b5deca07240dd1203a0063e775b6a9f8e49cc9b9c for the change that introduced this, due to a failure caused by upstream changes. llvm/llvm-project#118877 (comment) for the suggested correct form.
|
@joker-eph that method still seems to fail. It seems that to define an empty string I have to actually use the See for example google/heir@ccba176#diff-6f8c01b4d228f97d2a800c5b5deca07240dd1203a0063e775b6a9f8e49cc9b9c the file tests/Dialect/Polynomial/Transforms/elementwise_to_affine.mlir which corresponds to the option defined here: https://github.com/google/heir/blob/276bc7c53ad9b074ad3b8dde2756b1980e4c2707/lib/Transforms/ElementwiseToAffine/ElementwiseToAffine.td#L31 I am on vacation for a few weeks so I may not have time to repro this upstream. |
|
Sorry just saw the post-merge discussion.
I don't think it was intended that the syntax be changed for plain string options. An option of type
The example you gave in your last message is a |
|
@christopherbate indeed, the option is defined as I guess it's not so important to preserve that particular syntax, if an unset string option cannot be distinguished from an option set to empty string anyway. It's a bit weird that the syntax is invalid now, and also weird that setting it to an empty list (even though it's a string option) is valid. |
This commit makes the following changes:
Previously certain pipeline options could cause the options parser to
get stuck in an an infinite loop. An example is:
In this example, the 'list' option of the
test-options-super-passis itself a pass options specification (this capability was added in
[mlir] Add support for parsing nested PassPipelineOptions #101118).
However, while the textual format allows
ListOption<int>to be givenas
list=1,2,3, it did not allow the same format forListOption<T>when T is a subclass ofPassOptionswithout extraenclosing
{....}. Lack of enclosing{...}would cause the infinitelooping in the parser.
This change resolves the parser bug and also allows omitting the
outer
{...}forListOption-of-options.Previously, if you specified a default list value for your
ListOption, e.g.ListOption<int> opt{*this, "list", llvm::cl::list_init({1,2,3})},it would be impossible to override that default value of
{1,2,3}withan 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=""}ormy-pass{string-list={}}is interpreted as "empty list". This makes it imposssible to specify
that ListOption
string-listshould be a size-1 list containing theempty string. However,
my-pass{string-list={"",""}}does specifya size-2 list containing the empty string. This behavior seems preferable
to allow for overriding non-empty defaults as described above.