fix(hrr): replay pitched host copies over exactly the copied rect by andraghetti · Pull Request #12521 · ROCm/rocm-systems

andraghetti · 2026-09-30T11:12:59Z

Motivation

Replay of pitched host copies ignored pitch and offsets in two ways.

Device-to-host validation for the driver memcpy copies and the hipMemcpy3D family compared the first width*height*depth bytes of the device allocation with the first width*height*depth bytes of the host buffer. A pitched copy could therefore fail a correct replay (padding inside the window) or pass a broken one (later rows never compared). hipMemcpy2D compared the padding between rows, which fails a correct replay whenever the host padding at capture was not zero.
hipMemcpy3D host-to-device replay handed a flat width*height*depth source blob to the runtime with the recorded pitch and position, so the copy read past the end of the blob. With no blob, it copied from the capture-time host address.

Technical Details

Capture records the host side of a pitched copy as its copied rows only, packed end to end: width*height*depth bytes, whatever the pitch. Capture cost follows the bytes copied, not the distance between rows.
- It covers the D2H expected output of hipMemcpy2D, the hipMemcpy3D family, hipDrvMemcpy3D/Async, hipDrvMemcpy2DUnaligned and hipMemcpyParam2D/Async, plus the H2D sources of the same copies.
- events.bin marks such an archive with HRR_FILE_FLAG_PACKED_HOST_RECTS in the reserved field of its file header.
- A hipMemcpy3D the runtime rejected records neither blob.
D2H replay reissues the recorded copy with the host destination swapped for a scratch buffer laid out like the blob, then compares only the copied rows. In a flagged archive the host side is dense: pitch equal to width, no offsets. One helper, replay_pitched_d2h, serves all of these plus hipMemcpy2D. The geometry read from the archive is overflow-checked.
hipMemcpy3D H2D replay goes through the same size guard as the driver copies. It substitutes the blob only when it spans the recorded source rect and otherwise skips the copy.
HRR_VERSION goes from 6 to 7. ci(hrr): add HRR test and tooling support for CI #12778 also needs 7 and picks it up from develop. An archive without the flag still replays with the recorded pitch and offsets. Its flat blob is smaller than that span: its D2H check is reported as not validated and its H2D copy is skipped. A dense copy that starts at the base has the same layout in both and is still compared.
New hrr_pitched_d2h_test.cc:
- Seven pitched D2H copies with pitch strictly greater than width and non-zero offsets on both sides, with a roundtrip and blob edits for hipDrvMemcpy3D, hipMemcpy3D and hipMemcpy2D.
- Pitched hipMemcpy3D and hipMemcpy3DAsync H2D copies, with a roundtrip and a short-blob case.
- A hipMemcpy2D H2D and D2H pair with rows 16 MiB apart, whose blobs must stay at 192 bytes.

Issue Tracking

JIRA ID: AIRUNTIME-2564

Test Plan

gbt350 (gfx950, GPU 5), CLR built with HIP_HRR_CAPTURE_ENABLED: HRR unit tests and the full hrr-integration-tests binary.
The four new roundtrips under HIP_HRR_D2H_EXACT=1.
Negative controls:
- The D2H tests on develop without the fix.
- The H2D tests with the H2D commit reverted.
- The H2D tests with the new capture and the old replay.

Test Result

Unit tests pass. The integration suite passes: 760 assertions in 58 test cases.
D2H: an edit to the second or the last copied row fails exactly that check. Without the fix, an unedited replay gives 0 pass, 7 fail.
H2D: the roundtrip passes and a short source blob is skipped.
- Without the fix, capture records the first 288 bytes from the base pointer instead of the 288 copied bytes.
- With the old replay, the short blob is over-read into all 288 bytes of the rect and both readbacks fail.
HRR CI cross-check on users/danielbolsson/hrr-development, run 37304009360: all pitched tests pass. Negative control with the footprint capture, run 37304033885: the sparse-pitch blob is 48 MiB (50331696 bytes) and the pitched tests fail.
Not run in this PR's own CI: no workflow builds capture-enabled CLR yet (#12517 is the draft).

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

Archive format unchanged: an older archive keeps the flat blob, which is shorter than the footprint, so replay reports it as not validated.

Replay read the flat blob with the recorded pitch and position; a short or missing blob now skips the copy instead.

therock-pr-bot · 2026-09-30T11:13:44Z

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	—
🚫 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

Copilot review overview

🟡 Changes recommended

