Skip to content

[math-libs] Enable rocPRIM benchmark builds - #4334

Merged
lajagapp merged 10 commits into
mainfrom
users/lajagapp/add-rocprim-bench
Jun 1, 2026
Merged

lajagapp merged 10 commits into
mainfrom
users/lajagapp/add-rocprim-bench

Conversation

@lajagapp

@lajagapp lajagapp commented Apr 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Enable building of rocPRIM benchmarks in TheRock's build system, with platform guards for Windows.

Changes

  • math-libs/CMakeLists.txt:
    • Added -DBUILD_BENCHMARK=ON to the rocPRIM_tests subproject CMake args (Linux)
    • Added amdsmi to BUILD_DEPS (benchmarks link against amd_smi via primbench.hpp)
    • Disabled benchmarks on Windows (-DBUILD_BENCHMARK=OFF) since amdsmi is Linux-only
    • Injected therock_rocprim_benchmark_deps.cmake via CMAKE_INCLUDES to provide the missing find_package(amd_smi) call
  • cmake/therock_rocprim_benchmark_deps.cmake (new):
  • artifact-prim.toml:
    • Added bin/benchmark_* to the include list

Details

rocPRIM ships ~40 benchmarks under rocm-libraries/projects/rocprim/benchmark/ covering block, device, and warp-level primitives. These were not previously built because BUILD_BENCHMARK defaults to OFF.

The CMAKE_INCLUDES workaround is needed because upstream rocPRIM's benchmark/CMakeLists.txt links against amd_smi but never calls find_package(amd_smi), which only matters in non-system-install environments like TheRock. See #4651 for the upstream fix.

Add -DBUILD_BENCHMARK=ON and amdsmi build dependency to the
rocPRIM_tests subproject so that rocPRIM benchmark binaries are
built alongside tests.

Signed-off-by: Lenine Ajagappane <Lenine.Ajagappane@amd.com>
…able

Signed-off-by: Lenine Ajagappane <Lenine.Ajagappane@amd.com>
Signed-off-by: Lenine Ajagappane <Lenine.Ajagappane@amd.com>
Signed-off-by: Lenine Ajagappane <Lenine.Ajagappane@amd.com>
@lajagapp
lajagapp marked this pull request as ready for review April 20, 2026 13:47

@geomin12 geomin12 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@lajagapp

Copy link
Copy Markdown
Contributor Author

build fails

https://github.com/ROCm/TheRock/actions/runs/24582835143/job/71892283369?pr=4334

This build failure is caused by recent rocm-libraries commit ROCm/rocm-libraries#5512. Now I've created github-issue for it #4739.

It wasn't caught by rocm-libraries CI because benchmarks aren't built in their pre-checkin workflow. Our PR is the first to actually compile rocPRIM benchmarks, which is why it is observed here.

I've filed a fix upstream: ROCm/rocm-libraries#6662. And started trial run now - https://github.com/ROCm/TheRock/actions/runs/24775330791

@lajagapp
lajagapp requested a review from geomin12 April 22, 2026 11:34
@lajagapp

Copy link
Copy Markdown
Contributor Author

Upstream fix is merged now, we need to wait for rocm-libraries submodule update with fix.

@geomin12 geomin12 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's wait for rocm-libraries bump. i would also appreciate review from rocprim folks if this makes sense

@lajagapp
lajagapp requested a review from geomin12 April 27, 2026 16:18
@lajagapp

Copy link
Copy Markdown
Contributor Author

let's wait for rocm-libraries bump. i would also appreciate review from rocprim folks if this makes sense

rocm-libraries submodule updated with fix. Rebased the PR now.

@geomin12 geomin12 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm but i'm going to let rocprim folks review and confirm this is good

