Repository navigation
Conversation
f629edc to
6afcd5a
Compare
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
| # Passthrough -D<PROJECT_NAME>_ENABLE_COVERAGE=ON to the subproject | ||
| # Convert logical target name to uppercase (e.g., hipDNN -> HIPDNN_ENABLE_COVERAGE) | ||
| set(_coverage_arg) | ||
| string(TOUPPER "${_logical_target_name}" _coverage_project_name) | ||
| set(_coverage_var_name "${_coverage_project_name}_ENABLE_COVERAGE") | ||
|
|
||
| if(DEFINED ${_coverage_var_name}) | ||
| set(_coverage_arg "-D${_coverage_var_name}=${${_coverage_var_name}}") | ||
| endif() |
There was a problem hiding this comment.
Please document {project}_ENABLE_COVERAGE support here:
TheRock/docs/development/development_guide.md
Lines 178 to 202 in 6033313
| # Passthrough -D<PROJECT_NAME>_ENABLE_COVERAGE=ON to the subproject | ||
| # Convert logical target name to uppercase (e.g., hipDNN -> HIPDNN_ENABLE_COVERAGE) | ||
| set(_coverage_arg) | ||
| string(TOUPPER "${_logical_target_name}" _coverage_project_name) | ||
| set(_coverage_var_name "${_coverage_project_name}_ENABLE_COVERAGE") | ||
|
|
||
| if(DEFINED ${_coverage_var_name}) | ||
| set(_coverage_arg "-D${_coverage_var_name}=${${_coverage_var_name}}") | ||
| endif() |
There was a problem hiding this comment.
You can also move _coverage_arg up to be next to the other per-project CMake flags here:
TheRock/cmake/therock_subproject.cmake
Lines 872 to 881 in 6033313
and I think you can simplify this "check if the variable exists" to just match the style used for CMAKE_ARGS:
list(APPEND _cmake_args "${${target_name}_CMAKE_ARGS}")if the variable doesn't exist, list append will do nothing I think...
There was a problem hiding this comment.
I reordered the logic and used _cmake_args instead. I simplified the logic a little bit, but the TOUPPER is still required.
| # Convert logical target name to uppercase (e.g., hipDNN -> HIPDNN_ENABLE_COVERAGE) | ||
| set(_coverage_arg) | ||
| string(TOUPPER "${_logical_target_name}" _coverage_project_name) |
There was a problem hiding this comment.
If we standardize the case we should do it for all per-project variables. Is this actually needed?
There was a problem hiding this comment.
Yes it's needed, or rather should be there, as it ensures that we always get -DHIPDNN_ENABLE_COVERAGE, for example, instead of -DhipDNN_ENABLE_COVERAGE, which doesn't follow CMake's standard for definitions.
As it is defined, it will only check if the former is being passed, and if it is, propagate it to hipDNN. If someone uses some non-standard case, then it will ignore it.
See docs: 1ced5b9
There was a problem hiding this comment.
All per-project variables should use consistent casing, possibly standardizing with TOUPPER.
geomin12
left a comment
There was a problem hiding this comment.
lgtm but as Scott mentioned, docs to show the support!
4a1dbaa to
53d303c
Compare
Add documentation explaining that TheRock checks for uppercase PROJECT_ENABLE_COVERAGE flags and passes them through to subprojects. This clarifies that users must use HIPDNN_ENABLE_COVERAGE, not hipDNN_ENABLE_COVERAGE, as the code only checks for the uppercase form.
Propagate the COMPILER_RT_BUILD_PROFILE_ROCM flag from TheRock's top-level configure to the LLVM/compiler-rt sub-project build. This enables building libclang_rt.profile_rocm.a when coverage instrumentation is requested. The flag is set in the rocm-libraries coverage workflow and now properly reaches the compiler-rt configuration, allowing device-side profiling runtime to be built for HIP coverage builds.
Configure LLVM_RUNTIME_TARGETS with amdgcn-amd-amdhsa when COMPILER_RT_BUILD_PROFILE_ROCM is enabled. This builds compiler-rt for device targets, which is required for HIP device profiling. Per LLVM HIP offload PGO documentation, device coverage requires: - LLVM_RUNTIME_TARGETS="default;amdgcn-amd-amdhsa" - RUNTIMES_amdgcn-amd-amdhsa_LLVM_ENABLE_RUNTIMES="compiler-rt;libc" - RUNTIMES_amdgcn-amd-amdhsa_CACHE_FILES pointing to GPU.cmake - RUNTIMES_amdgcn-amd-amdhsa_RUNTIMES_USE_LIBC="llvm-libc" This enables building the device profiling runtime that provides __llvm_prf_* symbols needed by instrumented device code.
Correct the cache file reference from GPU.cmake (doesn't exist) to AMDGPU.cmake, and override COMPILER_RT_PROFILE_BAREMETAL to OFF. AMDGPU.cmake sets COMPILER_RT_PROFILE_BAREMETAL=ON which causes COMPILER_RT_BUILD_PROFILE_ROCM to default to OFF, disabling the device profiling runtime. Explicitly set both flags to ensure the device profiling runtime is built for coverage builds.
Coverage builds cross-compile compiler-rt for amdgcn targets in addition to host, significantly increasing memory usage. Flang is not required for HIP coverage instrumentation, so disable it when COMPILER_RT_BUILD_PROFILE_ROCM is enabled to prevent OOM during LLVM build. Minimal HIP offload PGO/coverage only requires clang and lld.
Flang will be kept enabled but with limited parallelism via LLVM_PARALLEL_LINK_JOBS to control memory usage.
Device profiling runtime cannot build for amdgcn targets because the profiling source code includes host OS headers (unistd.h, fcntl.h) that don't exist in freestanding GPU environments. Reverting commits: - b83de89 Forward COMPILER_RT_BUILD_PROFILE_ROCM to LLVM build - 1f7153f Enable amdgcn runtime targets for device coverage builds - 979c90b Fix amdgcn device profiling runtime configuration Will use host-only coverage instrumentation instead.
CreateIntrinsic now returns Value* instead of CallInst*, requiring explicit casts in SPIRVRegularizeLLVM and SPIRVReader.
Remove the coverage-specific branch that stripped down AMDGPU runtimes to just compiler-rt and libc. Device profiling doesn't conflict with libcxx, libcxxabi, flang-rt, or openmp - the minimal runtime list was purely a build-time optimization. Always build the full runtime set (compiler-rt, libc, libcxx, libcxxabi, flang-rt, openmp) for amdgcn-amd-amdhsa. When COMPILER_RT_BUILD_PROFILE_ROCM is enabled, just add the two coverage-specific flags without removing runtimes. This ensures other TheRock components that depend on libcxx or OpenMP aren't broken by coverage builds.
The coverage-specific logic that strips down amdgcn runtimes was already removed from the amd-staging block in commit 8a3c29b. This separate COMPILER_RT_BUILD_PROFILE_ROCM block (lines 71-92) was leftover redundant code that: 1. Removed Flang from LLVM_ENABLE_PROJECTS (conflicts with flang-rt in the amdgcn runtime list set by the earlier block) 2. Re-set LLVM_RUNTIME_TARGETS and amdgcn runtime config that's already handled in the amd-staging block Remove this entire block. Coverage builds now work the same as normal builds with the addition of COMPILER_RT_BUILD_PROFILE_ROCM=ON.
The device profiling runtime for amdgcn targets must use BAREMETAL=ON (the GPU.cmake default) because GPU device code cannot use filesystem operations or host-side initialization code. Device profiling support comes from InstrProfilingPlatformGPU.c which is automatically included for GPU targets when COMPILER_RT_BUILD_PROFILE=ON (already set by GPU.cmake cache). The previous override to BAREMETAL=OFF was attempting to enable the host-side libclang_rt.profile_rocm.a (InstrProfilingPlatformROCm.cpp), but that's only for host code. Device-side instrumentation works differently and requires BAREMETAL=ON. This fixes "Kernel launch failure" errors in hipRAND coverage tests caused by invalid device runtime configuration.
Motivation
We want to migrate code coverage jobs to TheRock
Technical Details
This PR introduces a passthrough for cmake definitions to their respective projects with:
Test Plan
We'll use the following PR to test that a code coverage job can be kicked off and run for any given project:
ROCm/rocm-libraries#4441
Test Result
Submission Checklist
Resolves: #6572