[mlir] Pass Options ownership modifications #110582
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-core Author: Nikhil Kalra (nikalra) ChangesThis change makes two (related) changes: First, it updates the tablegen option for Second, it updates the generated constructors for Passes to consume options by value instead of reference, and prefers moving options into the pass itself. This should be more efficient for non-trivial options objects, where the previous interface forced a copy to be materialized. Now, at worst case the API materializes a copy (no worse than before); at best-case, all options objects are moved into place. Ideally, we could update the Pass constructor to take an r-value reference to the Options object instead, but this approach will require numerous changes to existing passes and their factory functions. Full diff: https://github.com/llvm/llvm-project/pull/110582.diff 4 Files Affected:
diff --git a/mlir/include/mlir/Transforms/Passes.h b/mlir/include/mlir/Transforms/Passes.h
index 8e4a43c3f24586..5c977055e95dc8 100644
--- a/mlir/include/mlir/Transforms/Passes.h
+++ b/mlir/include/mlir/Transforms/Passes.h
@@ -15,6 +15,7 @@
#define MLIR_TRANSFORMS_PASSES_H
#include "mlir/Pass/Pass.h"
+#include "mlir/Pass/PassManager.h"
#include "mlir/Transforms/GreedyPatternRewriteDriver.h"
#include "mlir/Transforms/LocationSnapshot.h"
#include "mlir/Transforms/ViewOpGraph.h"
diff --git a/mlir/lib/Dialect/Arith/Transforms/IntNarrowing.cpp b/mlir/lib/Dialect/Arith/Transforms/IntNarrowing.cpp
index 70fd9bc0a1e68f..8b2e671038b362 100644
--- a/mlir/lib/Dialect/Arith/Transforms/IntNarrowing.cpp
+++ b/mlir/lib/Dialect/Arith/Transforms/IntNarrowing.cpp
@@ -44,7 +44,8 @@ struct NarrowingPattern : OpRewritePattern<SourceOp> {
NarrowingPattern(MLIRContext *ctx, const ArithIntNarrowingOptions &options,
PatternBenefit benefit = 1)
: OpRewritePattern<SourceOp>(ctx, benefit),
- supportedBitwidths(options.bitwidthsSupported) {
+ supportedBitwidths(options.bitwidthsSupported.begin(),
+ options.bitwidthsSupported.end()) {
assert(!supportedBitwidths.empty() && "Invalid options");
assert(!llvm::is_contained(supportedBitwidths, 0) && "Invalid bitwidth");
llvm::sort(supportedBitwidths);
@@ -757,7 +758,8 @@ struct ArithIntNarrowingPass final
MLIRContext *ctx = op->getContext();
RewritePatternSet patterns(ctx);
populateArithIntNarrowingPatterns(
- patterns, ArithIntNarrowingOptions{bitwidthsSupported});
+ patterns, ArithIntNarrowingOptions{SmallVector<unsigned>{
+ bitwidthsSupported.begin(), bitwidthsSupported.end()}});
if (failed(applyPatternsAndFoldGreedily(op, std::move(patterns))))
signalPassFailure();
}
diff --git a/mlir/tools/mlir-tblgen/PassGen.cpp b/mlir/tools/mlir-tblgen/PassGen.cpp
index 90aa67115a4007..655843f26201ac 100644
--- a/mlir/tools/mlir-tblgen/PassGen.cpp
+++ b/mlir/tools/mlir-tblgen/PassGen.cpp
@@ -97,7 +97,7 @@ static void emitPassOptionsStruct(const Pass &pass, raw_ostream &os) {
std::string type = opt.getType().str();
if (opt.isListOption())
- type = "::llvm::ArrayRef<" + type + ">";
+ type = "::llvm::SmallVector<" + type + ">";
os.indent(2) << llvm::formatv("{0} {1}", type, opt.getCppVariableName());
@@ -128,8 +128,8 @@ static void emitPassDecls(const Pass &pass, raw_ostream &os) {
// Declaration of the constructor with options.
if (ArrayRef<PassOption> options = pass.getOptions(); !options.empty())
- os << llvm::formatv("std::unique_ptr<::mlir::Pass> create{0}(const "
- "{0}Options &options);\n",
+ os << llvm::formatv("std::unique_ptr<::mlir::Pass> create{0}("
+ "{0}Options options);\n",
passName);
}
@@ -236,7 +236,7 @@ namespace impl {{
const char *const friendDefaultConstructorWithOptionsDeclTemplate = R"(
namespace impl {{
- std::unique_ptr<::mlir::Pass> create{0}(const {0}Options &options);
+ std::unique_ptr<::mlir::Pass> create{0}({0}Options options);
} // namespace impl
)";
@@ -247,8 +247,8 @@ const char *const friendDefaultConstructorDefTemplate = R"(
)";
const char *const friendDefaultConstructorWithOptionsDefTemplate = R"(
- friend std::unique_ptr<::mlir::Pass> create{0}(const {0}Options &options) {{
- return std::make_unique<DerivedT>(options);
+ friend std::unique_ptr<::mlir::Pass> create{0}({0}Options options) {{
+ return std::make_unique<DerivedT>(std::move(options));
}
)";
@@ -259,8 +259,8 @@ std::unique_ptr<::mlir::Pass> create{0}() {{
)";
const char *const defaultConstructorWithOptionsDefTemplate = R"(
-std::unique_ptr<::mlir::Pass> create{0}(const {0}Options &options) {{
- return impl::create{0}(options);
+std::unique_ptr<::mlir::Pass> create{0}({0}Options options) {{
+ return impl::create{0}(std::move(options));
}
)";
@@ -326,10 +326,10 @@ static void emitPassDefs(const Pass &pass, raw_ostream &os) {
if (ArrayRef<PassOption> options = pass.getOptions(); !options.empty()) {
os.indent(2) << llvm::formatv(
- "{0}Base(const {0}Options &options) : {0}Base() {{\n", passName);
+ "{0}Base({0}Options options) : {0}Base() {{\n", passName);
for (const PassOption &opt : pass.getOptions())
- os.indent(4) << llvm::formatv("{0} = options.{0};\n",
+ os.indent(4) << llvm::formatv("{0} = std::move(options.{0});\n",
opt.getCppVariableName());
os.indent(2) << "}\n";
diff --git a/mlir/unittests/TableGen/PassGenTest.cpp b/mlir/unittests/TableGen/PassGenTest.cpp
index 859eba7e517869..5d2e7f47624caf 100644
--- a/mlir/unittests/TableGen/PassGenTest.cpp
+++ b/mlir/unittests/TableGen/PassGenTest.cpp
@@ -72,7 +72,7 @@ TEST(PassGenTest, PassOptions) {
TestPassWithOptionsOptions options;
options.testOption = 57;
- llvm::SmallVector<int64_t, 2> testListOption = {1, 2};
+ llvm::SmallVector<int64_t> testListOption = {1, 2};
options.testListOption = testListOption;
const auto unwrap = [](const std::unique_ptr<mlir::Pass> &pass) {
|
|
@llvm/pr-subscribers-mlir-arith Author: Nikhil Kalra (nikalra) ChangesThis change makes two (related) changes: First, it updates the tablegen option for Second, it updates the generated constructors for Passes to consume options by value instead of reference, and prefers moving options into the pass itself. This should be more efficient for non-trivial options objects, where the previous interface forced a copy to be materialized. Now, at worst case the API materializes a copy (no worse than before); at best-case, all options objects are moved into place. Ideally, we could update the Pass constructor to take an r-value reference to the Options object instead, but this approach will require numerous changes to existing passes and their factory functions. Full diff: https://github.com/llvm/llvm-project/pull/110582.diff 4 Files Affected:
diff --git a/mlir/include/mlir/Transforms/Passes.h b/mlir/include/mlir/Transforms/Passes.h
index 8e4a43c3f24586..5c977055e95dc8 100644
--- a/mlir/include/mlir/Transforms/Passes.h
+++ b/mlir/include/mlir/Transforms/Passes.h
@@ -15,6 +15,7 @@
#define MLIR_TRANSFORMS_PASSES_H
#include "mlir/Pass/Pass.h"
+#include "mlir/Pass/PassManager.h"
#include "mlir/Transforms/GreedyPatternRewriteDriver.h"
#include "mlir/Transforms/LocationSnapshot.h"
#include "mlir/Transforms/ViewOpGraph.h"
diff --git a/mlir/lib/Dialect/Arith/Transforms/IntNarrowing.cpp b/mlir/lib/Dialect/Arith/Transforms/IntNarrowing.cpp
index 70fd9bc0a1e68f..8b2e671038b362 100644
--- a/mlir/lib/Dialect/Arith/Transforms/IntNarrowing.cpp
+++ b/mlir/lib/Dialect/Arith/Transforms/IntNarrowing.cpp
@@ -44,7 +44,8 @@ struct NarrowingPattern : OpRewritePattern<SourceOp> {
NarrowingPattern(MLIRContext *ctx, const ArithIntNarrowingOptions &options,
PatternBenefit benefit = 1)
: OpRewritePattern<SourceOp>(ctx, benefit),
- supportedBitwidths(options.bitwidthsSupported) {
+ supportedBitwidths(options.bitwidthsSupported.begin(),
+ options.bitwidthsSupported.end()) {
assert(!supportedBitwidths.empty() && "Invalid options");
assert(!llvm::is_contained(supportedBitwidths, 0) && "Invalid bitwidth");
llvm::sort(supportedBitwidths);
@@ -757,7 +758,8 @@ struct ArithIntNarrowingPass final
MLIRContext *ctx = op->getContext();
RewritePatternSet patterns(ctx);
populateArithIntNarrowingPatterns(
- patterns, ArithIntNarrowingOptions{bitwidthsSupported});
+ patterns, ArithIntNarrowingOptions{SmallVector<unsigned>{
+ bitwidthsSupported.begin(), bitwidthsSupported.end()}});
if (failed(applyPatternsAndFoldGreedily(op, std::move(patterns))))
signalPassFailure();
}
diff --git a/mlir/tools/mlir-tblgen/PassGen.cpp b/mlir/tools/mlir-tblgen/PassGen.cpp
index 90aa67115a4007..655843f26201ac 100644
--- a/mlir/tools/mlir-tblgen/PassGen.cpp
+++ b/mlir/tools/mlir-tblgen/PassGen.cpp
@@ -97,7 +97,7 @@ static void emitPassOptionsStruct(const Pass &pass, raw_ostream &os) {
std::string type = opt.getType().str();
if (opt.isListOption())
- type = "::llvm::ArrayRef<" + type + ">";
+ type = "::llvm::SmallVector<" + type + ">";
os.indent(2) << llvm::formatv("{0} {1}", type, opt.getCppVariableName());
@@ -128,8 +128,8 @@ static void emitPassDecls(const Pass &pass, raw_ostream &os) {
// Declaration of the constructor with options.
if (ArrayRef<PassOption> options = pass.getOptions(); !options.empty())
- os << llvm::formatv("std::unique_ptr<::mlir::Pass> create{0}(const "
- "{0}Options &options);\n",
+ os << llvm::formatv("std::unique_ptr<::mlir::Pass> create{0}("
+ "{0}Options options);\n",
passName);
}
@@ -236,7 +236,7 @@ namespace impl {{
const char *const friendDefaultConstructorWithOptionsDeclTemplate = R"(
namespace impl {{
- std::unique_ptr<::mlir::Pass> create{0}(const {0}Options &options);
+ std::unique_ptr<::mlir::Pass> create{0}({0}Options options);
} // namespace impl
)";
@@ -247,8 +247,8 @@ const char *const friendDefaultConstructorDefTemplate = R"(
)";
const char *const friendDefaultConstructorWithOptionsDefTemplate = R"(
- friend std::unique_ptr<::mlir::Pass> create{0}(const {0}Options &options) {{
- return std::make_unique<DerivedT>(options);
+ friend std::unique_ptr<::mlir::Pass> create{0}({0}Options options) {{
+ return std::make_unique<DerivedT>(std::move(options));
}
)";
@@ -259,8 +259,8 @@ std::unique_ptr<::mlir::Pass> create{0}() {{
)";
const char *const defaultConstructorWithOptionsDefTemplate = R"(
-std::unique_ptr<::mlir::Pass> create{0}(const {0}Options &options) {{
- return impl::create{0}(options);
+std::unique_ptr<::mlir::Pass> create{0}({0}Options options) {{
+ return impl::create{0}(std::move(options));
}
)";
@@ -326,10 +326,10 @@ static void emitPassDefs(const Pass &pass, raw_ostream &os) {
if (ArrayRef<PassOption> options = pass.getOptions(); !options.empty()) {
os.indent(2) << llvm::formatv(
- "{0}Base(const {0}Options &options) : {0}Base() {{\n", passName);
+ "{0}Base({0}Options options) : {0}Base() {{\n", passName);
for (const PassOption &opt : pass.getOptions())
- os.indent(4) << llvm::formatv("{0} = options.{0};\n",
+ os.indent(4) << llvm::formatv("{0} = std::move(options.{0});\n",
opt.getCppVariableName());
os.indent(2) << "}\n";
diff --git a/mlir/unittests/TableGen/PassGenTest.cpp b/mlir/unittests/TableGen/PassGenTest.cpp
index 859eba7e517869..5d2e7f47624caf 100644
--- a/mlir/unittests/TableGen/PassGenTest.cpp
+++ b/mlir/unittests/TableGen/PassGenTest.cpp
@@ -72,7 +72,7 @@ TEST(PassGenTest, PassOptions) {
TestPassWithOptionsOptions options;
options.testOption = 57;
- llvm::SmallVector<int64_t, 2> testListOption = {1, 2};
+ llvm::SmallVector<int64_t> testListOption = {1, 2};
options.testListOption = testListOption;
const auto unwrap = [](const std::unique_ptr<mlir::Pass> &pass) {
|
River707
reviewed
Sep 30, 2024
| @@ -15,6 +15,7 @@ | |||
| #define MLIR_TRANSFORMS_PASSES_H | |||
|
|
|||
| #include "mlir/Pass/Pass.h" | |||
| #include "mlir/Pass/PassManager.h" | |||
There was a problem hiding this comment.
Why is PassManager.h being included now?
There was a problem hiding this comment.
The inliner pass contains a vector of OpPassManager in InlinerOptions. After this change, the size of OpPassManager needs to be known to compile a TU containing this header.
Co-authored-by: River Riddle <[email protected]>
River707
approved these changes
Sep 30, 2024
kuhar
reviewed
Sep 30, 2024
Sterling-Augustine
pushed a commit
to Sterling-Augustine/llvm-project
that referenced
this pull request
Oct 3, 2024
This change makes two (related) changes: First, it updates the tablegen option for `ListOption` to emit a `SmallVector` instead of an `ArrayRef`. This brings `ListOption` more inline with the traditional `Option`, where values are typically provided using types that have storage. After this change, all options should be fully owned by a Pass' `Options` object after it has been fully constructed, unless the underlying type of the `Option` explicitly indicates otherwise. Second, it updates the generated constructors for Passes to consume options by value instead of reference, and prefers moving options into the pass itself. This should be more efficient for non-trivial options objects, where the previous interface forced a copy to be materialized. Now, at worst case the API materializes a copy (no worse than before); at best-case, all options objects are moved into place. Ideally, we could update the Pass constructor to take an r-value reference to the Options object instead, but this approach will require numerous changes to existing passes and their factory functions. --------- Authored-by: Nikhil Kalra <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change makes two (related) changes:
First, it updates the tablegen option for
ListOptionto emit aSmallVectorinstead of anArrayRef. This bringsListOptionmore inline with the traditionalOption, where values are typically provided using types that have storage. After this change, all options should be fully owned by a Pass'Optionsobject after it has been fully constructed, unless the underlying type of theOptionexplicitly indicates otherwise.Second, it updates the generated constructors for Passes to consume options by value instead of reference, and prefers moving options into the pass itself. This should be more efficient for non-trivial options objects, where the previous interface forced a copy to be materialized. Now, at worst case the API materializes a copy (no worse than before); at best-case, all options objects are moved into place. Ideally, we could update the Pass constructor to take an r-value reference to the Options object instead, but this approach will require numerous changes to existing passes and their factory functions.