-
Notifications
You must be signed in to change notification settings - Fork 72
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
Generating build files with cmake #21
Conversation
Using git submodules.
Adding the LLVM module so that all `lit` related functions are available. A new target is made available for running tests: `check-mlir-tutorial`.
So that `tutorial-opt` can be built. This introduces as well a new target `tutorial-opt`.
This introduces a new mlir lib: `AffineFullUnroll` and a new build target to build it: `MLIRAffineFullUnrollPasses`.
to have access to all tablgen related cmake statements.
This introduces a new mlir lib: `MulToAdd` and a new build target to build it: `MLIRMulToAddPasses`.
So that on the cmake side of things we can use the `add_mlir_dialect` macro without too many changes to the include directives. Ref: https://github.com/llvm/llvm-project/blob/main/mlir/cmake/modules/AddMLIR.cmake#L166-L177
This is an attempt to build the tutorial generating build files with CMake. I tried to make minimal changes to the source code itself, but given some CMake helper functions, notably add_mlir_dialect, renaming I have added as well a github action to test building using CMake (ninja). Let me know what you think about this PR. I could break things up. If people want to have access to how to build things up from scratch there is the entire commit history at mlir-tutorial-cmake |
Thank you for this! I will have to spend some time studying it to make sure I understand it. |
In re |
Sure, take your time to go through the changes. Let me know if anything is not clear.
Indeed ... one can definitely use directly the |
Instead of using `add_mlir_dialect` helper function for dialect generation, we inline it here so that we can have control over the naming of generated `*.inc` files. This avoids renaming of `PolyOps.td` so that cmake builds work well on the existing source code.
addressed this in c44cb50 |
Very useful, thanks! Is this ready to be used or are there still things missing? |
This is ready to be used yes ... the only thing to keep in mind is that if #20 gets merged I might need to update this |
I promise, I am going to merge this soon :) I've just been busy traveling for work. Currently at the LLVM developers meeting, where everyone is telling me how much they enjoy these tutorials. |
+1 for that, this series of tutorials is very instructive and useful. |
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.
It looks like the CI job hit an error at the end: https://github.com/j2kun/mlir-tutorial/pull/21/checks under "Post run actions." Any idea what that's about?
Post job cleanup.
/usr/bin/git version
git version [2](https://github.com/j2kun/mlir-tutorial/actions/runs/6351179046/job/17251997272#step:14:2).[4](https://github.com/j2kun/mlir-tutorial/actions/runs/6351179046/job/17251997272#step:14:4)2.0
Temporarily overriding HOME='/home/runner/work/_temp/df828214-0ad7-4d0b-b9[6](https://github.com/j2kun/mlir-tutorial/actions/runs/6351179046/job/17251997272#step:14:6)f-aaf1[7](https://github.com/j2kun/mlir-tutorial/actions/runs/6351179046/job/17251997272#step:14:7)a9c[8](https://github.com/j2kun/mlir-tutorial/actions/runs/6351179046/job/17251997272#step:14:8)6b6' before making global git config changes
Adding repository directory to the temporary git global config as a safe directory
/usr/bin/git config --global --add safe.directory /home/runner/work/mlir-tutorial/mlir-tutorial
/usr/bin/git config --local --name-only --get-regexp core\.sshCommand
/usr/bin/git submodule foreach --recursive sh -c "git config --local --name-only --get-regexp 'core\.sshCommand' && git config --local --unset-all 'core.sshCommand' || :"
fatal: not a git repository: externals/llvm-project/../../.git/modules/externals/llvm-project
Warning: The process '/usr/bin/git' failed with exit code 128
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.
It would be worthwhile to have the git submodule sync with the commit in bazel/import_llvm.bzl
, but it doesn't appear to be possible to have git do that in this config file. It only supports a branch. Maybe after merging this I will add a note to the readme for how to do this, with something like
cd externals/llvm-project
git checkout -b upstream $(grep LLVM_COMMIT ../../bazel/import_llvm.bzl | grep -o '".*"' | sed 's/"//g')
cd ../..
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.
it would be indeed nice to be able to keep in sync the llvm commit for both build systems, I guess a note as you propose would be enough as i dont think this will change often.
As a side note, I remember using a llvm checkout from september when i started to do the Cmake work and there were already some build errors due to some changes in MLIR.
- name: Build and test mlir-tutorial | ||
run: | | ||
mkdir build && cd build | ||
cmake -DLLVM_DIR=$PWD/../externals/llvm-project/build/lib/cmake/llvm -DMLIR_DIR=$PWD/../externals/llvm-project/build/lib/cmake/mlir .. |
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.
nit: might be better to use $github_workspace/externals/...
instead of the relative version
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.
indeed, will change that.
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.
addressed in #24
LGTM! Will give you a day or two to respond to the above, then merge either way, and I can clean up the nits. |
Eh, I'll just merge it. Thanks!!! |
Not sure what this errors means but will have a look at that and report back. |
thanks for having a look and merging |
addressed in #24 |
No description provided.