[Mlir-commits] [mlir] [mlir] Attempt to resolve edge cases in PassPipeline textual format (PR #118877)
Christopher Bate
llvmlistbot at llvm.org
Thu Dec 5 13:48:51 PST 2024
https://github.com/christopherbate created https://github.com/llvm/llvm-project/pull/118877
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 updates this behvior so that specifying empty lists (either
`list={}` or just `list=` is allowed.
>From fdecfbb12f355be4088a06f033fde7830c7d93f2 Mon Sep 17 00:00:00 2001
From: Christopher Bate <cbate at nvidia.com>
Date: Thu, 5 Dec 2024 21:37:14 +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 updates this behvior so that specifying empty lists (either
`list={}` or just `list=` is allowed.
---
mlir/include/mlir/Pass/PassOptions.h | 17 ++++--
mlir/lib/Pass/PassRegistry.cpp | 61 +++++++++++--------
mlir/test/Pass/pipeline-options-parsing.mlir | 19 +++++-
.../inlining-dump-default-pipeline.mlir | 2 +-
mlir/test/lib/Pass/TestPassManager.cpp | 10 ++-
5 files changed, 76 insertions(+), 33 deletions(-)
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{
More information about the Mlir-commits
mailing list