[Mlir-commits] [mlir] Enable superset options (PR #146370)
Sasa Vuckovic
llvmlistbot at llvm.org
Mon Jun 30 08:33:39 PDT 2025
https://github.com/svuckovicTT created https://github.com/llvm/llvm-project/pull/146370
## Problem
Given 3 pipelines, A, B, and a superset pipeline AB that runs both the A & B pipelines, it is not easy to manage their options - one needs to manually recreate all options from A and B into AB, and maintain them. This is tedious.
## Proposed solution
Ideally, AB options class inherits from both A and B options, making the maintenance effortless. Today though, this causes problems as their base classes `PassPipelineOptions<A>` and `PassPipelineOptions<B>` both inherit from `mlir::detail::PassOptions`, leading to the so called "diamond inheritance problem", i.e. multiple definitions of the same symbol, in this case parseFromString that is defined in mlir::detail::PassOptions.
Visually, the inheritance looks like this:
```
mlir::detail::PassOptions
↑ ↑
| |
PassPipelineOptions<A> PassPipelineOptions<B>
↑ ↑
| |
AOptions BOptions
↑ ↑
+---------+--------+
|
ABOptions
```
The exact error is:
```C++
include/mlir/Pass/PassRegistry.h:186:30: error: non-static member 'parseFromString' found in multiple base-class subobjects of type 'detail::PassOptions':
struct ABPipelineOptions -> APipelineOptions -> PassPipelineOptions<A> -> detail::PassOptions
struct ABPipelineOptions -> BPipelineOptions -> PassPipelineOptions<B> -> detail::PassOptions
```
A proposed fix is to use the common solution to the diamond inheritance problem - virtual inheritance:
```diff
-class PassPipelineOptions : public detail::PassOptions {
+class PassPipelineOptions : public virtual detail::PassOptions {
```
>From de2e476fe7d6d5771b019aec16e448173cf52d88 Mon Sep 17 00:00:00 2001
From: svuckovic <svuckovic at tenstorrent.com>
Date: Mon, 30 Jun 2025 12:22:06 +0000
Subject: [PATCH] Enable superset options
---
mlir/include/mlir/Pass/PassOptions.h | 2 +-
mlir/test/Pass/pipeline-options-parsing.mlir | 10 ++++
mlir/test/lib/Pass/TestPassManager.cpp | 59 ++++++++++++++++++++
3 files changed, 70 insertions(+), 1 deletion(-)
diff --git a/mlir/include/mlir/Pass/PassOptions.h b/mlir/include/mlir/Pass/PassOptions.h
index e1f16c6158ad5..0c71f78b52d3d 100644
--- a/mlir/include/mlir/Pass/PassOptions.h
+++ b/mlir/include/mlir/Pass/PassOptions.h
@@ -377,7 +377,7 @@ class PassOptions : protected llvm::cl::SubCommand {
/// ListOption<int> someListFlag{*this, "flag-name", llvm::cl::desc("...")};
/// };
template <typename T>
-class PassPipelineOptions : public detail::PassOptions {
+class PassPipelineOptions : public virtual detail::PassOptions {
public:
/// Factory that parses the provided options and returns a unique_ptr to the
/// struct.
diff --git a/mlir/test/Pass/pipeline-options-parsing.mlir b/mlir/test/Pass/pipeline-options-parsing.mlir
index 9385d353faf95..03ac38ea16112 100644
--- a/mlir/test/Pass/pipeline-options-parsing.mlir
+++ b/mlir/test/Pass/pipeline-options-parsing.mlir
@@ -13,6 +13,7 @@
// RUN: mlir-opt %s -verify-each=false -pass-pipeline='builtin.module(builtin.module(func.func(test-options-pass{list=3}), func.func(test-options-pass{enum=one list=1,2,3,4 string=foo"bar"baz})))' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_6 %s
// 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
+// RUN: mlir-opt %s -verify-each=false -test-options-super-set-ab-pipeline='foo=true bar=false' -dump-pass-pipeline 2>&1 | FileCheck --check-prefix=CHECK_11 %s
// This test checks that lists-of-nested-options like 'option1={...},{....}' can be parsed
@@ -106,3 +107,12 @@
// CHECK_10-NEXT: test-options-pass{enum=zero string= string-list={,}}
// CHECK_10-NEXT: )
// CHECK_10-NEXT: )
+
+// CHECK_11: builtin.module(
+// CHECK_11-NEXT: func.func(
+// CHECK_11-NEXT: test-options-pass-a
+// CHECK_11-NEXT: )
+// CHECK_11-NEXT: func.func(
+// CHECK_11-NEXT: test-options-pass-b
+// CHECK_11-NEXT: )
+// CHECK_11-NEXT: )
diff --git a/mlir/test/lib/Pass/TestPassManager.cpp b/mlir/test/lib/Pass/TestPassManager.cpp
index 7afe2109f04db..8ef83f77834fe 100644
--- a/mlir/test/lib/Pass/TestPassManager.cpp
+++ b/mlir/test/lib/Pass/TestPassManager.cpp
@@ -133,6 +133,51 @@ struct TestOptionsSuperPass
llvm::cl::desc("Example list of PassPipelineOptions option")};
};
+struct TestOptionsPassA
+ : public PassWrapper<TestOptionsPassA, OperationPass<func::FuncOp>> {
+ MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestOptionsPass)
+
+ struct Options : public PassPipelineOptions<Options> {
+ Option<bool> foo{*this, "foo", llvm::cl::desc("Example boolean option")};
+ };
+
+ TestOptionsPassA() = default;
+ TestOptionsPassA(const TestOptionsPassA &) : PassWrapper() {}
+ TestOptionsPassA(const Options &options) { this->options.foo = options.foo; }
+
+ void runOnOperation() final {}
+ StringRef getArgument() const final { return "test-options-pass-a"; }
+ StringRef getDescription() const final {
+ return "Test superset options parsing capabilities - subset A";
+ }
+
+ Options options;
+};
+
+struct TestOptionsPassB
+ : public PassWrapper<TestOptionsPassB, OperationPass<func::FuncOp>> {
+ MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestOptionsPass)
+
+ struct Options : public PassPipelineOptions<Options> {
+ Option<bool> bar{*this, "bar", llvm::cl::desc("Example boolean option")};
+ };
+
+ TestOptionsPassB() = default;
+ TestOptionsPassB(const TestOptionsPassB &) : PassWrapper() {}
+ TestOptionsPassB(const Options &options) { this->options.bar = options.bar; }
+
+ void runOnOperation() final {}
+ StringRef getArgument() const final { return "test-options-pass-b"; }
+ StringRef getDescription() const final {
+ return "Test superset options parsing capabilities - subset B";
+ }
+
+ Options options;
+};
+
+struct TestPipelineOptionsSuperSetAB : TestOptionsPassA::Options,
+ TestOptionsPassB::Options {};
+
/// A test pass that always aborts to enable testing the crash recovery
/// mechanism of the pass manager.
struct TestCrashRecoveryPass
@@ -270,6 +315,9 @@ void registerPassManagerTestPass() {
PassRegistration<TestOptionsPass>();
PassRegistration<TestOptionsSuperPass>();
+ PassRegistration<TestOptionsPassA>();
+ PassRegistration<TestOptionsPassB>();
+
PassRegistration<TestModulePass>();
PassRegistration<TestFunctionPass>();
@@ -306,5 +354,16 @@ void registerPassManagerTestPass() {
[](OpPassManager &pm, const TestOptionsSuperPass::Options &options) {
pm.addPass(std::make_unique<TestOptionsSuperPass>(options));
});
+
+ PassPipelineRegistration<TestPipelineOptionsSuperSetAB>
+ registerPipelineOptionsSuperSetABPipeline(
+ "test-options-super-set-ab-pipeline",
+ "Parses options of PassPipelineOptions using pass pipeline "
+ "registration",
+ [](OpPassManager &pm, const TestPipelineOptionsSuperSetAB &options) {
+ // Pass superset AB options to subset options A and B
+ pm.addPass(std::make_unique<TestOptionsPassA>(options));
+ pm.addPass(std::make_unique<TestOptionsPassB>(options));
+ });
}
} // namespace mlir
More information about the Mlir-commits
mailing list