Skip to content
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

[python] Remove unused MLIR components #2443

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

boschmitt
Copy link
Collaborator

@boschmitt boschmitt commented Nov 29, 2024

Description

We don't need to take everything from MLIR for our python bindings. This change cherry picks the upstream components our compiler depends on.

The commit also cleans up some unnecessary code that ends up registering dialects more than once, and surfaces the register_all_dialects function to a less obscure location.

EDIT: The process of registering dialects can be further improved and made completely automatic. MLIR provides a global _dialect_registry that gets automatically populated with all upstream dialects when RegisterEverything is built. This global registry is loaded to all Context() objects on its __init__ method. We can populate this global registry whenever cudaq.mlir is loaded so that every Context() gets all dialects that we need. This improvement will be left for later work.

@boschmitt boschmitt force-pushed the shrink_nvqpp_python branch 5 times, most recently from a8cef3f to 5b4059b Compare December 2, 2024 13:06
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Dec 11, 2024
python/runtime/mlir/py_register_dialects.cpp Outdated Show resolved Hide resolved
python/runtime/mlir/py_register_dialects.cpp Outdated Show resolved Hide resolved
registerAllDialects(registry);
auto *mlirContext = unwrap(context);
cudaq::registerAllDialects(registry);
MLIRContext *mlirContext = unwrap(context);
mlirContext->appendDialectRegistry(registry);
mlirContext->loadAllAvailableDialects();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does loadAllAvailableDialects actually load Quake, CC, and CodeGen?

Do we need an explicit test to verify that all the dialects we expect to be loaded are loaded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

loadAllAvailableDialects() will load all dialects registered in the context. So, Quake and CC will be loaded. The CodeGen dialect is not registered in cudaq::registerAllDialects. I'm not sure why, but its being registered in other places and in different ways:

registry.insert<cudaq::codegen::CodeGenDialect>();

cudaq::opt::registerCodeGenDialect(registry);

I can make it uniform in a follow-up PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The codegen dialect was intended to be sort of second-class in that it would only be applicable at codegen time. The point being we don't want folks to start using it in ad hoc ways in random passes, etc. Adding a few landmines was sort of the idea.

OTOH, we could clean it up a bit and remove some of those hurdles. We still don't want that dialect used "in the wild" though...

@boschmitt boschmitt force-pushed the shrink_nvqpp_python branch 2 times, most recently from f6a8985 to a30eddd Compare January 29, 2025 16:22
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Jan 29, 2025
@boschmitt boschmitt force-pushed the shrink_nvqpp_python branch from a30eddd to e04b6f8 Compare January 29, 2025 18:41
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Jan 29, 2025
@boschmitt boschmitt force-pushed the shrink_nvqpp_python branch from e04b6f8 to 2010d99 Compare January 31, 2025 13:26
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Jan 31, 2025
@boschmitt boschmitt force-pushed the shrink_nvqpp_python branch 2 times, most recently from 992ba52 to e62b6cf Compare January 31, 2025 19:11
Copy link

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Jan 31, 2025
@boschmitt boschmitt force-pushed the shrink_nvqpp_python branch 2 times, most recently from 754fffd to c30f508 Compare February 3, 2025 16:39
Copy link

github-actions bot commented Feb 3, 2025

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Feb 3, 2025
@boschmitt boschmitt force-pushed the shrink_nvqpp_python branch from c30f508 to 99ad7a6 Compare February 3, 2025 17:28
Copy link

github-actions bot commented Feb 3, 2025

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Feb 3, 2025
Copy link

github-actions bot commented Feb 3, 2025

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Feb 3, 2025
@boschmitt boschmitt force-pushed the shrink_nvqpp_python branch from 99ad7a6 to a156bdc Compare February 3, 2025 20:27
Copy link

github-actions bot commented Feb 3, 2025

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

github-actions bot pushed a commit that referenced this pull request Feb 3, 2025
We don't need to take everything from MLIR for our python bindings. This
change cherry picks the upstream components our compiler depends on.

The commit also cleans up some unnecessary code that ends up registering
dialects more than once, and surfaces the `register_all_dialects`
function to a less obscure location.

Signed-off-by: boschmitt <7152025+boschmitt@users.noreply.github.com>
@boschmitt boschmitt force-pushed the shrink_nvqpp_python branch from a156bdc to 479db23 Compare February 4, 2025 18:44
github-actions bot pushed a commit that referenced this pull request Feb 4, 2025
Copy link

github-actions bot commented Feb 4, 2025

CUDA Quantum Docs Bot: A preview of the documentation can be found here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants