-
Notifications
You must be signed in to change notification settings - Fork 196
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
base: main
Are you sure you want to change the base?
Conversation
e8b6526
to
a8cef3f
Compare
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 <[email protected]>
a8cef3f
to
5b4059b
Compare
CUDA Quantum Docs Bot: A preview of the documentation can be found here. |
|
||
void registerQuakeDialectAndTypes(py::module &m) { | ||
void registerQuakeTypes(py::module &m) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void registerQuakeTypes(py::module &m) { | |
static void registerQuakeTypes(py::module &m) { |
@@ -143,21 +115,10 @@ void registerQuakeDialectAndTypes(py::module &m) { | |||
}); | |||
} | |||
|
|||
void registerCCDialectAndTypes(py::module &m) { | |||
void registerCCTypes(py::module &m) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
void registerCCTypes(py::module &m) { | |
static void registerCCTypes(py::module &m) { |
Assuming that's ok, should move these 2 out of the namespace as well.
registerAllDialects(registry); | ||
auto *mlirContext = unwrap(context); | ||
cudaq::registerAllDialects(registry); | ||
MLIRContext *mlirContext = unwrap(context); | ||
mlirContext->appendDialectRegistry(registry); | ||
mlirContext->loadAllAvailableDialects(); |
There was a problem hiding this comment.
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?
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 whenRegisterEverything
is built. This global registry is loaded to allContext()
objects on its__init__
method. We can populate this global registry whenevercudaq.mlir
is loaded so that everyContext()
gets all dialects that we need. This improvement will be left for later work.