-
Notifications
You must be signed in to change notification settings - Fork 30
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
Build and cache llvm #736
Build and cache llvm #736
Conversation
ef7c7ff
to
b1a1aee
Compare
b1a1aee
to
f9d090f
Compare
6afca33
to
b8f64ac
Compare
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.
The extra build feels rather complicated to me, and it would be nice if iree-amd-aie didn't need to build LLVM itself in this way, but the approach you've taken looks good on the surface at least.
.github/workflows/ci-windows.yml
Outdated
git init | ||
git remote add origin $REPO_ADDRESS | ||
git -c protocol.version=2 fetch --depth 1 origin $BRANCH_NAME | ||
git reset --hard FETCH_HEAD | ||
git -c submodule."third_party/torch-mlir".update=none -c submodule."third_party/stablehlo".update=none -c submodule."src/runtime_src/core/common/aiebu".update=none submodule update --init --recursive --depth 1 --single-branch -j 10 |
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.
Another option for this style of git operations is to use a script like https://github.com/iree-org/iree/blob/main/build_tools/scripts/git/update_runtime_submodules.sh. Sample usage here: https://github.com/iree-org/iree/blob/45132f5b1f1697e1f4d3450f0d6371c2ec49700a/.github/workflows/ci.yml#L209-L213. That does require checking out the repo with the script (without submodules) though.
With such a script, developers and workflows can share the same steps without copy/pasting all this.
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.
in general i'm not a fan of hiding stuff in scripts that get used in CI ymls - at least until github let's you jump to those scripts like it does for names in cpp. the exception (at least for iree-amd-aie) being cmake configure/build, which we do put into a script.
here in particular, this incantation isn't useful for devs because it's overly complex because it needs to work for forks (which i guess you well know from that example repo you showed me).
.github/workflows/ci-windows.yml
Outdated
- name: Hack IREE | ||
shell: bash | ||
run: | | ||
# TODO(max): send IREE a fix for this | ||
# target_compile_definitions may only set INTERFACE properties on IMPORTED | ||
sed -i '114s/PRIVATE/INTERFACE/g' third_party/iree/compiler/src/iree/compiler/API/CMakeLists.txt |
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.
👀
Is 114 a line number, pointing here: https://github.com/iree-org/iree/blob/45132f5b1f1697e1f4d3450f0d6371c2ec49700a/compiler/src/iree/compiler/API/CMakeLists.txt#L114? At a minimum it would be nice to make that more stable and not reliant on the file remaining unchanged.
I haven't noticed that "target_compile_definitions may only set INTERFACE properties on IMPORTED" error/warning upstream. Seems reasonable enough to patch it if it is causing issues downstream.
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.
yea that's the line - i'm guessing no one ever exercises the combo of "windows" + "byo llvm".
-DCMAKE_EXE_LINKER_FLAGS_INIT=-fuse-ld=lld ` | ||
-DCMAKE_SHARED_LINKER_FLAGS_INIT=-fuse-ld=lld ` | ||
-DCMAKE_MODULE_LINKER_FLAGS_INIT=-fuse-ld=lld ` |
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.
FYI CMake 3.29+ has access to -DCMAKE_LINKER_TYPE=LLD
: https://cmake.org/cmake/help/latest/variable/CMAKE_LINKER_TYPE.html
(We plan on bumping the CMake minimum in IREE and replacing -DIREE_ENABLE_LLD=ON
with that official option)
b8f64ac
to
15b9276
Compare
yea i mean this is what we discussed on slack earlier this week - i would love to be able to either
or
both paths would save us a lot of CI and dev. but it's spot time right (full rebuild once every ~2 weeks when we bump iree if you're not sloppy and the bump is major). it mostly impacts new comers/explorers/tire kickers which i do care about but not so much that i'm gonna implement a plugin system for IREE lol. amortized it's not a lot of time so for now shaving 60% instead of 90% is good enough. revisit later 🤷 |
fea8be2
to
bf92851
Compare
b5458ef
to
7a7968e
Compare
7a7968e
to
81da01f
Compare
This PR splits up build only in CI into two steps
Why do this if it only affects CI? Because now we can also cache/save/upload the LLVM distro for each successful build thereby giving everyone (not just CI) a distro of LLVM they can use to build iree locally. This cuts down the number of files you have to compile by like 60% (from 7500 -> 2900).
In case you didn't know, the CI artifacts are at the bottom of the summary page for any given run:
EDIT: in fact we actually need this for HSA (because ROCR requires an up-to-date Clang)1.
EDIT2: This PR now also adds powershell scripts for building on windows
Footnotes
https://github.com/nod-ai/ROCR-Runtime/blob/amd-staging/runtime/hsa-runtime/core/runtime/blit_shaders/CMakeLists.txt#L71 ↩