[Mlir-commits] [mlir] [mlir] Pass Options ownership modifications (PR #110582)

[Nikhil Kalra]

https://github.com/nikalra created https://github.com/llvm/llvm-project/pull/110582

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.

>From a892aecd597af064bf4eb11777627dfbb0fba50a Mon Sep 17 00:00:00 2001
From: Nikhil Kalra <nkalra at apple.com>
Date: Mon, 30 Sep 2024 14:57:47 -0700
Subject: [PATCH 1/2] use smallvector

---
 mlir/include/mlir/Transforms/Passes.h              | 1 +
 mlir/lib/Dialect/Arith/Transforms/IntNarrowing.cpp | 6 ++++--
 mlir/tools/mlir-tblgen/PassGen.cpp                 | 2 +-
 mlir/unittests/TableGen/PassGenTest.cpp            | 2 +-
 4 files changed, 7 insertions(+), 4 deletions(-)

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..38eaefea5166f9 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());
 
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) {

>From 6d25612bcaa03e8210bbbe89afb3cecbf9915015 Mon Sep 17 00:00:00 2001
From: Nikhil Kalra <nkalra at apple.com>
Date: Mon, 30 Sep 2024 14:58:06 -0700
Subject: [PATCH 2/2] prefer move over copy

---
 mlir/tools/mlir-tblgen/PassGen.cpp | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/mlir/tools/mlir-tblgen/PassGen.cpp b/mlir/tools/mlir-tblgen/PassGen.cpp
index 38eaefea5166f9..655843f26201ac 100644
--- a/mlir/tools/mlir-tblgen/PassGen.cpp
+++ b/mlir/tools/mlir-tblgen/PassGen.cpp
@@ -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";