Skip to content

Split off the cooperative queue from the xfer queue - #13665

Merged
litvaOo merged 7 commits into
developfrom
users/b-sumner/split-xfer-queue
Nov 4, 2026
Merged

litvaOo merged 7 commits into
developfrom
users/b-sumner/split-xfer-queue

Conversation

@b-sumner

@b-sumner b-sumner commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Split the ROCm internal device queue into a general transfer queue and a cooperative queue

The single internal VirtualGPU (xferQueue_) served both as the blit/transfer queue (xferMgr() and friends) and as the device's cooperative-launch queue, so it was created as an HSA_QUEUE_TYPE_COOPERATIVE queue whenever HIP coop groups were enabled — even though only the coop-launch path needs cooperative semantics.

This separates the two roles:

  • xferQueue is now non-cooperative (new createInternalXferQueue() helper; createVirtualDevice unchanged). xferMgr() and all transfer callers are unchanged.
  • New coopQueue, created lazily on the first cooperative launch, holds the single per-device cooperative queue. submitKernel's coop path uses dev().coopQueue() instead of dev().xferQueue().

A cooperative HW queue is now allocated only when a cooperative launch actually occurs, rather than for every HIP process. The two queues each own their blit lock; transfers and coop launches are still ordered by the existing CPU-synchronous blit path and barrier-signal handshakes, not by queue sharing.

No functional change for transfers; cooperative-launch semantics (GWS init, barriers, exclusive access) are unchanged beyond the accessor name.

Test Plan

All existing tests should run unchanged

Test Result

Tests I tried ran unchanged

Submission Checklist

@skip-pr-bot

@b-sumner
b-sumner requested a review from a team as a code owner October 16, 2026 19:17
Copilot AI balanced review requested due to automatic review settings October 16, 2026 19:17
@therock-pr-bot

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

Copy link
Copy Markdown

❌ PR Check — Action Required

Check Status Details
📝 PR 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 ⚠️ 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/clr/rocclr/device/device.hpp, projects/clr/rocclr/device/pal/paldevice.cpp, projects/clr/rocclr/device/pal/paldevice.hpp, projects/clr/rocclr/device/rocm/rocdevice.cpp, projects/clr/rocclr/device/rocm/rocdevice.hpp (+1 more); no test file found
🔎 pre-commit ⏳ Pending ⏳ Still running…
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

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

🚫 Please fix the failed policies

  • ❌ PR Description

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.

🙋 Wish to Override Policy?

@therock-pr-bot

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Description

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

@b-sumner

Copy link
Copy Markdown
Contributor Author

This will reduce the exposure of the problem rocgdb has with cooperative queues.

Copilot AI 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.

🟡 Changes recommended

Dynamic queue reclamation can leave the transfer queue without a hardware queue, causing subsequent kernel-backed transfers to crash.

1 open finding
What changed in this PR

Separates ROCm’s internal transfer queue from cooperative launches so cooperative hardware queues are allocated only when needed.

Changes:

  • Creates a non-cooperative internal transfer queue.
  • Adds lazy cooperative-queue creation and cleanup.
  • Routes cooperative kernel launches through the new queue.
File Description
projects/​clr/​rocclr/​device/​rocm/​rocvirtual.cpp Uses the cooperative queue for cooperative launches.
projects/​clr/​rocclr/​device/​rocm/​rocdevice.hpp Declares separate queue state and accessors.
projects/​clr/​rocclr/​device/​rocm/​rocdevice.cpp Implements separate queue creation and cleanup.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@b-sumner
b-sumner force-pushed the users/b-sumner/split-xfer-queue branch from f7d21af to 33f6eaa Compare October 16, 2026 22:50

@mangupta mangupta 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.

🤖 AI-assisted pre-review (for @mangupta)

@mangupta

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (f9c4ffa36a76) — no new findings at or above the posting threshold.

@lumachad lumachad added the ci:run-all-archs Opt-in to building for all architectures on a pull request label Oct 20, 2026
@mangupta

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (4dbf4d38f603) — no new findings at or above the posting threshold.

@mangupta

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (ca96d33c801c) — no new findings at or above the posting threshold.

@mangupta

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (68f37aa7d803) — no new findings at or above the posting threshold.

@mangupta

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (1a6e53cc899f) — no new findings at or above the posting threshold.

@mangupta

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (c6b913f80933) — no new findings at or above the posting threshold.

@mangupta

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (cc30958ec225) — no new findings at or above the posting threshold.

Make the internal xfer queue non-cooperative and add a separate
coopQueue, created lazily on the first cooperative launch. Route both
through createVirtualDevice() via a new cooperative argument, and pin
the non-cooperative xfer queue before create() so its HW queue is not
reclaimed.
@litvaOo
litvaOo force-pushed the users/b-sumner/split-xfer-queue branch from cc30958 to 0ff9414 Compare October 26, 2026 08:27
@mangupta

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (02825b347c78) — no new findings at or above the posting threshold.

@geomin12

Copy link
Copy Markdown
Contributor

hi @b-sumner ! thanks for this PR, i notice you enabled ci:run-all-archs which is great! thanks for using that, we accidentially included some test machines that weren't meant to run. we are fixing it in #14272, so please re-base once this lands!

i noticed those jobs (particularly gfx117X) were massively queued. i cancelled the job!

@b-sumner

b-sumner commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

I have created PR 14396 to eliminate some of the false failures that are showing up here.

@mangupta

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (fbc351579f2c) — no new findings at or above the posting threshold.

@mangupta

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (2876ae46e774) — no new findings at or above the posting threshold.

@mangupta

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (b2c108e09037) — no new findings at or above the posting threshold.

@therock-pr-bot

therock-pr-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown

✅ Author chose to skip pr bot run hence removing label (@skip-pr-bot found in the PR description).

@mangupta

mangupta commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (84fb48fcb693) — no new findings at or above the posting threshold.

@litvaOo

litvaOo commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Errors are unrelated to PR's content, overriding.

@litvaOo
litvaOo merged commit fe3963a into develop Nov 4, 2026
1543 of 1614 checks passed
@litvaOo
litvaOo deleted the users/b-sumner/split-xfer-queue branch November 4, 2026 11:56
@systems-assistant

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

Labels

ci:run-all-archs Opt-in to building for all architectures on a pull request organization: ROCm project: clr

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants