-
Notifications
You must be signed in to change notification settings - Fork 133
Move non-sysdep third-party projects after base and compiler. #2045
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
base: main
Are you sure you want to change the base?
Conversation
| add_subdirectory(third-party) | ||
| add_subdirectory(third-party/sysdeps) | ||
| add_subdirectory(base) | ||
| add_subdirectory(compiler) | ||
| add_subdirectory(third-party) | ||
| add_subdirectory(core) |
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.
amdsmi (which is Linux only) in base/ depends on therock-googletest which is not a sysdep, see https://github.com/ROCm/TheRock/actions/runs/19153803999/job/54750039189?pr=2045#step:10:119
-- Including subproject amdsmi (from /__w/TheRock/TheRock/base/amdsmi)
-- PROVIDE amd_smi = lib/cmake (from amdsmi)
-- RUNTIME_DEPS: rocm-core therock-libdrm
-- INJECT rocm-core = /__w/TheRock/TheRock/build/base/rocm-core/dist/lib/cmake/rocm-core (from rocm-core)
CMake Error at cmake/therock_subproject.cmake:1026 (get_target_property):
get_target_property() called with non-existent target "therock-googletest".
Call Stack (most recent call first):
cmake/therock_subproject.cmake:1039 (_therock_assert_is_cmake_subproject)
cmake/therock_subproject.cmake:667 (_therock_cmake_subproject_setup_deps)
base/CMakeLists.txt:76 (therock_cmake_subproject_activate)
Code:
Lines 61 to 69 in 77e4a83
| therock_cmake_subproject_declare(amdsmi | |
| EXTERNAL_SOURCE_DIR "amdsmi" | |
| USE_DIST_AMDGPU_TARGETS | |
| BACKGROUND_BUILD | |
| CMAKE_ARGS | |
| # TODO: Enable tests | |
| # -DBUILD_TESTS=${THEROCK_BUILD_TESTING} | |
| BUILD_DEPS | |
| therock-googletest |
This will need more work on Linux in that case
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.
Looks like we can either
- Move therock-googletest to 'sysdeps'
- Move amdsmi and rocm_smi_lib from 'base' to 'core'
I'm leaning towards (2) architecturally (since the libraries may expect googletest to be compiled with clang too), though (1) looks easier in the context of this PR.
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.
Let's do 2 and should be fine. That will also solve a problem for me with respect to enabling ASAN for amdsmi (which depends on the clang toolchain).
If we do that, we should create an amdsmi artifact vs lumping it into "base". It needs to be its own and will need its own package, etc (so I think this was done as-is in error).
I'd love to further eliminate base/ entirely but we can get there eventually.
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.
Okay, can proceed with (2).
I'm also trying a third option: fiddle with the include order even further for just googletest. Might be able to unblock bringup work for rocroller and hipdnn on Windows faster that way.
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.
Filed #2072 (assigned to @erman-gurses ) for moving amdsmi to 'core'. The include order hack for just googletest did pass CI here, so we could proceed with this if we don't want to wait for moving amdsmi around.
## Motivation Corrects the windows build to allow proper building of the MIOpen legacy plugin on Windows machines ## Technical Details - Add workaround for ROCm/TheRock#2003 - Backlog item to remove workaround when ROCm/TheRock#2045 is merged - Corrected runtime output location for hipDNN MIOpen plugin - A further PR for TheRock will enable Windows builds ## Test Plan - Run all tests ## Test Result - [x] Tests run successfully ## Submission Checklist - [x] clang-tidy - [x] clang-format
[hipDNN] [ALMIOPEN-396] MIOpen plugin building on windows (#2690) ## Motivation Corrects the windows build to allow proper building of the MIOpen legacy plugin on Windows machines ## Technical Details - Add workaround for ROCm/TheRock#2003 - Backlog item to remove workaround when ROCm/TheRock#2045 is merged - Corrected runtime output location for hipDNN MIOpen plugin - A further PR for TheRock will enable Windows builds ## Test Plan - Run all tests ## Test Result - [x] Tests run successfully ## Submission Checklist - [x] clang-tidy - [x] clang-format
Motivation
Progress on #1822. Fixes #2003.
Technical Details
This splits third-party into sysdeps and application libraries:
This is a step towards a larger bootstrapping effort where we should first build the compiler than use it consistently for all subprojects.
After the split, it is now possible to use the
amd-llvmtoolchain to build non-sysdep third-party projects. That is required for spdlog on Windows, due to how spdlog sets the/Zc:__cplusplusflag for MSVC (see the code here) and we don't want that propagating to later builds using clang.Test Plan
Built spdlog and tried building rocroller and hipdnn on Windows.
Test Result
rocroller and hipdnn had more compilation issues that can be fixed after this.
Submission Checklist