Skip to content

Commit 9349484

Browse files
authored
[MLIR] Make PassPipelineOptions virtually inheriting from PassOptions to allow diamond inheritance (#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 ``` A proposed fix is to use the common solution to the diamond inheritance problem - virtual inheritance.
1 parent bd39ae6 commit 9349484

File tree

3 files changed

+70
-1
lines changed

3 files changed

+70
-1
lines changed

mlir/include/mlir/Pass/PassOptions.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -377,7 +377,7 @@ class PassOptions : protected llvm::cl::SubCommand {
377377
/// ListOption<int> someListFlag{*this, "flag-name", llvm::cl::desc("...")};
378378
/// };
379379
template <typename T>
380-
class PassPipelineOptions : public detail::PassOptions {
380+
class PassPipelineOptions : public virtual detail::PassOptions {
381381
public:
382382
/// Factory that parses the provided options and returns a unique_ptr to the
383383
/// struct.

mlir/test/Pass/pipeline-options-parsing.mlir

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
// 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
1414
// 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
1515
// 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
16+
// 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
1617

1718

1819
// This test checks that lists-of-nested-options like 'option1={...},{....}' can be parsed
@@ -106,3 +107,12 @@
106107
// CHECK_10-NEXT: test-options-pass{enum=zero string= string-list={,}}
107108
// CHECK_10-NEXT: )
108109
// CHECK_10-NEXT: )
110+
111+
// CHECK_11: builtin.module(
112+
// CHECK_11-NEXT: func.func(
113+
// CHECK_11-NEXT: test-options-pass-a
114+
// CHECK_11-NEXT: )
115+
// CHECK_11-NEXT: func.func(
116+
// CHECK_11-NEXT: test-options-pass-b
117+
// CHECK_11-NEXT: )
118+
// CHECK_11-NEXT: )

mlir/test/lib/Pass/TestPassManager.cpp

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,51 @@ struct TestOptionsSuperPass
133133
llvm::cl::desc("Example list of PassPipelineOptions option")};
134134
};
135135

136+
struct TestOptionsPassA
137+
: public PassWrapper<TestOptionsPassA, OperationPass<func::FuncOp>> {
138+
MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestOptionsPassA)
139+
140+
struct Options : public PassPipelineOptions<Options> {
141+
Option<bool> foo{*this, "foo", llvm::cl::desc("Example boolean option")};
142+
};
143+
144+
TestOptionsPassA() = default;
145+
TestOptionsPassA(const TestOptionsPassA &) : PassWrapper() {}
146+
TestOptionsPassA(const Options &options) { this->options.foo = options.foo; }
147+
148+
void runOnOperation() final {}
149+
StringRef getArgument() const final { return "test-options-pass-a"; }
150+
StringRef getDescription() const final {
151+
return "Test superset options parsing capabilities - subset A";
152+
}
153+
154+
Options options;
155+
};
156+
157+
struct TestOptionsPassB
158+
: public PassWrapper<TestOptionsPassB, OperationPass<func::FuncOp>> {
159+
MLIR_DEFINE_EXPLICIT_INTERNAL_INLINE_TYPE_ID(TestOptionsPassB)
160+
161+
struct Options : public PassPipelineOptions<Options> {
162+
Option<bool> bar{*this, "bar", llvm::cl::desc("Example boolean option")};
163+
};
164+
165+
TestOptionsPassB() = default;
166+
TestOptionsPassB(const TestOptionsPassB &) : PassWrapper() {}
167+
TestOptionsPassB(const Options &options) { this->options.bar = options.bar; }
168+
169+
void runOnOperation() final {}
170+
StringRef getArgument() const final { return "test-options-pass-b"; }
171+
StringRef getDescription() const final {
172+
return "Test superset options parsing capabilities - subset B";
173+
}
174+
175+
Options options;
176+
};
177+
178+
struct TestPipelineOptionsSuperSetAB : TestOptionsPassA::Options,
179+
TestOptionsPassB::Options {};
180+
136181
/// A test pass that always aborts to enable testing the crash recovery
137182
/// mechanism of the pass manager.
138183
struct TestCrashRecoveryPass
@@ -270,6 +315,9 @@ void registerPassManagerTestPass() {
270315
PassRegistration<TestOptionsPass>();
271316
PassRegistration<TestOptionsSuperPass>();
272317

318+
PassRegistration<TestOptionsPassA>();
319+
PassRegistration<TestOptionsPassB>();
320+
273321
PassRegistration<TestModulePass>();
274322

275323
PassRegistration<TestFunctionPass>();
@@ -306,5 +354,16 @@ void registerPassManagerTestPass() {
306354
[](OpPassManager &pm, const TestOptionsSuperPass::Options &options) {
307355
pm.addPass(std::make_unique<TestOptionsSuperPass>(options));
308356
});
357+
358+
PassPipelineRegistration<TestPipelineOptionsSuperSetAB>
359+
registerPipelineOptionsSuperSetABPipeline(
360+
"test-options-super-set-ab-pipeline",
361+
"Parses options of PassPipelineOptions using pass pipeline "
362+
"registration",
363+
[](OpPassManager &pm, const TestPipelineOptionsSuperSetAB &options) {
364+
// Pass superset AB options to subset options A and B
365+
pm.addPass(std::make_unique<TestOptionsPassA>(options));
366+
pm.addPass(std::make_unique<TestOptionsPassB>(options));
367+
});
309368
}
310369
} // namespace mlir

0 commit comments

Comments
 (0)