Skipped D2H validations are not consistently counted, allowing some unvalidated archives to pass.

Review effort: Balanced
Findings: 2 · 1

Open (3)

Count overflowing D2H checks as skipped · New
Count short footprint blobs as skipped D2H checks · New
Remove duplicate H2D-source behavior documentation · New

What changed in this PR

Improves HRR replay fidelity for pitched host-memory copies.

Changes:

Captures complete pitched host footprints.
Replays D2H copies into pitched scratch buffers and compares copied rows only.
Adds pitched D2H/H2D integration coverage.

File	Description
`projects/hrr/tests/integration/hrr_pitched_d2h_test.cc`	Adds pitched-copy integration tests.
`projects/hrr/tests/integration/CMakeLists.txt`	Registers the new tests.
`projects/hrr/playback/hip_playback.cpp`	Implements pitched replay and validation.
`projects/hrr/DESIGN.md`	Documents pitched-copy behavior.
`projects/clr/hipamd/src/hrr/hip_capture.cpp`	Captures pitched host footprints.

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

A pitched D2H check whose destination rect overflows size_t, or whose expected blob is shorter than the copy, now counts as attempted and so shows up as skipped in the summary. An archive where every check is skipped then fails the replay instead of passing with nothing validated. Unit_HRR_PitchedD2HShortBlobs cuts one blob, then all seven, and checks the summary and the exit code. Also drops a duplicated bullet in DESIGN.md. Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

Footprint blobs currently archive untouched host padding, risking unintended data exposure or capture-time faults.

Review effort: Balanced
Findings: 1

Open (1)

Avoid archiving untransferred padding and host memory · New

Resolved since last review (3)

Count short footprint blobs as skipped D2H checks
Count overflowing D2H checks as skipped
Remove duplicate H2D-source behavior documentation

mangupta · 2026-10-01T17:12:12Z

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

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

…nt (do not merge)

A pitched host blob spans the host buffer from the copy's base pointer through its last copied byte, so it also covered the bytes before the first copied row and the padding between rows and slices. Those were read and written to the archive although the copy never touched them: they can hold unrelated data, and an unmapped gap such as a guard page between rows made a valid copy fault only when capture was on. The blob keeps the footprint layout, but only the copied rows are read from the host buffer and every other byte is zero. Replay substitutes H2D sources with the recorded pitches and compares D2H checks row by row, so it never reads those bytes. This covers every pitched host blob: hipMemcpy2D, hipMemcpy3D and the driver 2D/3D copies, both directions. The pitched test now salts the device window per copy, since the host fill no longer makes the expected blobs distinct, and checks that the D2H and H2D blobs hold zero around the copied rows. Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Copilot review overview

🟡 Changes recommended

Async D2H capture can snapshot stale data when using a null stream or after synchronization failure.

Review effort: Balanced
Findings: 1

Open (1)

Synchronize null-stream async 3D D2H copies before snapshotting · New

Resolved since last review (1)

Avoid archiving untransferred padding and host memory

hipMemcpy3DAsync, hipMemcpy2DAsync and hipMemcpyParam2DAsync on the null stream are still asynchronous, but the capture skipped the sync for a null stream and could hash the host rows before the copy landed. The driver 3D path already synchronised every async copy; the others now do too. hipMemcpyParam2DAsync also skips the D2H blob when the sync fails, rather than snapshotting anyway. Co-Authored-By: Claude <noreply@anthropic.com>

…nt (do not merge)

hipMemset can return before the fill lands, and the non-blocking stream the async copy runs on does not wait for the null stream, so on some parts the fill overwrote the copied window. Synchronise after it, which replay then repeats too. Compare the buffers through byte_diff(), which names the first differing offset: comparing the vectors made Catch2 print every byte as a char, which is not valid UTF-8 and broke the log reader. Co-Authored-By: Claude <noreply@anthropic.com>

andraghetti · 2026-10-05T07:56:27Z

Done in dd59783: replay_memcpy2d now sends a host-to-device blob through the same span check as the driver copies and hipMemcpy3D, and skips the copy when the blob is shorter than spitch * (height - 1) + width. Unit_HRR_PitchedH2D2DShortBlob cuts the blob one byte short for hipMemcpy2D and hipMemcpy2DAsync and checks the skip; Unit_HRR_PitchedH2D2DRoundtrip covers the whole blob. On a cross-check with hrr-development the new test fails with the check reverted and passes with it.

I'll mark the first criterion on AIRUNTIME-2565 done when this merges.

