Skip to content

MLIR Conversion(s) #880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 142 commits into
base: main
Choose a base branch
from
Draft

MLIR Conversion(s) #880

wants to merge 142 commits into from

Conversation

flowerthrower
Copy link
Collaborator

Description

This PR originates from this mqt-mlir PR and introduces conversions between Catalysts quantum dialect and the MQTOpt dialect.

Checklist:

  • The pull request only contains commits that are related to it.
  • I have added appropriate tests and documentation.
  • I have made sure that all CI jobs on GitHub pass.
  • The pull request introduces no new warnings and follows the project's style guidelines.

@github-project-automation github-project-automation bot moved this to In Progress in MQT Core Mar 19, 2025
@flowerthrower flowerthrower self-assigned this Mar 19, 2025
@flowerthrower flowerthrower added MLIR Anything related to MLIR feature New feature or request labels Mar 19, 2025
@flowerthrower flowerthrower added this to the MLIR Support milestone Mar 19, 2025
Copy link

codecov bot commented Mar 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

📢 Thoughts on this report? Let us know!

@burgholzer
Copy link
Member

Now that #879 is merged, this needs to be rebased

@@ -26,6 +25,13 @@ string(REPLACE "." ";" MLIR_VERSION_COMPONENTS ${MLIR_VERSION})
list(GET MLIR_VERSION_COMPONENTS 0 MLIR_VERSION_MAJOR)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just because I stumbe over it right now: Is this gymnastic with the version string necessary or can the VERSION_LESS operator of cmake be used directly? That might make the code cleaner here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable to use VERSION_LESS 👍🏼

Copy link
Member

@burgholzer burgholzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of comments because the PR notifications where in my inbox 😌

Comment on lines +78 to +79
- name: Set macOS deployment target for linker
run: echo "MACOSX_DEPLOYMENT_TARGET=14.0" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be necessary as you are running on macOS 14 already.

Comment on lines +78 to +79
- name: Set macOS deployment target for linker
run: echo "MACOSX_DEPLOYMENT_TARGET=14.0" >> $GITHUB_ENV
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be necessary as you are running on macOS 14 already.

Comment on lines +22 to +25
runs-on: macos-14
strategy:
matrix:
llvm-version: [19]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be interesting to see this work on macos-13 (x86 runners) as well as llvm-version: 20.
The current build pipeline seems to pass, which is great already!

Unrelated to this PR in particular, but what could be a nice byproduct in a separate PR is that this already represents a functional macOS-14 CI pipeline for MLIR, which would be part of #925.

Comment on lines +78 to +88
# Manually detect the installed Catalyst Python and get its cmake directory.
execute_process(
COMMAND "${Python_EXECUTABLE}" -c
"import catalyst, os; print(os.path.dirname(catalyst.__file__))"
OUTPUT_STRIP_TRAILING_WHITESPACE
OUTPUT_VARIABLE Python_Catalyst_DIR
ERROR_QUIET)
message(STATUS "Found Catalyst package: ${Python_Catalyst_DIR}")

# TODO: once the Catalyst Python package provides the necessary files set(Catalyst_DIR
# "${Python_Catalyst_DIR}/mlir/build/lib/cmake/catalyst")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, there would be no need to do something like set(Catalyst_DIR "${Python_Catalyst_DIR}/..., but Catalyst_DIR would already be a result of the execute_process call.
Either catalyst provides an entry point for that (similar to pybind, nanobind, or mqt-core), or we parse this somewhat from output similar to the existing command here by a simple Python script.

@ystade ystade marked this pull request as draft May 9, 2025 09:29
@ystade
Copy link
Collaborator

ystade commented May 9, 2025

We decided that we first merge the plugin (#881) and afterward integrate those changes here and then merge this PR.

This has the following advantages:

  • Right now, to test the conversion passes of this PR, quantum-opt must be linked with a target from the Catalyst project, in particular the MLIRQuantum target containing the dialect. This is necessary even tough apart from testing there is no real use case for processing catalyst mlir without using the frontend Pennylane.
  • Vice versa, the plugin does not need to be linked with any target of the Catalyst project, it only requires the header files that (hopefully) will be shipped with the pip wheel of catalyst sonn. Then this plugin can (and must) be invoked from pennylane and this way the conversion passes can be tested without requireing any target of catalyst. Only an easy install of catalyst in the python environment is necessary.

@ystade ystade mentioned this pull request May 9, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request MLIR Anything related to MLIR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants