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

Introduce a single CMakeLists.txt #2118

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

fzakaria
Copy link
Contributor

@fzakaria fzakaria commented Mar 21, 2024

Build MLIR & StableHLO via a much simpler CMake configure & build command. This will make the build_mlir.sh script unecessary.

This should simplify setting CMake values to be consistent across MLIR and StableHLO builds such as PYTHON_BINDINGS.

You can now easily configure & build the whole StableHLO & LLVM build via two CMake commands.

The following will configure & build StableHLO and the dependent MLIR/LLVM code.

cmake --preset debugcmake --build ./build --target check-stablehlo-tests
[0/1] Running the tests/ suite

Testing Time: 4.08s

Total Discovered Tests: 137
  Passed: 137 (100.00%)

I am still learning the usefulness of CMakePresets and striking a good balance on how to use them. Discussing them on CMake's discourse has shown that the feature was introduced by CLion and is a bit IDE-centric. It's definitely not the panacea 1 to define all configurations but I believe in this particular case it helps streamline development.

Build MLIR & StableHLO via a single cmake configure & build command.
This will make the build_mlir.sh script unecessary.

This should simplify setting cmake values to be consistent across MLIR
and StableHLO builds such as PYTHON_BINDINGS.
* Simplified GitHub action
* Removed uneeded ci scripts now
* Fixed whitespace
Comment on lines +74 to +75
cmake --preset debug
cmake --build ./build --target check-stablehlo-ci
Copy link
Contributor Author

Choose a reason for hiding this comment

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

CMake has a concept of a "workflow" that can do this but there is some weird history to CMakePresets [1]

[1] https://gitlab.kitware.com/cmake/cmake/-/issues/22538

@fzakaria
Copy link
Contributor Author

fzakaria commented Mar 25, 2024

There is also CMakeFetchContent which may also even simplify this more.
https://cmake.org/cmake/help/latest/module/FetchContent.html
Something to consider.

Here is an example

@fzakaria fzakaria marked this pull request as ready for review March 26, 2024 00:04
Comment on lines +97 to +118
set(LLVM_INCLUDE_EXAMPLES OFF CACHE BOOL "")
set(LLVM_INCLUDE_TESTS OFF CACHE BOOL "")
set(LLVM_INCLUDE_BENCHMARKS OFF CACHE BOOL "")
set(LLVM_APPEND_VC_REV OFF CACHE BOOL "")
set(LLVM_ENABLE_IDE ON CACHE BOOL "")
set(LLVM_ENABLE_BINDINGS OFF CACHE BOOL "")
# LLVM defaults to building all targets. We always enable targets that we need
# as we need them, so default to none. The user can override this as needed,
# which is fine.
set(LLVM_TARGETS_TO_BUILD "" CACHE STRING "")

# We enable LLVM projects as needed. The user can override this.
set(LLVM_ENABLE_PROJECTS "" CACHE STRING "")
set(LLVM_EXTERNAL_PROJECTS "" CACHE STRING "")

# Unconditionally enable mlir.
list(APPEND LLVM_ENABLE_PROJECTS mlir)

# Setup LLVM lib and bin directories.
set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/llvm-project/bin)
set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/llvm-project/lib)
set(LLVM_TOOLS_BINARY_DIR ${CMAKE_BINARY_DIR}/llvm-project/bin)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

having to set these here are pretty ugly; we can discuss if this is an adequate trade-off.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this does and/or why it's needed. I believe this sets where compiled LLVM binaries/libraries will go? I'm assuming LLVM sets this already? Are the values it sets inappropriate for us?

@fzakaria fzakaria requested a review from GleasonK March 26, 2024 00:09
@mlevesquedion mlevesquedion self-requested a review March 27, 2024 15:19
Copy link
Contributor

@mlevesquedion mlevesquedion left a comment

Choose a reason for hiding this comment

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

