You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix(hip): load dispatch table slots atomically in the entry points - #13259
HRR capture installs its shims into the live HIP dispatch table while other threads call HIP. Since #12258 every install and uninstall writes one slot with one atomic store. The HIP entry points in hip_table_interface.cpp still read the slots with ordinary loads. In the C++ memory model an ordinary read that races with an atomic store is a data race, and the behaviour is undefined. This PR gives the read side the matching atomic load.
Technical Details
New header projects/clr/hipamd/src/hip_dispatch_load.hpp with hip::LoadDispatchSlot(). It loads one slot with acquire order. Outside Windows it uses __atomic_load_n. On Windows it uses std::atomic_ref<void*>, the same view the HRR store uses there. amdhip64 is C++20 on Windows, so std::atomic_ref is available.
Why acquire and not relaxed: the HRR store is a release store. A shim reads state that HRR wrote before installing it, such as g_real_table. Acquire pairs with the release store, so a thread that loads the shim also sees that state. A relaxed load would make the slot itself race-free but leave the shim's reads racing. The new TSan test shows both cases.
Cost: an acquire load of a pointer is a plain MOV on x86-64 and LDAR on AArch64. In the built hip_table_interface.cpp.o, hipGetDeviceCount changes from call *0x340(%rax) to mov 0x340(%rax),%rax then call *%rax. There is no fence.
hip_table_interface.cpp gets two macros, HIP_LOAD_DISPATCH_TABLE_FN(fn) and HIP_LOAD_COMPILER_TABLE_FN(fn). All 548 runtime table entry points and all 9 compiler table entry points use them, 557 in total. The tools table has no entry point there.
Most of the diff in hip_table_interface.cpp is mechanical. The runtime call sites are exactly sed -E 's/hip::GetHipDispatchTable\(\)->([A-Za-z0-9_]+_fn)\(/HIP_LOAD_DISPATCH_TABLE_FN(\1)(/' applied to develop. The new prefix has the same width, so continuation lines keep their alignment. Beyond that sed, the file changes in 34 lines: the include, the two macros and the nine compiler table call sites. Without the mechanical lines the PR is about 260 changed lines.
hip_table_interface.cpp is outside src/hrr. It is the entry point shared with rocprofiler, so it needs a review from CLR owners.
The capture shims, the install path and hip_capture.cpp do not change.
AIRUNTIME-2845 moves HRR's slot store into a shared header, hip_dispatch_slot.hpp. Once that lands, LoadDispatchSlot() can move into the same header next to the store.
projects/hrr/DESIGN.md states the contract next to the install path. HRR changes one slot at a time with one atomic store, release or stronger, and never copies a table as a whole. Every entry point loads its slot with an acquire load. It also names the writers that do not follow the contract yet: the CLR profiler and rocprofiler-register tools during ToolsInit().
New workflow .github/workflows/hip-dispatch-slot-loads.yml runs the reader check and the TSan test on ubuntu-24.04 for every pull request that changes hip_table_interface.cpp, hip_dispatch_load.hpp or either test. They need no GPU and no ROCm. No other CI job runs them: TheRock does not build projects/hrr, and HRR's own CI runs hrr-unit-tests through run_catch2.py, not ctest. The regression they catch comes from CLR pull requests that touch hip_table_interface.cpp, so that is where the workflow triggers (43044f1ea3).
Issue Tracking
JIRA ID: AIRUNTIME-2837
Test Plan
hrr-unit-tests-tsan (new). It is built with -fsanitize=thread outside Windows; -DHRR_UNIT_TESTS_TSAN=OFF turns it off for a toolchain without the TSan runtime (bb5230ecc4). Building hrr-unit-tests builds it too. Four reader threads call with hip::LoadDispatchSlot() through one function pointer slot. The test includes only hip_dispatch_load.hpp and links no HIP library. GCC's TSan runtime needs setarch $(uname -m) -R on kernels with vm.mmap_rnd_bits=32; the workflow uses it, and so does the ctest case where setarch works (Docker's default seccomp refuses it, and there the test runs directly) (1c39618970). One writer installs and removes a shim in that slot 2000 times with the release store HRR uses. Before the cycles start, the writer waits until a reader has called the real function and then the shim, so the race runs on any schedule. The shim reads a pointer that the writer set before the first install. TSan exits with code 66 on any report.
hrr-unit-tests-dispatch-readers (new). A CMake script fails if any entry point in hip_table_interface.cpp reads a slot as Get*DispatchTable()->X_fn instead of through the macros, or if either macro's #define does not call hip::LoadDispatchSlot( (550f8e4b49).
The branch has develop merged in. That brought hipKernelSetAttributeForDevice from #13004, which now uses HIP_LOAD_DISPATCH_TABLE_FN too (30b19d7487). The merge of develop at 77796e3e36 brought hipKernelSetCacheConfig from #12628, converted the same way (bbd4ef99a2).
The reader check allows whitespace before -> (1f476af769) and reads a #define continued with \ to its end (550f8e4b49). It also fails when no call site uses the macros. Negative controls on each: a read split before -> fails the check, and so does a file with no macro call sites. A macro split over two lines passes when it loads through hip::LoadDispatchSlot() and fails when it reads ->fn. With these checks removed, the split read and the file with no call sites pass, and the split macro that loads atomically fails (bb5230ecc4).
Negative controls. The TSan test was run 50 times against each of three versions of LoadDispatchSlot(): the acquire load, a relaxed load, and develop's ordinary read. The reader check was run against develop's hip_table_interface.cpp.
A full gate on the same tree with the ordinary read in LoadDispatchSlot() (p2837neg).
hip_dispatch_load.hpp compiled with -Wall -Wextra and run with GCC 11.4 and AMD Clang 23. The default branch was built as C++17. The Windows branch was built as C++20 with _WIN32 forced, against libstdc++'s std::atomic_ref.
HRR unit and integration suites on MI350X (gfx950) with capture compiled into CLR, then every Catch2 case one at a time, hidden cases included. The develop baseline is the same commit, 2d8b4d61b8.
Test Result
Check
With this change
Without it
hrr-unit-tests-tsan, 50 runs
50 pass, no TSan report
ordinary read: 50 of 50 fail, data race on the slot (store_slot against LoadDispatchSlot)
hrr-unit-tests-tsan with a relaxed load, 50 runs
n/a, this change uses acquire
50 of 50 fail, data race on the state the shim reads
hrr-unit-tests-tsan, all threads on one CPU, 50 runs
50 pass
the test without the startup waits: 49 of 50 fail, race not exercised
hrr-unit-tests-dispatch-readers
pass, 557 entry points
develop's file: fails, 557 direct reads
hrr-unit-tests-dispatch-readers, one macro body turned back into a plain ->fn read
fails and names the macro
the check before 550f8e4b49: passes, 556 entry points
hrr-unit-tests-tsan without HIP headers, g++ 11.4 under setarch -R, 20 runs each
acquire: 20 pass
relaxed: 20 of 20 fail; ordinary read: 20 of 20 fail
HRR unit ctest, MI350X
3 of 3 pass
2 of 3 pass; hrr-unit-tests-tsan fails with a data race on the slot
HRR integration ctest, MI350X
pass
pass
Visible Catch2 cases, one at a time
unit 23 of 23, integration 112 of 112 pass
unit 23 of 23, integration 112 of 112 pass
Hidden Catch2 cases, one at a time
105 of 132 pass; the other 27 match develop exactly
105 of 132 pass; the other 27 match develop exactly
hip_dispatch_load.hpp, GCC 11.4 and AMD Clang 23, both branches
compiles with no warnings, pass
n/a
The develop baseline at 2d8b4d61b8 has the same 27 hidden cases that do not pass, with the same status: 23 fail, 2 end in SIGSEGV, 1 in SIGABRT and 1 is skipped. Most are _Direct cases that need their driver's environment, plus FirstMallocRoundtrip and ApiMatrix_T2/T4_Roundtrip. The GPU suites cannot see this race, so they also pass with the ordinary read. Only the TSan test tells the two apart.
Gate after the develop merge, on MI350X: 30b19d7487 against develop at bfffd9e1a4. Build, unit ctest (3 of 3) and integration ctest pass. Unit Catch2 cases 30 of 30 pass, visible integration cases 110 of 113 pass with 3 skipped, and every hidden case has the same status as on develop (106 of 134 pass, the other 28 match). 1f476af769 only changes the check script: the build and unit ctest (3 of 3) pass, the check passes with 556 entry points, and a clang-formatted sample with a read split before -> passes the old regex and fails the new one.
Gate on MI350X for 7b3cc82de7, with develop merged in at 08c6e8f7c9, against develop at the same commit. Build, unit ctest (3 of 3) and integration ctest pass. Every Catch2 case, hidden ones included, has the same status as on develop: unit 78 of 78 pass, visible integration 126 pass and 4 skip, hidden integration 123 of 154 pass and the other 31 match. On the earlier head 43044f1ea3, the same tree with HIP_LOAD_DISPATCH_TABLE_FN turned back into a plain read fails hrr-unit-tests-dispatch-readers ("does not load through hip::LoadDispatchSlot").
Gate on MI350X for bbd4ef99a2, with develop merged in at 77796e3e36, against develop at the same commit. Build, integration ctest and every Catch2 case, hidden ones included, match develop: 26 cases do not pass on both. Unit ctest passes hrr-unit-tests-dispatch-readers and hrr-unit-tests-tsan; hrr-generator-rules fails on develop too (test_generated_files_current).
Windows: TheRock's Windows::release "Stage - Compiler Runtime" job (run 38542826175) compiled hip_table_interface.cpp with MSVC 19.44 and linked amdhip64_7.dll on 30b19d7. The Windows branch of LoadDispatchSlot() is the same std::atomic_ref<void*> view as the contract's release store of a slot on Windows.
Follow-ups, not in this PR:
GetDispatchTableImpl() in hip_api_trace.cpp runs ToolsInit() in the first thread only. rocprofiler-register tools write the table with ordinary stores during that call, while a second thread can already get the table and call through it. That is a separate race between the tool and the entry points.
The CLR profiler still writes the tables with whole-table copies. AIRUNTIME-2845 covers it.
HRR capture installs its shims into the live HipDispatchTable while other
threads call HIP. Since #12258 each install writes one slot with a release
store. The entry points in hip_table_interface.cpp still read the slots
with ordinary loads. In the C++ memory model that is a data race.
Add hip::LoadDispatchSlot in hip_dispatch_load.hpp. It loads one slot with
acquire order: __atomic_load_n outside Windows, std::atomic_ref<void*> on
Windows, the same views the HRR store uses. Acquire pairs with the release
store, so a shim also sees the state its tool wrote before installing it.
On x86-64 the load is a plain MOV. On AArch64 it is LDAR.
Every entry point now calls through HIP_LOAD_DISPATCH_TABLE_FN or
HIP_LOAD_COMPILER_TABLE_FN. The runtime table call sites are a mechanical
rewrite of hip::GetHipDispatchTable()->X_fn( into
HIP_LOAD_DISPATCH_TABLE_FN(X_fn)(. The new prefix has the same width, so
continuation lines keep their alignment. The nine compiler table call
sites are rewrapped by hand.
hrr-unit-tests-tsan is built with -fsanitize=thread when the compiler
supports it. Reader threads call through a real HipDispatchTable slot with
hip::LoadDispatchSlot. A writer installs and removes a shim in that slot
with the release store HRR uses. The shim reads state set before the
first install, as HRR shims read g_real_table. An ordinary read in the
helper makes TSan report a race on the slot. A relaxed load makes it
report a race on the shim state.
hrr-unit-tests-dispatch-readers fails if an entry point in
hip_table_interface.cpp reads its slot without the load macros. Building
hrr-unit-tests builds the TSan binary too, so a plain ctest run of the
unit tests covers both.
DESIGN.md states the contract next to the install path: one atomic store
per slot with release order or stronger, and an acquire load in every
entry point.
The TSan test started its readers and ran the 2000 store cycles right
away. A schedule where no reader ran before the writer finished, or where
readers missed every short shim interval, failed the test with "race was
not exercised" on correct code.
The writer now waits until a reader has called the real function, then
installs the shim and waits until a reader has called it, and only then
runs the cycles. The waits use relaxed loads, so they give TSan no
happens-before edge between writer and readers, and the ordinary-read and
relaxed-load controls still fail. A wait gives up after 60 s and fails the
test.
This describes a guarantee that the current live tables do not satisfy: the CLR profiler still installs and removes both tables with whole-table memcpy operations (projects/clr/hipamd/src/profiler/hip_clr_dispatch_wrappers.cpp:6101-6125), as the PR’s follow-up notes. Scope this contract to HRR’s writer (and record the profiler exception), otherwise the design document incorrectly implies that every writer is race-free.
DESIGN.md said the live tables follow one writer contract. Only HRR's
writer does. The CLR profiler installs and removes its wrappers with a
whole-table memcpy, and rocprofiler-register tools write slots with
ordinary stores during ToolsInit(). Say so next to the contract.
State that the single-slot atomic store contract covers HRR's writer only,
and note that the CLR profiler wrappers still copy whole tables until
AIRUNTIME-2845 lands.
"Scope race-free table contract to HRR writer": fixed in 15ec889. projects/hrr/DESIGN.md now says the single-slot atomic store contract covers HRR's writer only, so the old-or-new guarantee is stated for slots that HRR writes. The paragraph on other writers now says the CLR profiler wrappers in hip_clr_dispatch_wrappers.cpp keep copying whole tables until AIRUNTIME-2845 (#13260) lands. Documentation only; no code changed.
The reason will be displayed to describe this comment to others. Learn more.
Needs one change before merge. I checked that the 557 converted entry points are a pure wrap of the old reads (old and new text identical after mapping the macros), so the conversion itself is correct. But develop now has hipKernelSetAttributeForDevice in hip_table_interface.cpp with a direct hip::GetHipDispatchTable()->..._fn read; once this merges, that entry point is not atomic and hrr-unit-tests-dispatch-readers fails on develop. Merge develop into the branch and convert it.
MSVC ignores -fsanitize=thread with a warning, so the probe passed there and
added a target that needs __atomic_store_n; hrr-unit-tests depends on it.
The ctest case now runs the test under setarch -R when setarch works, so GCC's
TSan runtime does not abort on kernels with vm.mmap_rnd_bits=32. Docker's
seccomp refuses setarch, and there the test runs directly.
The reason will be displayed to describe this comment to others. Learn more.
Re-checked 1c39618 on top of my approval of 29df209: the only change is hrr-unit-tests-tsan skipped on Windows and run under setarch -R where it works, which answers the MSVC probe comment. Nothing blocking.
…tch slot loads
- hip_dispatch_load.hpp: drop the static_assert, which no call site can trip
(every argument is a function-pointer slot), and cut the comment to two lines
pointing at DESIGN.md.
- hrr_dispatch_load_tsan_test.cc: one plain slot instead of a table with an
unread size field; drop the return-value abort (only TSan detects the race)
and the 60 s deadline (the workflow's timeout bounds a hang).
- tests/unit/CMakeLists.txt: an HRR_UNIT_TESTS_TSAN option (ON) replaces the
TSan link probe; HRR_CAPTURE_SRC/.. replaces HRR_HIPAMD_SRC; the setarch note
is written once.
- DESIGN.md and the workflow header: drop text that restates the code, the test
names and the PR status.
AIRUNTIME-2837: scope unchanged; the atomic loads and both checks stay, trimmed.
AIRUNTIME-2845: DESIGN.md no longer names it as the fix for the CLR profiler's memcpy.
The reason will be displayed to describe this comment to others. Learn more.
Re-checked bb5230e on top of my approval of 7b3cc82: the dropped static_assert is unreachable (every argument is a function-pointer slot), the TSan test still races the acquire load against the release store, and HRR_CAPTURE_SRC/.. resolves to hipamd/src for the readers check and the include path. The rest is the develop merge. Nothing blocking.
The reason will be displayed to describe this comment to others. Learn more.
Review — no blocking findings
Review of changes since the review at bbd4ef9, plus the files they touch. The earlier pass was mine, so I could attribute it. I re-read the one changed PR-owned file in full.
Ticket AIRUNTIME-2837 is named but could not be retrieved from here. The Jira tracker tool was not available.
Blocking
None
Non-blocking
None
Decisions for the author
Three CI failures need an owner's call before merge. This is not a defect in the PR. I could not tie any of the three to the PR from logs and diff.
Not tied to the PR:rocsparse-test_quick_suite and rocfft-test_quick_suite on Windows gfx110X both end in ctest Timeout (300 s and 600 s). Nothing in the diff touches those libraries, so they look like runner or timing issues.
Not ruled out:Test rocprofiler-sdk on gfx94X fails validate.py::test_collection_period_trace with "no records were collected inside active collection window(s)". rocprofiler consumes the same HIP dispatch table this PR reads through. A tool that fails to intercept would show up like this.
Why it still looks unrelated: the new loads are acquire loads with the same call semantics, and the earlier GPU gate matched develop. I could not run it to rule the PR out.
Suggested check: re-run the job, or look at whether the same test fails on other PRs against the same develop base.
Positive signals
The develop merge brought in hipExtMemcpyBatchAsync, and it already goes through HIP_LOAD_DISPATCH_TABLE_FN (hip_table_interface.cpp, +13 lines). The hrr-unit-tests-dispatch-readers check and the new HIP dispatch slot loads workflow caught nothing to fix here.
Deployment notes
None
What this covered
The only commit since bbd4ef9 is 81b7789b52, a merge of develop. I diffed bbd4ef99a2…81b7789b52 and read the PR-owned files in full.
hip_table_interface.cpp has exactly one change, the new hipExtMemcpyBatchAsync entry point, which loads its slot through the macro. A search finds no direct Get*DispatchTable()->… reads left outside the macro definitions.
hip_dispatch_load.hpp and .github/workflows/hip-dispatch-slot-loads.yml are unchanged since bbd4ef9.
I skimmed the projects/hrr changes in the merge. They are develop's generator and API-matrix updates, not this PR's work.
The HIP dispatch slot loads check passes on the head commit.
The other develop-sourced changes in hipamd/src, such as the persisting-L2 limit and the table step 39 ABI entry, came from develop and were not reviewed as part of this PR.
Did not run: the full multi-pass fan-out. The delta is a single 13-line, macro-conforming addition plus a develop merge, so I read it directly and spawned no workers. Impact analysis was limited to the new entry point: the ABI enforcement and the table update already exist on develop. I did not retrieve the ticket. I did not reconcile against any review discussion.
The reason will be displayed to describe this comment to others. Learn more.
Nothing blocking: the only change since my last approval is a develop merge, and the new hipExtMemcpyBatchAsync entry in hip_table_interface.cpp goes through HIP_LOAD_DISPATCH_TABLE_FN; the dispatch-slot-loads check passes. The failing rocprofiler-sdk (gfx94X) and Windows rocsparse/rocfft jobs are outside this change (rocprofiler-sdk also fails on #13895, which has no dispatch-table change).
Comments in hip_dispatch_load.hpp and the TSan test named the store as store_slot in hip_capture.cpp. Name the contract instead, the writers' release store of a slot, so they stay true wherever that store lives.
AIRUNTIME-2837
The reason will be displayed to describe this comment to others. Learn more.
Delta is a develop merge (#13895, PR files unchanged) plus a comment-only commit naming the slot-store contract in hip_dispatch_load.hpp and hrr_dispatch_load_tsan_test.cc.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
danielbolsson
left a comment
Motivation
HRR capture installs its shims into the live HIP dispatch table while other threads call HIP. Since #12258 every install and uninstall writes one slot with one atomic store. The HIP entry points in
hip_table_interface.cppstill read the slots with ordinary loads. In the C++ memory model an ordinary read that races with an atomic store is a data race, and the behaviour is undefined. This PR gives the read side the matching atomic load.Technical Details
projects/clr/hipamd/src/hip_dispatch_load.hppwithhip::LoadDispatchSlot(). It loads one slot with acquire order. Outside Windows it uses__atomic_load_n. On Windows it usesstd::atomic_ref<void*>, the same view the HRR store uses there.amdhip64is C++20 on Windows, sostd::atomic_refis available.g_real_table. Acquire pairs with the release store, so a thread that loads the shim also sees that state. A relaxed load would make the slot itself race-free but leave the shim's reads racing. The new TSan test shows both cases.MOVon x86-64 andLDARon AArch64. In the builthip_table_interface.cpp.o,hipGetDeviceCountchanges fromcall *0x340(%rax)tomov 0x340(%rax),%raxthencall *%rax. There is no fence.hip_table_interface.cppgets two macros,HIP_LOAD_DISPATCH_TABLE_FN(fn)andHIP_LOAD_COMPILER_TABLE_FN(fn). All 548 runtime table entry points and all 9 compiler table entry points use them, 557 in total. The tools table has no entry point there.hip_table_interface.cppis mechanical. The runtime call sites are exactlysed -E 's/hip::GetHipDispatchTable\(\)->([A-Za-z0-9_]+_fn)\(/HIP_LOAD_DISPATCH_TABLE_FN(\1)(/'applied to develop. The new prefix has the same width, so continuation lines keep their alignment. Beyond that sed, the file changes in 34 lines: the include, the two macros and the nine compiler table call sites. Without the mechanical lines the PR is about 260 changed lines.hip_table_interface.cppis outsidesrc/hrr. It is the entry point shared with rocprofiler, so it needs a review from CLR owners.hip_capture.cppdo not change.hip_dispatch_slot.hpp. Once that lands,LoadDispatchSlot()can move into the same header next to the store.projects/hrr/DESIGN.mdstates the contract next to the install path. HRR changes one slot at a time with one atomic store, release or stronger, and never copies a table as a whole. Every entry point loads its slot with an acquire load. It also names the writers that do not follow the contract yet: the CLR profiler and rocprofiler-register tools duringToolsInit()..github/workflows/hip-dispatch-slot-loads.ymlruns the reader check and the TSan test on ubuntu-24.04 for every pull request that changeship_table_interface.cpp,hip_dispatch_load.hppor either test. They need no GPU and no ROCm. No other CI job runs them: TheRock does not buildprojects/hrr, and HRR's own CI runshrr-unit-teststhroughrun_catch2.py, not ctest. The regression they catch comes from CLR pull requests that touchhip_table_interface.cpp, so that is where the workflow triggers (43044f1ea3).Issue Tracking
JIRA ID: AIRUNTIME-2837
Test Plan
hrr-unit-tests-tsan(new). It is built with-fsanitize=threadoutside Windows;-DHRR_UNIT_TESTS_TSAN=OFFturns it off for a toolchain without the TSan runtime (bb5230ecc4). Buildinghrr-unit-testsbuilds it too. Four reader threads call withhip::LoadDispatchSlot()through one function pointer slot. The test includes onlyhip_dispatch_load.hppand links no HIP library. GCC's TSan runtime needssetarch $(uname -m) -Ron kernels withvm.mmap_rnd_bits=32; the workflow uses it, and so does the ctest case where setarch works (Docker's default seccomp refuses it, and there the test runs directly) (1c39618970). One writer installs and removes a shim in that slot 2000 times with the release store HRR uses. Before the cycles start, the writer waits until a reader has called the real function and then the shim, so the race runs on any schedule. The shim reads a pointer that the writer set before the first install. TSan exits with code 66 on any report.hrr-unit-tests-dispatch-readers(new). A CMake script fails if any entry point inhip_table_interface.cppreads a slot asGet*DispatchTable()->X_fninstead of through the macros, or if either macro's#definedoes not callhip::LoadDispatchSlot((550f8e4b49).hipKernelSetAttributeForDevicefrom #13004, which now usesHIP_LOAD_DISPATCH_TABLE_FNtoo (30b19d7487). The merge of develop at 77796e3e36 broughthipKernelSetCacheConfigfrom #12628, converted the same way (bbd4ef99a2).->(1f476af769) and reads a#definecontinued with\to its end (550f8e4b49). It also fails when no call site uses the macros. Negative controls on each: a read split before->fails the check, and so does a file with no macro call sites. A macro split over two lines passes when it loads throughhip::LoadDispatchSlot()and fails when it reads->fn. With these checks removed, the split read and the file with no call sites pass, and the split macro that loads atomically fails (bb5230ecc4).LoadDispatchSlot(): the acquire load, a relaxed load, and develop's ordinary read. The reader check was run against develop'ship_table_interface.cpp.LoadDispatchSlot()(p2837neg).hip_dispatch_load.hppcompiled with-Wall -Wextraand run with GCC 11.4 and AMD Clang 23. The default branch was built as C++17. The Windows branch was built as C++20 with_WIN32forced, against libstdc++'sstd::atomic_ref.Test Result
hrr-unit-tests-tsan, 50 runsstore_slotagainstLoadDispatchSlot)hrr-unit-tests-tsanwith a relaxed load, 50 runshrr-unit-tests-tsan, all threads on one CPU, 50 runshrr-unit-tests-dispatch-readershrr-unit-tests-dispatch-readers, one macro body turned back into a plain->fnreadhrr-unit-tests-tsanwithout HIP headers, g++ 11.4 undersetarch -R, 20 runs eachhrr-unit-tests-tsanfails with a data race on the slothip_dispatch_load.hpp, GCC 11.4 and AMD Clang 23, both branchesThe develop baseline at 2d8b4d61b8 has the same 27 hidden cases that do not pass, with the same status: 23 fail, 2 end in SIGSEGV, 1 in SIGABRT and 1 is skipped. Most are
_Directcases that need their driver's environment, plusFirstMallocRoundtripandApiMatrix_T2/T4_Roundtrip. The GPU suites cannot see this race, so they also pass with the ordinary read. Only the TSan test tells the two apart.Gate after the develop merge, on MI350X: 30b19d7487 against develop at bfffd9e1a4. Build, unit ctest (3 of 3) and integration ctest pass. Unit Catch2 cases 30 of 30 pass, visible integration cases 110 of 113 pass with 3 skipped, and every hidden case has the same status as on develop (106 of 134 pass, the other 28 match). 1f476af769 only changes the check script: the build and unit ctest (3 of 3) pass, the check passes with 556 entry points, and a clang-formatted sample with a read split before
->passes the old regex and fails the new one.Gate on MI350X for 7b3cc82de7, with develop merged in at 08c6e8f7c9, against develop at the same commit. Build, unit ctest (3 of 3) and integration ctest pass. Every Catch2 case, hidden ones included, has the same status as on develop: unit 78 of 78 pass, visible integration 126 pass and 4 skip, hidden integration 123 of 154 pass and the other 31 match. On the earlier head 43044f1ea3, the same tree with
HIP_LOAD_DISPATCH_TABLE_FNturned back into a plain read failshrr-unit-tests-dispatch-readers("does not load through hip::LoadDispatchSlot").Gate on MI350X for bbd4ef99a2, with develop merged in at 77796e3e36, against develop at the same commit. Build, integration ctest and every Catch2 case, hidden ones included, match develop: 26 cases do not pass on both. Unit ctest passes
hrr-unit-tests-dispatch-readersandhrr-unit-tests-tsan;hrr-generator-rulesfails on develop too (test_generated_files_current).Windows: TheRock's
Windows::release"Stage - Compiler Runtime" job (run 38542826175) compiledhip_table_interface.cppwith MSVC 19.44 and linkedamdhip64_7.dllon 30b19d7. The Windows branch ofLoadDispatchSlot()is the samestd::atomic_ref<void*>view as the contract's release store of a slot on Windows.Follow-ups, not in this PR:
GetDispatchTableImpl()inhip_api_trace.cpprunsToolsInit()in the first thread only. rocprofiler-register tools write the table with ordinary stores during that call, while a second thread can already get the table and call through it. That is a separate race between the tool and the entry points.Submission Checklist