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

[Binding] enable mlir python binding for graph compiler #91

Merged
merged 17 commits into from
May 27, 2024

Conversation

xurui1995
Copy link
Contributor

@xurui1995 xurui1995 commented May 20, 2024

Enable basic mlir python binding for onednngraph dialect and passes in gc/Transforms, will add support for more dialects and related passes in the next PR.
Tracking #57

@xurui1995
Copy link
Contributor Author

@WangJialei-A you can develop bench gc based on bindings of this PR

# TODO: Add an upstream cmake param for this vs having a global here.
add_compile_definitions("MLIR_PYTHON_PACKAGE_PREFIX=gc_mlir.")


Choose a reason for hiding this comment

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

can we add a cmake option to turn off the pybind even if LLVM has python enabled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can we add a cmake option to turn off the pybind even if LLVM has python enabled?

yes, I can add a new cmake option for the gc pybind, should we set the default value to OFF?

Choose a reason for hiding this comment

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

@ZhennanQin Do we need pybind by default? Or should we only target onednn graph api for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest to enable pybind by default as this is the way for release. For develop purposes, you can turn it off with extra cmake flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -0,0 +1,43 @@

/*
* Copyright (C) 2024 Intel Corporation

Choose a reason for hiding this comment

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

Please use LLVM license style here. Reference: #85

Copy link
Contributor

Choose a reason for hiding this comment

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

#85 is merged. Please rebase this PR and have a license check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

license fixed

CMakeLists.txt Outdated
@@ -33,6 +51,16 @@ include(AddLLVM)
include(AddMLIR)
include(HandleLLVMOptions)

if(GC_ENABLE_BINDINGS_PYTHON AND NOT MLIR_ENABLE_BINDINGS_PYTHON)
message(STATUS "Enabling Python API failed due to the 'MLIR_ENABLE_BINDINGS_PYTHON' for LLVM is not ON.")

Choose a reason for hiding this comment

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

Wording: Failed to enable Python API ...

Choose a reason for hiding this comment

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

Would you please also update the readme of the for the cmake command. The main branch has updated the readme now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you please also update the readme of the for the cmake command. The main branch has updated the readme now.

have updated the readme of main project, and I will update a more specific readme in the python dir after binding more dialects in the future

@xurui1995
Copy link
Contributor Author

@WangJialei-A @Menooker could you please help approve this PR if it looks good to you, thanks!

@WangJialei-A
Copy link
Contributor

Hi, @xurui1995
Please place an empty __init__.py file under folder gc_mlir, gc_mlir/dialects in order to import it as a normal module.

#===------------------------------------------------------------------------===#

from ._onednn_graph_ops_gen import *
from .._mlir_libs._gc_mlir.onednn_graph import *
Copy link
Contributor

Choose a reason for hiding this comment

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

@xurui1995
There is no _gc_mlir folder generated in the build. Vscode failed to lint this package in my environment.

@xurui1995 xurui1995 merged commit f40e433 into main May 27, 2024
4 checks passed
@xurui1995 xurui1995 deleted the xur/enable_binding branch May 27, 2024 05:23
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.

4 participants