Copilot

Copilot review overview

🟡 Changes recommended

Footprint-sized blobs can impose unbounded memory, CPU, and storage costs for valid copies with large sparse pitches.

Review effort: Balanced
Findings: 1

Open (1)

Avoid materializing sparse pitch gaps during capture · New

mangupta · 2026-10-05T08:53:35Z

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

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

Adds Unit_HRR_DenseOffsetD2HRoundtrip over a hipDrvMemcpy3D of one row at an offset and a hipMemcpy3D into packed slices after the first. Capture must record the host destination from its base pointer through the last copied byte with zero before the first copied byte, and replay must compare the contiguous run in place at that offset: an edit before it is not reported and an edit of its last byte fails exactly that check. Co-Authored-By: Claude <noreply@anthropic.com>

…nt (do not merge)

…ot merge)

mangupta

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

Copilot

Copilot review overview

🔵 Needs a closer look

The cross-cutting GPU capture/replay changes require final human review, particularly because capture-enabled CI is unavailable.

Review effort: Balanced
Findings: 1

Open (1)

Avoid materializing sparse pitch gaps during capture

A pitched host blob spanned the host rect from its base pointer through the last copied byte, zero between rows. Its cost grew with the pitch, not with the bytes copied: two rows 1 GiB apart allocated, hashed and wrote 1 GiB. Capture now packs the copied rows end to end, width*height*depth bytes, and sets HRR_FILE_FLAG_PACKED_HOST_RECTS in the events.bin header. With the flag, replay gives the host side of each pitched copy a dense layout and keeps the device side as recorded. Archives without the flag replay as before. Unit_HRR_SparsePitchRoundtrip copies four rows 16 MiB apart and requires 192-byte blobs. The existing pitched tests now expect packed blobs, and clear the flag to check that legacy replay skips them. Co-Authored-By: Claude <noreply@anthropic.com>

…nt (do not merge)

… (do not merge)

mangupta · 2026-10-05T12:17:57Z

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

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

Copilot

Copilot review overview

🟡 Changes recommended

The unchanged format version lets older readers accept and dangerously misinterpret the new packed blob layout.

Review effort: Balanced
Findings: 1

Open (1)

Bump HRR_VERSION for incompatible packed archive layout · New

Resolved since last review (1)

Avoid materializing sparse pitch gaps during capture

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

mangupta · 2026-10-06T13:35:44Z

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

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

A v6 reader ignores HRR_FILE_FLAG_PACKED_HOST_RECTS in the file header and replays a packed blob with the recorded pitch, reading past its end. The version is what keeps such a reader out. #12778 also moves to 7 and picks this up from develop.

Copilot

Copilot review overview

🔵 Needs a closer look

The low-level archive-format and pitched-memory changes warrant final human sign-off despite extensive targeted coverage.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Bump HRR_VERSION for incompatible packed archive layout

mangupta · 2026-10-07T08:38:53Z

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

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

systems-assistant · 2026-10-08T14:28:08Z

TheRock Submodule Bump Activity

Newest first

2026-10-08 — Included in TheRock via ROCm/TheRock#8831.

andraghetti added 3 commits September 30, 2026 12:58

fix(hrr): validate pitched D2H copies over the copied rows only

7ab3d4b

Archive format unchanged: an older archive keeps the flat blob, which is shorter than the footprint, so replay reports it as not validated.

test(hrr): cover pitched D2H copies with offsets and blob edits

8407310

fix(hrr): size and guard the hipMemcpy3D H2D source blob

b1be9e3

Replay read the flat blob with the recorded pitch and position; a short or missing blob now skips the copy instead.

Copilot AI balanced review requested due to automatic review settings September 30, 2026 11:13

andraghetti requested review from a team as code owners September 30, 2026 11:13

github-actions Bot added project: clr project: hrr labels Sep 30, 2026

Copilot started reviewing on behalf of andraghetti September 30, 2026 11:13

View session

systems-assistant Bot added the organization: ROCm label Sep 30, 2026

Copilot AI reviewed Sep 30, 2026

View reviewed changes

Comment thread projects/hrr/playback/hip_playback.cpp Resolved

Comment thread projects/hrr/playback/hip_playback.cpp Resolved

Comment thread projects/hrr/DESIGN.md Resolved

mangupta added the lrt-bot: ready label Sep 30, 2026

andraghetti requested a balanced review from Copilot October 1, 2026 16:10

Copilot started reviewing on behalf of andraghetti October 1, 2026 16:12

