Repository navigation
fix(hrr): materialise a bypassed segment on the memcpy path - #10634
Conversation
|
The new code materializes dst_rec unconditionally at the top of replay_memcpy_impl(): That is correct for H2D and D2D device destinations, but for D2H/H2H the destination is a host pointer. Unlike the old translate_ptr() lookup, translate_or_materialize() has side effects: if ctx.regions_enabled and the host pointer value happens to fall inside an annotated SEGMENT range, replay can incorrectly create/register a materialized device allocation and skew alloc_map / Region segs accounting. Suggested fix: only call translate_or_materialize() inside branches where the operand is known to be a device pointer: H2D dst, D2D dst/src, and D2H src. |
Region materialisation was wired into kernel-argument translation only. replay_memcpy_impl called translate_ptr directly, so a sidecar-declared allocation whose first touch was a captured H2D, D2H or D2D copy failed as unmapped in the memcpy handler before materialisation could run. That contradicted the promise made above advance_to in dispatch_event, which says the region timeline is advanced there so memcpys and every other handler see the same view. The advance was shared; the materialisation was not. The three translate_ptr sites now go through translate_or_materialize, which mirrors what the whole-pointer kernel-argument path already does: translate, and on failure back the declared segment and translate into it. The regression test needs memory that genuinely exists while being genuinely invisible to HRR, because a memcpy has to succeed at capture for the event to be recorded at all. The fabricated address the other bypass tests use cannot do that, so the workload allocates from an HSA coarse-grained pool: real and GPU-reachable, but never across the HIP dispatch table, so nothing in the archive can explain it. Access is granted to every agent, since the copy is serviced by a DMA engine that faults on an unshared coarse-grained allocation. Verified failing before the fix: the replay loaded the sidecar, reported "H2D dst not mapped", aborted, and never printed a Region segs line. This is the first test under catch to include HSA headers, so it adds an optional hsa-runtime64 link. Where the package is absent the workload and its test compile out and the test reports itself skipped. Co-authored-by: Cursor <cursoragent@cursor.com>
…on/hrr-regions-memcpy
… into wt-10634 Signed-off-by: Daniel Olsson <Daniel.Olsson@amd.com>
…/wsl Signed-off-by: Daniel Olsson <Daniel.Olsson@amd.com>
…on/hrr-regions-memcpy Signed-off-by: Daniel Olsson <Daniel.Olsson@amd.com>
… into wt-10634 Signed-off-by: Daniel Olsson <Daniel.Olsson@amd.com>
Signed-off-by: Daniel Olsson <Daniel.Olsson@amd.com>
…on/hrr-regions-memcpy
… into users/danielbolsson/hrr-regions-memcpy
There was a problem hiding this comment.
🟡 Changes recommended
The new HSA test helper leaves HSA runtime initialization unbalanced and a new build comment is misleading about “runtime skip” behavior when HSA is absent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a replay correctness gap in HRR where bypassed-region materialisation was only triggered via kernel-argument translation, causing captured memcpys (H2D/D2H/D2D) to fail when the first touch of a sidecar-declared segment was a copy rather than a launch.
Changes:
- Route memcpy pointer translation through a new
translate_or_materializehelper so sidecar segments can be backed on-demand during memcpy replay. - Add an HSA-backed regression test that exercises “memcpy first touch” on a real device-accessible allocation that HIP/HRR never observed.
- Document the broader “fails to translate at point-of-use” materialisation rule in the HRR design notes and register the new tests.
File summaries
| File | Description |
|---|---|
| projects/hip-tests/catch/unit/hrr/hrr_regions_test.cc | Adds an HSA-backed hidden workload and a regression test covering memcpy-first-touch segment materialisation. |
| projects/hip-tests/catch/unit/hrr/CMakeLists.txt | Optionally links hsa-runtime64 and enables the HSA-based HRR region test when available. |
| projects/hip-tests/catch/config/configs/unit/hrr.yaml | Registers the new direct workload and the memcpy-first-touch materialisation test. |
| projects/clr/hipamd/src/hrr/playback/hip_playback.cpp | Adds translate_or_materialize and uses it for memcpy replay pointer translation. |
| projects/clr/hipamd/src/hrr/DESIGN.md | Updates the design doc to reflect materialisation being triggered by translation failures at any use site (including memcpys). |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Follows the HRR migration renames from #11157: the memcpy-path segment materialisation test moves with hrr_regions_test.cc into projects/hrr/tests/integration/, and its added cases are ported to the standalone harness (HRR_HIP_CHECK, hrr::test::SpawnProc). The hip-tests smoke sentinel (catch/config/configs/unit/hrr.yaml and catch/unit/hrr/CMakeLists.txt) is left byte-identical to #11157.
… into users/danielbolsson/hrr-regions-memcpy Carries develop, now containing the merged HRR standalone-project layout (#11157), up the stack from the parent branch. No conflicts.
… into users/danielbolsson/hrr-regions-memcpy Carries develop down the stack from the parent branch, picking up #10454. No conflicts; the generator reproduces the checked-in artifacts unchanged.
…gions-memcpy Base-first cascade: each child is merged only after its parent is pushed and confirmed, so no child's diff is ever computed against a base lacking the parent's tip. Merge rather than rebase -- these branches carry published approvals and rewriting them would dismiss the reviews. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… into users/danielbolsson/hrr-regions-memcpy
… into users/danielbolsson/hrr-regions-memcpy
… into users/danielbolsson/hrr-regions-memcpy
Raw fprintf(stderr, ...) is not permitted in clr: it bypasses the AMD_LOG_LEVEL machinery and writes unconditionally from inside a library that other processes embed. mark_incomplete() is the last such call site here. Convert it to LogPrintfError from utils/debug.hpp, which the file already includes. Error level rather than warning: once this fires the archive is not faithful and replay must not treat it as one. The trailing newline is dropped from the format string because amd::log_printf appends its own. Signed-off-by: Daniel Olsson <Daniel.Olsson@amd.com>
…n/hrr-regions-memcpy
…ogging" This reverts commit a909957.
TheRock Submodule Bump ActivityNewest first
|
Motivation
Addresses @LeoZhao-BJ's review comment on #10225.
Region materialisation was wired into kernel-argument translation only.
replay_memcpy_implcalledtranslate_ptrdirectly, so a sidecar-declared allocation whose first touch was a captured H2D, D2H or D2D copy failed as unmapped in the memcpy handler before materialisation could run.His "breaks the stated behavior" framing is right, and the promise being broken is our own. The comment above
advance_toindispatch_eventsays the region timeline is advanced there so that memcpys and every other handler get the same view. The advance was shared; the materialisation was not.Technical Details
The three
translate_ptrcall sites inreplay_memcpy_implnow go through a smalltranslate_or_materializehelper, which mirrors what the whole-pointer kernel-argument path already does: translate, and on failure back the declared segment and translate into it.The regression test needed real HSA memory
Every other bypassed-allocation test in this file fakes the address — the workload passes
probe + kProbeFarOff, which nothing allocated, and no thread dereferences it. That cannot be reused here: a memcpy has to succeed at capture for the event to be recorded at all. The memory has to genuinely exist while genuinely being invisible to HRR.So the new hidden workload allocates from an HSA coarse-grained device pool. The buffer is real and GPU-reachable but never crosses the HIP dispatch table, so nothing in the archive can explain the address — exactly the property the assertion needs. Two details worth knowing:
Because an HSA address appears in no
hipMallocrecord,find_alloc_basecannot recover it, so the child reports the base on stdout and the parent builds the sidecar from that.This is the first test under
catchto include HSA headers, so it adds an optionalhsa-runtime64link. Where the package is absent, the workload and its test compile out and the test reports itself skipped rather than failing an otherwise fine build.Issue Tracking
AIRUNTIME-2700
Test Plan
The test is only worth anything if it fails without the fix, so that was verified explicitly rather than assumed: the fix was stashed, the suite rebuilt, and the new test run against the unfixed memcpy path.
Test Result
Without the fix —
Unit_HRR_Regions_MemcpyFirstTouchMaterializationfails. The sidecar loads and is visible in the log, but the memcpy handler never consults it:No
Region segsline is printed at all.With the fix — the test passes:
Region segs : 1 materialised, replay exits 0, and the D2H blob matches byte for byte.Full suite: 78/78 passed, 0 failed. GPU-free pass PASS, GPU pass PASS. All five region tests green, including the new one. One skip (
Unit_HRR_MemcpyPeerRoundtrip, needs a second device).Submission Checklist
Made with Cursor