A few thoughts:

  • First of all, this is a massive quality of life improvement and makes the project way more approachable. Even better would be if the developer didn't have to manually clone LLVM and it was just managed by the build system (I believe this is the case with the Bazel build). Regardless, this is huge.
  • I don't really know CMake so take these comments with a grain of salt: 1. Some of the config seems kinda fragile, like it may be relying on implementation details of LLVM's CMake build; this could lead to issues down the road; 2. Because I don't know CMake, I'm worried about maintainability. I feel like this is making the config more complicated than before and it feels like it would be harder to change/debug. I'm also wondering if this will have an impact on building StableHLO embedded in another project. (I don't think so, but I'm not 100% confident.)

Overall, I'm in favor of this change. I think the complexity it adds for project maintainers is worth the gains for everyone.

Another thought, vaguely related to this change: I don't love that we have two build systems. I wish we could generate CMakeLists.txt from Bazel or something. There is some prior art here but no established solution unfortunately: https://groups.google.com/g/bazel-discuss/c/MlI-xk034lw, https://github.com/google/bazel-to-cmake

CMAKE_BUILD_TYPE: Release
STABLEHLO_ENABLE_BINDINGS_PYTHON: OFF
STABLEHLO_ENABLE_SANITIZER: address
cmake --preset debug
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we have some kind of release preset? debug feels like the wrong setting for CI.

STABLEHLO_ENABLE_BINDINGS_PYTHON: OFF
STABLEHLO_ENABLE_SANITIZER: address
cmake --preset debug
cmake --build ./build --target check-stablehlo-ci
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I believe build is sufficient (instead of ./build).

# These defaults are moderately important to us, but the user *can*
# override them (enabling some of these brings in deps that will conflict,
# so ymmv).
# https://github.com/openxla/iree/blob/f3b6bcd79b24ef4a9b355eb3f3496ffafcbd0881/build_tools/cmake/iree_llvm.cmake#L127
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider giving some context about what this link represents in the comment. It seems the lines below were mostly taken from there?

Comment on lines +97 to +118
set(LLVM_INCLUDE_EXAMPLES OFF CACHE BOOL "")
set(LLVM_INCLUDE_TESTS OFF CACHE BOOL "")
set(LLVM_INCLUDE_BENCHMARKS OFF CACHE BOOL "")
set(LLVM_APPEND_VC_REV OFF CACHE BOOL "")
set(LLVM_ENABLE_IDE ON CACHE BOOL "")
set(LLVM_ENABLE_BINDINGS OFF CACHE BOOL "")
# LLVM defaults to building all targets. We always enable targets that we need
# as we need them, so default to none. The user can override this as needed,
# which is fine.
set(LLVM_TARGETS_TO_BUILD "" CACHE STRING "")

# We enable LLVM projects as needed. The user can override this.
set(LLVM_ENABLE_PROJECTS "" CACHE STRING "")
set(LLVM_EXTERNAL_PROJECTS "" CACHE STRING "")

# Unconditionally enable mlir.
list(APPEND LLVM_ENABLE_PROJECTS mlir)

# Setup LLVM lib and bin directories.
set(LLVM_RUNTIME_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/llvm-project/bin)
set(LLVM_LIBRARY_OUTPUT_INTDIR ${CMAKE_BINARY_DIR}/llvm-project/lib)
set(LLVM_TOOLS_BINARY_DIR ${CMAKE_BINARY_DIR}/llvm-project/bin)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this does and/or why it's needed. I believe this sets where compiled LLVM binaries/libraries will go? I'm assuming LLVM sets this already? Are the values it sets inappropriate for us?

Testing Time: 4.13s

Total Discovered Tests: 137
Passed: 137 (100.00%)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: This would be accurate if we were running check-stablehlo-tests, but because we are running check-stablehlo-ci 4 test suites need to run.

IMO, check-stablehlo-tests is more appropriate because the other 3 suites (tosa, linalg, testdata) are not as relevant to day-to-day development.

Also I commented about this elsewhere, and IDK if it's because we're using a debug build or something else, but running these tests is usually way faster for me. On main, I can run all 3888 tests in under 10 seconds, but with this branch it takes over a minute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's add a release preset and test against that?
I think starting the debug binaries are expensive.

@ghpvnist ghpvnist added the Build label Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants