Skip to content

fix(clr): Reserve only the descriptor size a batch copy dispatch needs - #11650

Merged
satyanveshd merged 1 commit into
developfrom
users/sdittaka/fixbatchasyncbehavingsync
Sep 28, 2026
Merged

satyanveshd merged 1 commit into
developfrom
users/sdittaka/fixbatchasyncbehavingsync

Conversation

@satyanveshd

@satyanveshd satyanveshd commented Sep 15, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

hipMemcpyBatchAsync call waits on the host side after 16 dispatches causing delays in enqueue.

Technical Details

hipMemcpyBatchAsync API should reserve only the total descriptor size on the kernel arg pool for that dispatch and not the maximum amount of chunk memory that can be allocated each time.

Issue Tracking

JIRA ID: ROCM-30841

Test Plan

Verify with the provided reproducer that hipMemcpyBatchAsync doesn't wait on the host side after 16 dispatches causing dispatch latency.

Test Result

PASS

Submission Checklist

Copilot AI lite review requested due to automatic review settings September 15, 2026 16:28
@satyanveshd
satyanveshd requested a review from a team as a code owner September 15, 2026 16:28
@therock-pr-bot

Copy link
Copy Markdown

✅ All Policy Checks Passed

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/clr/rocclr/device/rocm/rocblit.cpp; no test file found
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

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

🙋 Wish to Override Policy?

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

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

I would remove the comment as code is descriptive enough already.

@jaydeeppatel1111

Copy link
Copy Markdown
Contributor

I would remove the comment as code is descriptive enough already.

Right, How about test to be added?, JIRA has already provided reproducer.

@satyanveshd

Copy link
Copy Markdown
Contributor Author

I would remove the comment as code is descriptive enough already.

Right, How about test to be added?, JIRA has already provided reproducer.

This is not a functional issue. It is increasing the enqueue time of hipMemcpyBatchAsync API if more than 16 dispatches are enqueued after a long kernel in the same stream. This can affect performance but doesn't break anything as such. Hence there is no new unit test required to be added for this specific scenario.

@jaydeeppatel1111

jaydeeppatel1111 commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

I would remove the comment as code is descriptive enough already.

Right, How about test to be added?, JIRA has already provided reproducer.

This is not a functional issue. It is increasing the enqueue time of hipMemcpyBatchAsync API if more than 16 dispatches are enqueued after a long kernel in the same stream. This can affect performance but doesn't break anything as such. Hence there is no new unit test required to be added for this specific scenario.

Thanks — fix looks right, not blocking.

Small nuance: hipMemcpyBatchAsync is an async enqueue, so blocking the host until the in-flight kernel finishes (~80 ms in the repro) is a behavior deviation, with perf as the visible symptom. hipMemcpyAsync doesn't do this, and 7.14 didn't either.

Also, nothing in hipMemcpyBatchAsync.cc catches it today — those cases verify bytes after sync and never wrap the 16 kernarg chunks. A small case there (long kernel, 17 count==1 calls, assert enqueue isn't ~kernel duration) would cover it but I let you decide.

Thanks!

@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)

@satyanveshd
satyanveshd force-pushed the users/sdittaka/fixbatchasyncbehavingsync branch from 885cf08 to db825af Compare September 28, 2026 08:19
@satyanveshd
satyanveshd merged commit 9451b7a into develop Sep 28, 2026
400 of 404 checks passed
@satyanveshd
satyanveshd deleted the users/sdittaka/fixbatchasyncbehavingsync branch September 28, 2026 12:24
@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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants