Skip to content

[hipthreads] Add hipThreads build recipe and artifact to TheRock#6769

Open
marsavicc wants to merge 6 commits into
mainfrom
users/marsavic/hipthreads-therock-build
Open

[hipthreads] Add hipThreads build recipe and artifact to TheRock#6769
marsavicc wants to merge 6 commits into
mainfrom
users/marsavic/hipthreads-therock-build

Conversation

@marsavicc

@marsavicc marsavicc commented Jul 22, 2026

Copy link
Copy Markdown

Motivation

JIRA ID : ROCM-23943

Onboard hipThreads (GPU-side std::thread-style concurrency library, now living in rocm-libraries) into TheRock so it builds as part of ROCm.

Technical Details

  • Add the hipthreads build recipe in math-libs/CMakeLists.txt, pulling source from ${THEROCK_ROCM_LIBRARIES_SOURCE_DIR}/projects/hipthreads.
  • Register [artifacts.hipthreads] in BUILD_TOPOLOGY.toml (target-neutral: the static libhipthreads.a is identical across architectures) and add math-libs/artifact-hipthreads.toml.
  • Register libhipcxx as a CMake package provider so hipThreads can find_package(libhipcxx) (hipThreads is the first consumer of it this way).
  • Build with -DDISABLE_WERROR=ON (HIP runtime headers trip hipThreads' strict warnings) and document the new THEROCK_ENABLE_HIPTHREADS flag.

Test Plan

Build hipThreads via TheRock against rocm-libraries.

Test Result

Builds succesfully and produces the hipthreads artifact.

Note: CI is red until the TheRock rocm-libraries submodule is bumped to a SHA that includes projects/hipthreads (migration landed after the current pin).

Submission Checklist

@therock-pr-bot

therock-pr-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

❌ PR Check — Action Required

Check Status Details
🌿 Branch Name ✅ Pass
📝 PR Title/Description ❌ Fail Error: PR description must reference a JIRA ID, ISSUE ID, or a GitHub closing keyword.
Expected: include a JIRA ID / ISSUE ID line (separator : or -, or omitted; value may be a JIRA key, a number with/without #, or a link), OR a closing keyword + issue reference. Accepted examples:
JIRA ID : TESTAUTO-6039
JIRA ID - #330
JIRA ID #330
JIRA ID (on separate line)
ROCM-25757
ISSUE ID : TESTUTO-3334
ISSUE ID #3334
ISSUE ID - TESTAUTO-3433
ISSUE ID (on separate line)
AIRUNTIME-2352
ISSUE ID : https://github.com/<org_name>/<repo_name>/issues/1234
Closes #10
Fixes octo-org/octo-repo#100
Resolves: #123
#123
https://github.com/<org_name>/<repo_name>/issues/123
Current: no valid JIRA/ISSUE/closing-keyword reference found
Forbidden Files ✅ Pass
🧪 Unit Test ✅ Pass PR does not contain code files — Unit Test auto-passed
🔎 pre-commit ❌ Fail Error: Check concluded with failure.
🚫 Draft PR 🔜 To Be Enabled
🚩 Feature Flag 🔜 To Be Enabled
📊 Code Coverage 🔜 To Be Enabled

⚠️ 2 policy check(s) failed. Please address the issues above before this PR can be Reviewed.

🚫 Please fix the failed policies

  • ❌ PR Title/Description
  • ❌ pre-commit

The Not ready to Review label was added to this PR. Once all policies pass, the label is removed automatically.

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

@therock-pr-bot

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Title/Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

@therock-pr-bot therock-pr-bot Bot added the Not ready to Review PR has unresolved policy failures — reviews blocked label Jul 22, 2026

@marbre marbre left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Some initial feedback.

Comment thread math-libs/artifact-hipthreads.toml Outdated
Comment on lines +2 to +5
[components.lib."math-libs/hipthreads/stage"]
optional = true
[components.dbg."math-libs/hipthreads/stage"]
optional = true

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why those are marked optional? I think we can drop those if empty.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The static lib lands in dev, so lib/dbg were empty. I'll drop both.

Comment thread math-libs/CMakeLists.txt Outdated
Comment on lines +551 to +552
# Make libhipcxx's CMake package resolvable by dependents (e.g. hipthreads)
# via find_package(libhipcxx).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
# Make libhipcxx's CMake package resolvable by dependents (e.g. hipthreads)
# via find_package(libhipcxx).

Comment thread math-libs/CMakeLists.txt Outdated
# on therock_provide_artifact below) so per-arch CI upload races are harmless.
USE_TEST_AMDGPU_TARGETS
DEFAULT_GPU_TARGETS
gfx1201 # Fixes issue with empty target list for unsupported architectures.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why do we pick gfx1201 here? This needs an public issue, linked here instead of the comment.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This was copied from the libhipcxx recipe (I looked up that PR for reference), but hipthreads supports all architectures and is never in EXCLUDE_TARGET_PROJECTS, so the DEFAULT_GPU_TARGETS fallback never fires for us.

Removed it rather than link an issue for a workaround we don't need.

Comment thread math-libs/CMakeLists.txt Outdated
-DHIP_PLATFORM=amd
-DROCM_PATH=
-DROCM_DIR=
-DCMAKE_BUILD_WITH_INSTALL_RPATH=ON # Needed for Ninja build?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
-DCMAKE_BUILD_WITH_INSTALL_RPATH=ON # Needed for Ninja build?
-DCMAKE_BUILD_WITH_INSTALL_RPATH=ON

Why this comment? What do you need to know?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think it was copied from some libhipcxx PRs that I was looking at as a reference and I forgot to remove it.

Comment thread math-libs/CMakeLists.txt
rocm-cmake
libhipcxx
RUNTIME_DEPS
amd-llvm

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why does this require amd-llvm as a runtime dep?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The static lib embeds device code and HIP fatbin symbols, so consumers link through amd-llvm's clang++/ld.lld (--hip-link). A host-only linker can't resolve it, so amd-llvm is a real consume-time dep (same as libhipcxx).

Comment thread BUILD_TOPOLOGY.toml Outdated
Comment on lines +645 to +647
# identical across per-arch shards. Built for all targets once (see
# USE_TEST_AMDGPU_TARGETS in math-libs/CMakeLists.txt) and published as a single
# _generic artifact, making per-arch CI upload races harmless (latest write wins).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
# identical across per-arch shards. Built for all targets once (see
# USE_TEST_AMDGPU_TARGETS in math-libs/CMakeLists.txt) and published as a single
# _generic artifact, making per-arch CI upload races harmless (latest write wins).
# identical across per-arch shards. Built for all targets once.

Comment thread math-libs/artifact-hipthreads.toml Outdated
"hipthreads/**/*",
]
exclude = [
# Exclude folders not needed for testing

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
# Exclude folders not needed for testing

That comment is not needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Not ready to Review PR has unresolved policy failures — reviews blocked

Projects

Status: TODO

Development

Successfully merging this pull request may close these issues.

2 participants