[MLIR] Make `PassPipelineOptions` virtually inheriting from PassOptions to allow diamond inheritance #146370

svuckovicTT · 2025-06-30T15:33:32Z

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 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.

github-actions · 2025-06-30T15:33:55Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-06-30T15:34:27Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: Sasa Vuckovic (svuckovicTT)

Changes

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 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&lt;A&gt;      PassPipelineOptions&lt;B&gt;
                            ↑                  ↑
                            |                  |
                         AOptions           BOptions
                            ↑                  ↑
                            +---------+--------+
                                      |
                                  ABOptions

The exact error is:

include/mlir/Pass/PassRegistry.h:186:30: error: non-static member 'parseFromString' found in multiple base-class subobjects of type 'detail::PassOptions':
    struct ABPipelineOptions -&gt; APipelineOptions -&gt; PassPipelineOptions&lt;A&gt; -&gt; detail::PassOptions
    struct ABPipelineOptions -&gt; BPipelineOptions -&gt; PassPipelineOptions&lt;B&gt; -&gt; detail::PassOptions

A proposed fix is to use the common solution to the diamond inheritance problem - virtual inheritance:

-class PassPipelineOptions : public detail::PassOptions {
+class PassPipelineOptions : public virtual detail::PassOptions {

Full diff: https://github.com/llvm/llvm-project/pull/146370.diff

3 Files Affected:

(modified) mlir/include/mlir/Pass/PassOptions.h (+1-1)
(modified) mlir/test/Pass/pipeline-options-parsing.mlir (+10)
(modified) mlir/test/lib/Pass/TestPassManager.cpp (+59)

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

joker-eph · 2025-06-30T16:12:04Z

The main issue with a C++ inheritance approach is that we can't manage collision on the options names. What is the symptom if someone tries to combine two pipelines which have conflicting option names?

emosy · 2025-06-30T17:16:24Z

I would like this, as it could simplify composing a larger pipeline of smaller pipelines without having to rewrite the same options many times.

The main issue with a C++ inheritance approach is that we can't manage collision on the options names. What is the symptom if someone tries to combine two pipelines which have conflicting option names?

I agree, this could be an issue if there are shared generic-sounding options specified across multiple pipelines.

svuckovicTT · 2025-06-30T18:38:08Z

Thanks for the comments!

I tested the scenario with identical option names, it fails during runtime with:

mlir-opt: CommandLine Error: Option 'foo' registered more than once!

On one hand, it's not the optimal setup as it errors out in runtime vs compile-time. On the other hand, it errors out in runtime vs causing a silent override, and it's a very deliberate move to subclass multiple Option classes.

Lmk what you think!

Copilot

Pull Request Overview

This PR enables superset pipeline options by introducing virtual inheritance for PassPipelineOptions and adds corresponding test passes and MLIR tests to verify parsing of combined options.

Convert PassPipelineOptions to use virtual inheritance to resolve diamond inheritance.
Add TestOptionsPassA, TestOptionsPassB, and a combined TestPipelineOptionsSuperSetAB along with registration.
Extend pipeline-options-parsing.mlir with a new RUN line to test the superset pipeline options.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
mlir/include/mlir/Pass/PassOptions.h	Changed `PassPipelineOptions` to inherit virtually
mlir/test/lib/Pass/TestPassManager.cpp	Added test passes A/B, superset options, and registrations
mlir/test/Pass/pipeline-options-parsing.mlir	Added RUN/CHECK lines for superset pipeline test

mlir/test/lib/Pass/TestPassManager.cpp

svuckovicTT · 2025-07-04T15:43:51Z

Hey @River707, just a friendly ping - lmk if you've had a chance to take a look 😊

svuckovicTT · 2025-07-17T20:09:13Z

Ping

svuckovicTT · 2025-07-23T09:29:51Z

Ping

svuckovicTT · 2025-07-31T10:52:52Z

Ping

svuckovicTT · 2025-08-07T14:08:18Z

Ping

joker-eph · 2025-08-07T14:28:00Z

I don't think that's an ideal solution, but the one-line change does not seem harmful either, so LGTM.
Let me ping River though!

River707 · 2025-08-08T10:28:46Z

I agree that this doesn't feel like the right solution, but I don't think we ever fully baked out pipeline options anyways. If this solves the problem for now, I don't think it's a huge burden to change the single line.

github-actions · 2025-08-08T10:34:12Z

@svuckovicTT Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

…ns to allow diamond inheritance (llvm#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` 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 ↑ ↑ | | AOptions BOptions ↑ ↑ +---------+--------+ | ABOptions ``` A proposed fix is to use the common solution to the diamond inheritance problem - virtual inheritance.

…ns 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` 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 ↑ ↑ | | AOptions BOptions ↑ ↑ +---------+--------+ | ABOptions ``` A proposed fix is to use the common solution to the diamond inheritance problem - virtual inheritance.

Enable superset options

de2e476

llvmbot added mlir:core MLIR Core Infrastructure mlir labels Jun 30, 2025

joker-eph requested review from River707 and Copilot June 30, 2025 19:38

Copilot AI reviewed Jun 30, 2025

View reviewed changes

mlir/test/lib/Pass/TestPassManager.cpp Outdated Show resolved Hide resolved

mlir/test/lib/Pass/TestPassManager.cpp Outdated Show resolved Hide resolved

Fix TypeIDs of test options passes A and B

e4812b2

svuckovicTT mentioned this pull request Jul 23, 2025

LLVM Patch: Enable PassPipelineOptions superset tenstorrent/tt-mlir#4166

Closed

1 task

joker-eph changed the title ~~Enable superset options~~ [MLIR] Make PassPipelineOptions virtual to allow diamond inheritance Aug 7, 2025

joker-eph changed the title ~~[MLIR] Make PassPipelineOptions virtual to allow diamond inheritance~~ [MLIR] Make PassPipelineOptions virtually inheriting from PassOptions to allow diamond inheritance Aug 7, 2025

River707 approved these changes Aug 8, 2025

View reviewed changes

joker-eph merged commit 9349484 into llvm:main Aug 8, 2025
7 checks passed

svuckovicTT mentioned this pull request Sep 10, 2025

[MLIR] Improve mlir options construction tenstorrent/tt-mlir#3860

Closed

dwblaikie mentioned this pull request Sep 16, 2025

port 5b4819e to release #159209

Merged

jhuber6 mentioned this pull request Sep 17, 2025

[LLVM] Update CUDA ELF flags for their new ABI #159451

Merged

azecevicTT mentioned this pull request Sep 21, 2025

tt-mlir does not compile tenstorrent/tt-mlir#5035

Open

[MLIR] Make PassPipelineOptions virtually inheriting from PassOptions to allow diamond inheritance #146370

[MLIR] Make PassPipelineOptions virtually inheriting from PassOptions to allow diamond inheritance #146370

Uh oh!

Conversation

svuckovicTT commented Jun 30, 2025 • edited by joker-eph Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Proposed solution

Uh oh!

github-actions bot commented Jun 30, 2025

Uh oh!

llvmbot commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Proposed solution

Uh oh!

joker-eph commented Jun 30, 2025

Uh oh!

emosy commented Jun 30, 2025

Uh oh!

svuckovicTT commented Jun 30, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

svuckovicTT commented Jul 4, 2025

Uh oh!

svuckovicTT commented Jul 17, 2025

Uh oh!

svuckovicTT commented Jul 23, 2025

Uh oh!

svuckovicTT commented Jul 31, 2025

Uh oh!

svuckovicTT commented Aug 7, 2025

Uh oh!

joker-eph commented Aug 7, 2025

Uh oh!

River707 commented Aug 8, 2025

Uh oh!

Uh oh!

github-actions bot commented Aug 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[MLIR] Make `PassPipelineOptions` virtually inheriting from PassOptions to allow diamond inheritance #146370

[MLIR] Make `PassPipelineOptions` virtually inheriting from PassOptions to allow diamond inheritance #146370

svuckovicTT commented Jun 30, 2025 •

edited by joker-eph

Loading

llvmbot commented Jun 30, 2025 •

edited

Loading