Skip to content

fix(rocprim): Add proper gfx950 config support, and fix launch_manager wavefront support (legacy) - #13881

Merged
stanleytsang-amd merged 1 commit into
developfrom
users/stanleytsang-amd/fix-gfx950-configs-rocprim-legacy
Oct 17, 2026
Merged

stanleytsang-amd merged 1 commit into
developfrom
users/stanleytsang-amd/fix-gfx950-configs-rocprim-legacy

Conversation

@stanleytsang-amd

@stanleytsang-amd stanleytsang-amd commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Fixes Issue: #13871

Motivation

Legacy `projects/rocprim` has the same bug fixed for hipccl3 in #13251. `rocprim.linking` fails in TheRock CI on gfx950 (https://github.com/ROCm/TheRock/actions/runs/38904910579) with:

`hip_global.cpp: Cannot find Symbol with name: _ZN7rocprim17ROCPRIM_400800_NS6detail17trampoline_kernelI...launch_manager...`

gfx950 config support also needed fixing: it was listed as both CDNA4 and CDNA3, and MI355X was not supported.

Technical Details

`visit_config` resolves size64/size32 for the fallback (dynamic wavefront) target, but `launch_manager::make_launch_plan` ignored it and called `trampoline_kernel<..., Target, LaunchSelector>`, so `TargetWaveSize` fell back to `get_wavefront_size(Target::g)`. For the fallback target that is `dynamic` in the host pass but a concrete size in the gfx950 device pass, so the host looked up a kernel symbol that is not in the code object.

  • `launch_manager` takes a `Wavefront` template parameter and forwards it to `trampoline_kernel`.
  • `visit_config` passes the resolved wavefront explicitly in the fallback branch.
  • Removed gfx950 from `ROCPRIM_IS_CDNA3()`.
  • Added `gpu::mi355x` / `"MI355X"`.

Test Plan

Rerun `rocprim.linking` and the rocPRIM tests on gfx950 in CI. Not built or tested locally.

🤖 Generated with Claude Code

…r wavefront support

launch_manager dropped the wavefront size chosen by visit_config for fallback
configs. The host then instantiated trampoline_kernel with a `dynamic`
wavefront, while the gfx950 code object only contains the size64 instantiation,
giving "Cannot find Symbol" at runtime (rocprim.linking).

Pass the wavefront explicitly to launch_manager in the fallback branch.

Also remove gfx950 from ROCPRIM_IS_CDNA3 (it is CDNA4 only) and add mi355x.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

@amd-jmahovsky amd-jmahovsky 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

@therock-pr-bot

therock-pr-bot Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

⏳ Waiting on Required Checks

⏳ Required checks have not all concluded yet. This bot run is exiting without failing so those checks can report independently.

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/rocprim/rocprim/include/rocprim/config.hpp, projects/rocprim/rocprim/include/rocprim/device/config_types.hpp; no test file found
🔎 pre-commit ⏳ Pending ⏳ Still running…
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

⏳ Required checks are still running or have not started yet.
This PR is not ready for review until they complete.

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

🙋 Wish to Override Policy?

@therock-pr-bot

therock-pr-bot Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

⏳ Waiting for required checks to start or finish:

  • ⏳ pre-commit

@stanleytsang-amd
stanleytsang-amd merged commit 2610106 into develop Oct 17, 2026
158 of 164 checks passed
@stanleytsang-amd
stanleytsang-amd deleted the users/stanleytsang-amd/fix-gfx950-configs-rocprim-legacy branch October 17, 2026 03:33
@assistant-librarian

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants