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

Generating build files with cmake #21

Merged
merged 15 commits into from
Oct 16, 2023
Merged

Conversation

vguerra
Copy link
Contributor

@vguerra vguerra commented Sep 27, 2023

No description provided.

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
@vguerra
Copy link
Contributor Author

vguerra commented Sep 27, 2023

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 PolyOps.td to Poly.td would be less intrusive.

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

@j2kun
Copy link
Owner

j2kun commented Sep 29, 2023

Thank you for this! I will have to spend some time studying it to make sure I understand it.

@j2kun
Copy link
Owner

j2kun commented Sep 29, 2023

In re add_mlir_dialect, that is such a weird upstream convention. I wonder if they would consider accepting a patch to make it more flexible... but otherwise, it doesn't seem that bad to just inline that particular function to customize the names.

@vguerra
Copy link
Contributor Author

vguerra commented Sep 29, 2023

Thank you for this! I will have to spend some time studying it to make sure I understand it.

Sure, take your time to go through the changes. Let me know if anything is not clear.

In re add_mlir_dialect, that is such a weird upstream convention. I wonder if they would consider accepting a patch to make it more flexible... but otherwise, it doesn't seem that bad to just inline that particular function to customize the names.

Indeed ... one can definitely use directly the mlir_tablegen command to have control on resulting *inc files. Will look into that :).

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.
@vguerra
Copy link
Contributor Author

vguerra commented Sep 29, 2023

Indeed ... one can definitely use directly the mlir_tablegen command to have control on resulting *inc files. Will look into that :).

addressed this in c44cb50

@fruffy
Copy link

fruffy commented Oct 10, 2023

Very useful, thanks! Is this ready to be used or are there still things missing?

@vguerra
Copy link
Contributor Author

vguerra commented Oct 11, 2023

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

@j2kun
Copy link
Owner

j2kun commented Oct 11, 2023

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.

@vguerra
Copy link
Contributor Author

vguerra commented Oct 11, 2023

where everyone is telling me how much they enjoy these tutorials.

+1 for that, this series of tutorials is very instructive and useful.

Copy link
Owner

@j2kun j2kun left a 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

Copy link
Owner

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 ../..

Copy link
Contributor Author

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 ..
Copy link
Owner

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, will change that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in #24

@j2kun
Copy link
Owner

j2kun commented Oct 16, 2023

LGTM! Will give you a day or two to respond to the above, then merge either way, and I can clean up the nits.

@j2kun
Copy link
Owner

j2kun commented Oct 16, 2023

Eh, I'll just merge it. Thanks!!!

@j2kun j2kun merged commit d04fa9c into j2kun:main Oct 16, 2023
2 checks passed
@vguerra
Copy link
Contributor Author

vguerra commented Oct 16, 2023

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?

Not sure what this errors means but will have a look at that and report back.

@vguerra
Copy link
Contributor Author

vguerra commented Oct 16, 2023

thanks for having a look and merging

@vguerra
Copy link
Contributor Author

vguerra commented Oct 23, 2023

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?

Not sure what this errors means but will have a look at that and report back.

addressed in #24

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.

3 participants