-
Notifications
You must be signed in to change notification settings - Fork 303
fix missing LIBOMPTARGET_DEVICE_ARCHITECTURES in third stage of bootstrap build in LLVM easyblock
#3851
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
fix missing LIBOMPTARGET_DEVICE_ARCHITECTURES in third stage of bootstrap build in LLVM easyblock
#3851
Conversation
By using `_cmakeopts`, the argument was only passed for the first stage of the build, and ignored for subsequent stages. We therefore build all available architectures for AMDGPU / NVPTX when not necessary. Fix this by moving the argument to `general_opts`. Signed-off-by: Jan André Reuter <[email protected]>
LIBOMPTARGET_DEVICE_ARCHITECTURES in third stage of bootstrap build
|
@boegelbot please test @ jsc-zen3 |
|
@Thyre: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 3102298599 processed Message to humans: this is just bookkeeping information for me, |
Flamefire
left a comment
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.
Good catch. Maybe add documentation to those 2 members because it is hard to tell what the difference is which led to this issue
Good idea. I'll try to find a place where we can document this. |
|
Yeah, I introduced that mistake in bb67396#diff-9560297abd14bd1649f4ea97ffc545517160060a13db3b3194ac97a3c2541049R506-R792 due to this confusion Not sure why my tests didn't show this, i.e. why I've only seen the expected arch files. |
Signed-off-by: Jan André Reuter <[email protected]>
|
It fortunately isn't a really noticeable regression. We just build a few architectures too much. I wonder though if it would be worth adding tests / sanity checks to avoid this from happening again (in a separate PR...). |
Co-authored-by: Alexander Grund <[email protected]>
|
Test report by @boegelbot Overview of tested easyconfigs (in order)
Build succeeded for 2 out of 2 (2 easyconfigs in total) |
|
$ grep -rni "\-DLIBOMPTARGET_DEVICE_ARCHITECTURES"
[...]
easybuild-LLVM-19.1.7-20250722.135204.log:1540:== 2025-07-22 11:34:16,167 run.py:502 INFO Running shell command 'cmake -DCMAKE_INSTALL_LIBDIR:PATH=lib -DCMAKE_FIND_USE_PACKAGE_REGISTRY=OFF -DLLVM_ENABLE_PROJECTS='llvm;clang;lld' -DLLVM_ENABLE_RUNTIMES='compiler-rt;libunwind;libcxx;libcxxabi' -DCMAKE_VERBOSE_MAKEFILE=ON -DLLVM_INCLUDE_BENCHMARKS=OFF -DLLVM_INSTALL_UTILS=ON -DPython3_FIND_VIRTUALENV=STANDARD -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON -DLIBCXX_ENABLE_SHARED=ON -DLIBCXXABI_ENABLE_SHARED=ON -DLIBUNWIND_ENABLE_SHARED=ON -DLLVM_ENABLE_RTTI=ON -DLLVM_REQUIRES_RTTI=ON -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCLANG_DEFAULT_LINKER=lld -DFLANG_DEFAULT_LINKER=lld -DCMAKE_BUILD_TYPE=Release -DLLVM_TARGETS_TO_BUILD='X86;NVPTX' -DCMAKE_INSTALL_PREFIX=/project/def-maintainers/boegelbot/rocky9/zen3/software/LLVM/19.1.7-GCCcore-13.3.0 -DLLVM_ENABLE_ASSERTIONS=OFF -DLLVM_ENABLE_LIBXML2=ON -DLLVM_ENABLE_ZLIB=ON -DLLVM_ENABLE_ZSTD=OFF -DLLDB_ENABLE_SWIG=OFF -DLIBOMP_OMPD_GDB_SUPPORT=OFF -DLLVM_ENABLE_Z3_SOLVER=ON -DLLVM_Z3_INSTALL_DIR=/project/def-maintainers/boegelbot/rocky9/zen3/software/Z3/4.13.0-GCCcore-13.3.0 -DPYTHON_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python -DPython_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python -DPython3_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python -DLIBOMPTARGET_DEVICE_ARCHITECTURES='sm_80' -DRUNTIMES_CMAKE_ARGS='-DCMAKE_C_FLAGS=--gcc-install-dir=/project/def-maintainers/boegelbot/rocky9/zen3/software/GCCcore/13.3.0/lib/gcc/x86_64-pc-linux-gnu/13.3.0;-DCMAKE_CXX_FLAGS=--gcc-install-dir=/project/def-maintainers/boegelbot/rocky9/zen3/software/GCCcore/13.3.0/lib/gcc/x86_64-pc-linux-gnu/13.3.0;-DPYTHON_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python;-DPython_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python;-DPython3_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python' /tmp/boegelbot/LLVM/19.1.7/GCCcore-13.3.0/llvm-project-19.1.7.src/llvm' in /tmp/boegelbot/LLVM/19.1.7/GCCcore-13.3.0/llvm.obj.1
[...]
easybuild-LLVM-19.1.7-20250722.135204.log:26661:== 2025-07-22 11:55:39,680 run.py:502 INFO Running shell command 'cmake -DCMAKE_VERBOSE_MAKEFILE=ON -DLLVM_INCLUDE_BENCHMARKS=OFF -DLLVM_INSTALL_UTILS=ON -DPython3_FIND_VIRTUALENV=STANDARD -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON -DLIBCXX_ENABLE_SHARED=ON -DLIBCXXABI_ENABLE_SHARED=ON -DLIBUNWIND_ENABLE_SHARED=ON -DLLVM_ENABLE_RTTI=ON -DLLVM_REQUIRES_RTTI=ON -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCLANG_DEFAULT_LINKER=lld -DFLANG_DEFAULT_LINKER=lld -DCMAKE_BUILD_TYPE=Release -DLLVM_TARGETS_TO_BUILD='X86;NVPTX' -DCMAKE_INSTALL_PREFIX=/project/def-maintainers/boegelbot/rocky9/zen3/software/LLVM/19.1.7-GCCcore-13.3.0 -DLLVM_ENABLE_ASSERTIONS=OFF -DLLVM_ENABLE_LIBXML2=ON -DLLVM_ENABLE_ZLIB=ON -DLLVM_ENABLE_ZSTD=OFF -DLLDB_ENABLE_SWIG=OFF -DLIBOMP_OMPD_GDB_SUPPORT=OFF -DLLVM_ENABLE_Z3_SOLVER=ON -DLLVM_Z3_INSTALL_DIR=/project/def-maintainers/boegelbot/rocky9/zen3/software/Z3/4.13.0-GCCcore-13.3.0 -DPYTHON_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python -DPython_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python -DPython3_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python -DLIBOMPTARGET_DEVICE_ARCHITECTURES='sm_80' -DRUNTIMES_CMAKE_ARGS='-DCMAKE_C_COMPILER=/tmp/eb-wnuge6w7/tmpukrjpa9e/rpath_wrappers/clang_wrapper/clang;-DCMAKE_C_FLAGS=--gcc-install-dir=/project/def-maintainers/boegelbot/rocky9/zen3/software/GCCcore/13.3.0/lib/gcc/x86_64-pc-linux-gnu/13.3.0;-DCMAKE_CXX_COMPILER=/tmp/eb-wnuge6w7/tmpukrjpa9e/rpath_wrappers/clangxx_wrapper/clang++;-DCMAKE_CXX_FLAGS=--gcc-install-dir=/project/def-maintainers/boegelbot/rocky9/zen3/software/GCCcore/13.3.0/lib/gcc/x86_64-pc-linux-gnu/13.3.0;-DPYTHON_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python;-DPython_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python;-DPython3_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python' -DLLVM_ENABLE_PROJECTS='llvm;clang;lld' -DLLVM_ENABLE_RUNTIMES='compiler-rt;libunwind;libcxx;libcxxabi' -DCMAKE_C_COMPILER=/tmp/eb-wnuge6w7/tmp4c_qge53/rpath_wrappers/clang_wrapper/clang -DCMAKE_CXX_COMPILER=/tmp/eb-wnuge6w7/tmp4c_qge53/rpath_wrappers/clangxx_wrapper/clang++ -DCMAKE_ASM_COMPILER=/tmp/eb-wnuge6w7/tmp4c_qge53/rpath_wrappers/clang_wrapper/clang -DCMAKE_ASM_COMPILER_ID=Clang /tmp/boegelbot/LLVM/19.1.7/GCCcore-13.3.0/llvm-project-19.1.7.src/llvm' in /tmp/boegelbot/LLVM/19.1.7/GCCcore-13.3.0/llvm.obj.2
[...]
easybuild-LLVM-19.1.7-20250722.135204.log:74143:== 2025-07-22 12:14:54,566 run.py:502 INFO Running shell command 'cmake -DCMAKE_VERBOSE_MAKEFILE=ON -DLLVM_INCLUDE_BENCHMARKS=OFF -DLLVM_INSTALL_UTILS=ON -DPython3_FIND_VIRTUALENV=STANDARD -DLLVM_BUILD_LLVM_DYLIB=ON -DLLVM_LINK_LLVM_DYLIB=ON -DLIBCXX_ENABLE_SHARED=ON -DLIBCXXABI_ENABLE_SHARED=ON -DLIBUNWIND_ENABLE_SHARED=ON -DLLVM_ENABLE_RTTI=ON -DLLVM_REQUIRES_RTTI=ON -DCMAKE_POSITION_INDEPENDENT_CODE=ON -DCLANG_DEFAULT_LINKER=lld -DFLANG_DEFAULT_LINKER=lld -DCMAKE_BUILD_TYPE=Release -DLLVM_TARGETS_TO_BUILD='X86;NVPTX' -DCMAKE_INSTALL_PREFIX=/project/def-maintainers/boegelbot/rocky9/zen3/software/LLVM/19.1.7-GCCcore-13.3.0 -DLLVM_ENABLE_ASSERTIONS=OFF -DLLVM_ENABLE_LIBXML2=ON -DLLVM_ENABLE_ZLIB=ON -DLLVM_ENABLE_ZSTD=OFF -DLLDB_ENABLE_SWIG=OFF -DLIBOMP_OMPD_GDB_SUPPORT=OFF -DLLVM_ENABLE_Z3_SOLVER=ON -DLLVM_Z3_INSTALL_DIR=/project/def-maintainers/boegelbot/rocky9/zen3/software/Z3/4.13.0-GCCcore-13.3.0 -DPYTHON_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python -DPython_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python -DPython3_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python -DLIBOMPTARGET_DEVICE_ARCHITECTURES='sm_80' -DRUNTIMES_CMAKE_ARGS='-DCMAKE_C_COMPILER=/tmp/eb-wnuge6w7/tmpomzrnagi/rpath_wrappers/clang_wrapper/clang;-DCMAKE_C_FLAGS=--gcc-install-dir=/project/def-maintainers/boegelbot/rocky9/zen3/software/GCCcore/13.3.0/lib/gcc/x86_64-pc-linux-gnu/13.3.0;-DCMAKE_CXX_COMPILER=/tmp/eb-wnuge6w7/tmpomzrnagi/rpath_wrappers/clangxx_wrapper/clang++;-DCMAKE_CXX_FLAGS=--gcc-install-dir=/project/def-maintainers/boegelbot/rocky9/zen3/software/GCCcore/13.3.0/lib/gcc/x86_64-pc-linux-gnu/13.3.0;-DPYTHON_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python;-DPython_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python;-DPython3_EXECUTABLE=/project/def-maintainers/boegelbot/rocky9/zen3/software/Python/3.12.3-GCCcore-13.3.0/bin/python' -DLLVM_ENABLE_PROJECTS='llvm;mlir;clang;flang;openmp;polly;clang-tools-extra;lld;lldb;bolt' -DLLVM_ENABLE_RUNTIMES='compiler-rt;libunwind;libcxx;libcxxabi;offload' -DLIBOMPTARGET_PLUGINS_TO_BUILD='host;cuda' -DLIBOMPTARGET_DLOPEN_PLUGINS='cuda' -DOPENMP_ENABLE_LIBOMPTARGET=ON -DLIBOMP_INSTALL_ALIASES=OFF -DLLVM_LIT_ARGS="-j 16 -v --timeout 300" -DLLVM_POLLY_LINK_INTO_TOOLS=ON -DLLVM_INCLUDE_TESTS=ON -DLLVM_BUILD_TESTS=ON -DCMAKE_C_COMPILER=/tmp/eb-wnuge6w7/tmpvo100a5a/rpath_wrappers/clang_wrapper/clang -DCMAKE_CXX_COMPILER=/tmp/eb-wnuge6w7/tmpvo100a5a/rpath_wrappers/clangxx_wrapper/clang++ -DCMAKE_ASM_COMPILER=/tmp/eb-wnuge6w7/tmpvo100a5a/rpath_wrappers/clang_wrapper/clang -DCMAKE_ASM_COMPILER_ID=Clang /tmp/boegelbot/LLVM/19.1.7/GCCcore-13.3.0/llvm-project-19.1.7.src/llvm' in /tmp/boegelbot/LLVM/19.1.7/GCCcore-13.3.0/llvm.obj.3 |
Crivella
left a comment
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.
LGTM, tested both here and in easybuilders/easybuild-easyconfigs#23459
|
Going in, thanks @Thyre! |
| # CMake options passed to each build stage. | ||
| # Will be cleared between stages. If arguments are needed in multiple stages, | ||
| # consider adding them to general_opts instead. | ||
| self._cmakeopts = {} |
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.
Too late for this PR now, but I thought we could set this to None at this point such that adding dict entries to this too early would fail loudly.
What do you think?
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.
Probably a good idea. We can either add this in a separate PR, or just add it to one of the remaining LLVM ones.
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.
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.
I think we are already setting it at the edge where we want to access it. If you try to use it before in the init it will be non-existent and fail regardless
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.
I think it must only be used after the _configure_general_build call in configure_step: https://github.com/Thyre/easybuild-easyblocks/blob/9771da4f8969ba373c19b99a6988b3b65113d225/easybuild/easyblocks/l/llvm.py#L833
There cmakeopts should be initialized to a copy of generalopts. Or is there any earlier use intended? Maybe derived easyblocks adding options to that before calling the super-method?
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.
Then I dont understand your remark:
Only if
_cmakeoptswas not an attribute there which i still think it should.
Getting an error is intended to detect using it too early. And IMO the explicit None shows that it does not exist yet and provides means to add a comment what it is and where it will be set.
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.
What i meant is that i think _cmakeopts should be already usable in the configure step.
I think unless we decide do alter the logic quite a bit this will not really save us from PRs using the wrong attributes/methods if we do not catch it in review...
What i think is more worth the effort is increase the sanity checks to make sure all the feature we want on are on.
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.
But should it be usable before _configure_general_build in the configure step?
I'd just make it as hard as possible to use the wrong one if it doesn't impact what we really need and my suggestion is a step towards it while not impacting current usage as far as I can tell.
Better sanity checks would be good either way, yes.
Then maybe the minimal common ground would be renaming the generically named cmakeopts to something more specific like "_stage_cmake_opts"? That would make it easier to review without any real change
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.
But should it be usable before _configure_general_build in the configure step?
I would say so, otherwise we would need a section for updating general_opts based on present packages (what we have right now before _configure_general_build in the configure_step and one to update the single stage after.
I feel that having 2 section there could also lead to confusion and people misplacing stuff (eg modifying general_opts after the call to _configure_general_build
Then maybe the minimal common ground would be renaming the generically named cmakeopts to something more specific like "_stage_cmake_opts"? That would make it easier to review without any real change
Not against renaming variables for clarity, might even make it_current_stage_cmake_optsfor clarity
I think the only way to really make sure stuff is not misused is to introduce 4 methods:
- add_stage1_cmake_opts
- add_stage2_cmake_opts
- add_stage3_cmake_opts
- add_all_stages_cmake_opts
And than we could ensure they are never called after a particular stage has already been ran, and that the internal methods are never touched manually.
This would require quite a lot of refactoring, and high possibility to introduce new errors which i would avoid for now
PS would also require rewriting the logic in how the configopts coming from the EC file now are treated as general_opts + would need to streamline and clarify how stages are treated in the case of bootstrap vs not
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.
I think the only way to really make sure stuff is not misused is to introduce 4 methods:
Would it then need to be a property at all? The current member methods that additionally modify it could just a return a dict that is then used to update a local cmakeopts
LIBOMPTARGET_DEVICE_ARCHITECTURES in third stage of bootstrap buildLIBOMPTARGET_DEVICE_ARCHITECTURES in third stage of bootstrap build in LLVM easyblock
By using
_cmakeopts, the argument was only passed for the first stage of the build, and ignored for subsequent stages. We therefore build all available architectures for AMDGPU / NVPTX when not necessary. Fix this by moving the argument togeneral_opts.