View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Comment thread projects/hrr/DESIGN.md Resolved

andraghetti added a commit that referenced this pull request Oct 1, 2026

ci: HRR CI cross-check of #12521 on users/danielbolsson/hrr-developme…

accd49e

…nt (do not merge)

andraghetti added a commit that referenced this pull request Oct 1, 2026

ci: expected-case manifests for the #12521 cross-check (do not merge)

6b3d400

andraghetti requested a balanced review from Copilot October 1, 2026 18:17

Copilot started reviewing on behalf of andraghetti October 1, 2026 18:18

View session

Copilot AI reviewed Oct 1, 2026

View reviewed changes

Comment thread projects/clr/hipamd/src/hrr/hip_capture.cpp Resolved

andraghetti added a commit that referenced this pull request Oct 1, 2026

ci: HRR CI cross-check of #12521 on users/danielbolsson/hrr-developme…

ad11675

…nt (do not merge)

andraghetti added a commit that referenced this pull request Oct 1, 2026

ci: expected-case manifests for the #12521 cross-check (do not merge)

aac6336

andraghetti requested a balanced review from Copilot October 1, 2026 18:50

Copilot started reviewing on behalf of andraghetti October 1, 2026 18:52

View session

andraghetti requested a review from mangupta October 5, 2026 07:33

Merge branch 'develop' into users/andraghetti/hrr-pitched-d2h

a9660e7

andraghetti requested a balanced review from Copilot October 5, 2026 07:59

Copilot started reviewing on behalf of andraghetti October 5, 2026 08:00

View session

Copilot AI reviewed Oct 5, 2026

View reviewed changes

Comment thread projects/clr/hipamd/src/hrr/hip_capture.cpp Resolved

andraghetti added a commit that referenced this pull request Oct 5, 2026

ci: HRR CI cross-check of #12521 on users/danielbolsson/hrr-developme…

6211a73

…nt (do not merge)

andraghetti added a commit that referenced this pull request Oct 5, 2026

ci: expected-case manifests for the #12521 cross-check (do not merge)

fd14a3d

andraghetti added a commit that referenced this pull request Oct 5, 2026

ci: negative control, revert #12521 capture and playback change (do n…

1081f06

…ot merge)

mangupta reviewed Oct 5, 2026

View reviewed changes

Comment thread projects/clr/hipamd/src/hrr/hip_capture.cpp Resolved

andraghetti requested a balanced review from Copilot October 5, 2026 10:53

Copilot started reviewing on behalf of andraghetti October 5, 2026 10:54

View session

Copilot AI reviewed Oct 5, 2026

View reviewed changes

andraghetti added a commit that referenced this pull request Oct 5, 2026

ci: HRR CI cross-check of #12521 on users/danielbolsson/hrr-developme…

defd837

…nt (do not merge)

andraghetti added a commit that referenced this pull request Oct 5, 2026

ci: expected-case manifests for the #12521 cross-check (do not merge)

640b1e6

andraghetti added a commit that referenced this pull request Oct 5, 2026

ci: negative control for #12521, capture writes footprint blobs again…

717a571

… (do not merge)

andraghetti requested a balanced review from Copilot October 5, 2026 12:47

Copilot started reviewing on behalf of andraghetti October 5, 2026 12:48

View session

Copilot AI reviewed Oct 5, 2026

View reviewed changes

Comment thread projects/clr/hipamd/src/hrr/hip_capture_writer.cpp Resolved

Merge branch 'develop' into users/andraghetti/hrr-pitched-d2h

be4c959

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

Copilot AI reviewed Oct 7, 2026

View reviewed changes

danielbolsson mentioned this pull request Oct 19, 2026

feat(hrr): make capture metadata-only unless HIP_HRR_CAPTURE_CONTENT is set #13700

Closed

1 task done

Conversation

andraghetti commented Sep 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Issue Tracking

Test Plan

Test Result

Submission Checklist

Uh oh!

therock-pr-bot Bot commented Sep 30, 2026

✅ All Policy Checks Passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

mangupta commented Oct 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

andraghetti commented Oct 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

mangupta commented Oct 5, 2026

Uh oh!

mangupta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

mangupta commented Oct 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

mangupta commented Oct 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

mangupta commented Oct 7, 2026

Uh oh!

systems-assistant Bot commented Oct 8, 2026

TheRock Submodule Bump Activity

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

andraghetti commented Sep 30, 2026 •

edited

Loading