@lajagapp
lajagapp merged commit 50b96d6 into main Jun 1, 2026
174 of 223 checks passed
@github-project-automation github-project-automation Bot moved this from TODO to Done in TheRock Triage Jun 1, 2026
@lajagapp
lajagapp deleted the users/lajagapp/add-rocprim-bench branch June 1, 2026 17:16
geomin12 pushed a commit that referenced this pull request Jun 4, 2026
…lable (#5612)

## Summary

- Guard `_rocprim_benchmark_enable` on `TARGET amdsmi` so the
prim-isolated CI build (`-DTHEROCK_ENABLE_ALL=OFF
-DTHEROCK_ENABLE_PRIM=ON`) no longer fails configure with
`get_target_property() called with non-existent target "amdsmi"`.
- Full SDK builds are unaffected — `amdsmi` is declared (via
`THEROCK_ENABLE_CORE_AMDSMI`), so the target is present, and benchmarks
build exactly as before.
- Mirrors the existing precedent at `core/CMakeLists.txt:589` for
`rocrtst`.

## Background

#4334 added `amdsmi` to `rocPRIM_tests` `BUILD_DEPS` unconditionally for
non-Windows, but `amdsmi` is only declared as a TheRock subproject when
`THEROCK_ENABLE_CORE_AMDSMI` is ON. The prim-isolated CI group doesn't
enable CORE, so `_therock_cmake_subproject_collect_build_deps` blows up
before configure completes.

This is breaking the rocm-libraries TheRock-pin bump in
[ROCm/rocm-libraries#7979](ROCm/rocm-libraries#7979)
(e.g. `Linux (rocthrust,rocprim,hipcub | gfx94X-dcgpu) / Build`).

Related upstream tracking: #4651 (rocPRIM should also gracefully skip
`find_package(amd_smi)` when not requested — once that lands, the
`therock_rocprim_benchmark_deps.cmake` workaround can be dropped, but
this guard is still the right shape regardless).

## Test plan

- [ ] TheRock CI green on full SDK (benchmarks still built — confirm
`benchmark_*` binaries under `dist/rocm/`)
- [x] TheRock CI green on any prim-isolated job (configure no longer
errors)
- [ ] After merge, repoint rocm-libraries#7979's `therock-ref` to the
merge SHA and re-run its failing build jobs
davidd-amd pushed a commit that referenced this pull request Jun 5, 2026
…lable (#5612)

## Summary

- Guard `_rocprim_benchmark_enable` on `TARGET amdsmi` so the
prim-isolated CI build (`-DTHEROCK_ENABLE_ALL=OFF
-DTHEROCK_ENABLE_PRIM=ON`) no longer fails configure with
`get_target_property() called with non-existent target "amdsmi"`.
- Full SDK builds are unaffected — `amdsmi` is declared (via
`THEROCK_ENABLE_CORE_AMDSMI`), so the target is present, and benchmarks
build exactly as before.
- Mirrors the existing precedent at `core/CMakeLists.txt:589` for
`rocrtst`.

## Background

#4334 added `amdsmi` to `rocPRIM_tests` `BUILD_DEPS` unconditionally for
non-Windows, but `amdsmi` is only declared as a TheRock subproject when
`THEROCK_ENABLE_CORE_AMDSMI` is ON. The prim-isolated CI group doesn't
enable CORE, so `_therock_cmake_subproject_collect_build_deps` blows up
before configure completes.

This is breaking the rocm-libraries TheRock-pin bump in
[ROCm/rocm-libraries#7979](ROCm/rocm-libraries#7979)
(e.g. `Linux (rocthrust,rocprim,hipcub | gfx94X-dcgpu) / Build`).

Related upstream tracking: #4651 (rocPRIM should also gracefully skip
`find_package(amd_smi)` when not requested — once that lands, the
`therock_rocprim_benchmark_deps.cmake` workaround can be dropped, but
this guard is still the right shape regardless).

## Test plan

- [ ] TheRock CI green on full SDK (benchmarks still built — confirm
`benchmark_*` binaries under `dist/rocm/`)
- [x] TheRock CI green on any prim-isolated job (configure no longer
errors)
- [ ] After merge, repoint rocm-libraries#7979's `therock-ref` to the
merge SHA and re-run its failing build jobs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants