Skip to content

perf(clr): fill with wide copies in HostBlitManager::fillBuffer - #12629

Merged
iassiour merged 2 commits into
developfrom
users/iassiour/clr-host-fill-wide-copies
Oct 8, 2026
Merged

iassiour merged 2 commits into
developfrom
users/iassiour/clr-host-fill-wide-copies

Conversation

@iassiour

@iassiour iassiour commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

HostBlitManager::fillBuffer is slow for large fills because it calls memcpy() once per pattern element. The host fill path is taken when the destination is directly accessible from the host, for example:

  • HIP on Windows: hipMemset/hipMemsetAsync on hipHostMalloc memory.
  • OpenCL: clEnqueueFillBuffer on host-accessible buffers.

Technical Details

The pattern size is a runtime value, so each loop iteration is a real libc memcpy() call that copies a few bytes. The per-call overhead dominates, and fill time scales with the element count rather than the byte count. A 64 MiB hipMemset makes about 67M calls.

The fill now uses wide copies:

  • 1-byte patterns use a single memset().
  • Wider patterns are replicated into a 4 KiB stack staging block, which is then copied out in block-sized memcpy() calls. The block is built by doubling and capped at the fill size, so small fills stay cheap.
  • Fill patterns are capped at FillMemoryCommand::MaxFillPatterSize (128 bytes), so a pattern always fits in the staging block. A static_assert enforces this.
  • The destination is only ever written, never read back, since it may be write-combined memory.

The result of the fill is unchanged, including the existing behaviour when the size is not a multiple of the pattern size.

Issue Tracking

JIRA ID: ROCM-32043

Test Plan

  • Standalone host microbenchmark comparing the old and new fill loops on regular host memory, for 1/2/4-byte patterns and fills from 4 B to 1 MiB.
  • Byte-for-byte comparison against the old loop for every pattern size from 1 to 128 bytes, across 25 fill sizes from 0 to 1 MiB (including sizes that are not a multiple of the pattern), with an unaligned destination and guard bytes checked for overruns.
  • hip-tests memset tests, using a local build in which HIP 1D memsets take the host fill path.

Test Result

Microbenchmark, time per fill (old loop -> new):

Pattern 64 B 4 KiB 64 KiB 1 MiB
1 byte 84 ns -> 2 ns 4.9 us -> 24 ns 78 us -> 0.3 us 1.25 ms -> 5 us
2 bytes 46 ns -> 20 ns 2.5 us -> 66 ns 39 us -> 0.43 us 627 us -> 11.5 us
4 bytes 22 ns -> 16 ns 1.3 us -> 60 ns 21 us -> 0.42 us 333 us -> 11.4 us

The output matched the old loop in all 3,200 comparison cases.

All 41 positive 1D memset tests in MemoryTest1/MemoryTest2 pass with the final code (D8/D16/D32, sync and async, unaligned, small and zero size). The 2D/3D memset, pitch and graph-capture memset tests also passed on the first commit; the second commit only restructures the staging loop.

Not tested on Windows or with OpenCL.

Submission Checklist

🤖 Generated with Claude Code

HostBlitManager::fillBuffer called memcpy() once per pattern element.
The pattern size is a runtime value, so every iteration is a real libc
call that copies 1-4 bytes, and the per-call overhead dominates: a
64 MiB hipMemset made 67M calls. The time scaled with the element
count rather than the byte count.

Changes:
- 1-byte patterns use a single memset().
- Wider patterns are replicated into a 4 KiB stack staging block,
  built by doubling and capped at the fill size, which is then copied
  out in block-sized memcpy() calls.
- The destination is only ever written, never read back, since it may
  be write-combined memory.

The fill result is unchanged, including the existing handling of a
size that is not a multiple of the pattern size.

Co-Authored-By: Claude <noreply@anthropic.com>
@therock-pr-bot

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

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/blit.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?

@therock-pr-bot

therock-pr-bot Bot commented Oct 1, 2026

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.

Fill patterns are capped at FillMemoryCommand::MaxFillPatterSize (128
bytes), so a pattern always fits in the 4 KiB staging block. Drop the
fallback that copied straight from the pattern for larger patterns and
assert the bound at compile time instead.

Changes:
- static_assert that MaxFillPatterSize fits in the staging block
- Always copy out of the staging block; remove the conditional source
- Shorten the doubling and copy-out loops

Output is byte-identical to the per-element loop for every pattern size
from 1 to 128 bytes across 25 fill sizes (0 to 1 MiB, odd sizes,
unaligned destination, guard bytes checked). Speed is unchanged within
noise. HIP 1D memset tests pass with a build where they take the host
fill path.

Co-Authored-By: Claude <noreply@anthropic.com>
@iassiour
iassiour marked this pull request as ready for review October 2, 2026 01:22
@iassiour
iassiour requested a review from a team as a code owner October 2, 2026 01:22
Copilot AI balanced review requested due to automatic review settings October 2, 2026 01:22

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 review overview

🟢 Approval recommended

The bounded staging algorithm preserves existing semantics and avoids reading write-combined destinations.

Review effort: Balanced
Findings: None

What changed in this PR

Optimizes host-accessible buffer fills by reducing small memcpy() calls.

Changes:

  • Uses memset() for single-byte patterns.
  • Replicates wider patterns into a 4 KiB staging block for bulk copies.
  • Preserves existing partial-pattern behavior.
File Description
projects/​clr/​rocclr/​device/​blit.cpp Implements wide-copy host buffer filling.

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

@mangupta

mangupta commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

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

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

@iassiour
iassiour merged commit d1e287a into develop Oct 8, 2026
1104 of 1132 checks passed
@iassiour
iassiour deleted the users/iassiour/clr-host-fill-wide-copies branch October 8, 2026 21:02
systems-assistant Bot pushed a commit to ROCm/clr that referenced this pull request Oct 8, 2026
 HostBlitManager::fillBuffer (#12629)
[rocm-systems] ROCm/rocm-systems#12629 (commit d1e287a)
@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.

4 participants