Repository navigation
[rocjitsu] Race detector: capture instruction encodings instead of caching disassembly - #14223
Conversation
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
aaa8b31 to
ba99b1f
Compare
d8e4503 to
e046619
Compare
0d8424f to
df1b32c
Compare
kuhar
left a comment
There was a problem hiding this comment.
One comment, plus an inline reply
df1b32c to
6c58505
Compare
6c58505 to
869cb36
Compare
kuhar
left a comment
There was a problem hiding this comment.
It would be useful to add a short deferred-disassembly example to the plugin guide (emulation/rocjitsu/docs/plugins.md:157, or under "Adding a new plugin"). Plugins can copy the PC and size()/4 words from raw_encoding() in onAmdgpuBeforeExecuteInstruction, then call Wavefront::disassemble_instruction on the CU execution thread when a report needs text. This would make the new service discoverable alongside the guide's existing lifetime advice.
Please distinguish borrowed encodings and mnemonic views from Instruction::disassemble(), which now returns an owned std::string that callers may safely retain. Capturing bytes lets plugins defer formatting; retaining the returned string is also valid.
869cb36 to
cf629c3
Compare
kuhar
left a comment
There was a problem hiding this comment.
Optional: it would help to spell out the value types at these two capture points, following the style guide's preference when the return type is not visible on the line: const std::string dis in emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/code/rj_code.cpp:341, and const InstructionSnapshot instruction in emulation/rocjitsu/lib/rocjitsu/src/rocjitsu/vm/plugins/race_detector/plugin.cpp:542. This makes the owned disassembly text and captured snapshot easier to recognize without following the helper declarations.
LGTM
Copy encodings from issued instructions into wave traces and detector events, then decode only the records selected for a report. Keep producer bytes for the event lifetime and use wave-local sequences to distinguish repeated PCs. Reports remain correct after code rewrites, trace overwrites, and loss of a code mapping, without a shared disassembly cache or diagnostic memory reads. Remove instruction text memoization and format VOPD slots on demand, adapting the generator and C API. Cover byte ownership, dynamic execution selection, cross-wave event lifetime, and real execution with rewritten backing code.
Remove execution sequences and retain PC-based trace selection, trimming, and annotations. Keep captured bytes in wave traces and pending events so disassembly remains independent of code-memory lifetime. Use the captured instruction address when execution changes the wave PC. Cover the scalar-load/swap-PC race and waited control, preserve the existing trace output, and supply byte records in the shared wait fixtures.
The routed-access hook re-captured the instruction's bytes and fell back to wf.pc when the trace was empty. onAmdgpuBeforeExecuteInstruction has already recorded the executing instruction as the newest trace entry, so take it from there, as the report path does, and fail on an empty trace. Drop explicit default arguments added to registration calls, return trace entries by reference, and document how formatTrace selects rows, what the wave trace holds, and why Wavefront::disassemble_instruction is pure virtual. Explain in the race detector guide how to read a report's instruction trace, and show addresses in its example output.
State the snapshot precondition and validation once for every registration method, and reuse testInstruction() for synthetic events in the race plugin tests.
Define snapshot validity once on InstructionSnapshot and reference it from EventRegistry::add, RaceDetector::allocateEventId, WaveRaceState and formatTrace instead of restating the word-count bounds. Document that findConflict throws std::out_of_range for an unavailable event, and cover formatTrace's invalid-snapshot and disassembly-failure behavior.
Give InstructionSnapshot a constructor that rejects empty or oversized encodings and keep its fields private, so every snapshot in a trace or event holds a complete encoding. Registration and report formatting no longer validate snapshots, and formatTrace compares them with a defaulted equality because unused encoding words are always zero. RingBuffer stores its retained values in a vector that grows to its capacity, so it no longer requires default-constructible elements.
Explain which Instruction data is borrowed during a callback, that Instruction::disassemble() returns an owned string, and how a plugin can copy an instruction's PC and encoding and later format it with Wavefront::disassemble_instruction.
Check that the instruction has an encoding whose size is a positive multiple of four bytes that fits the copy buffer before copying it, and reject anything else at capture as the race detector does. Format text only after an instruction has been captured, so the example never passes an empty span to Wavefront::disassemble_instruction.
Build the race backend's synthetic events with testInstruction() and keep passing kProbeWord, the encoding that reports() decodes.
Name std::string for the C API's disassembly copy and InstructionSnapshot for the race detector's captured instruction, since neither type is visible from the call on that line.
aadebdb to
84669b9
Compare
rocjitsu post-commit fuzz passedThe 32-worker multiplexer campaigns for commit
|
TheRock Submodule Bump ActivityNewest first
|
When the race detector plugin reports a race, it prints the instructions that the reporting wave executed between the conflicting memory instruction and the racing access. To produce that listing, each wave kept a ring buffer of its last 256 PCs, and each dispatch kept a shared table that mapped every PC executed by any of its waves to disassembly text. Every wave took that table's mutex on every issued instruction, even when the PC was already present. Profiling hipblaslt-bench GEMMs under multithreaded emulation showed this lock as a bottleneck, although those runs reported no races at all.
This PR removes the shared table. Each trace entry now holds the instruction's PC and a copy of its encoding, and each pending memory event keeps a copy of the instruction that produced it:
Recording an instruction is now a copy of at most 16 bytes into wave-local storage, with no lock or hash lookup. Disassembly happens only when a report is printed: the plugin passes the captured bytes to the CU's target decoder through a new
Wavefront::disassemble_instruction. That call is virtual because standalone plugin modules do not link the simulator. Because reports decode the bytes that actually executed, they remain correct when the code is overwritten or unmapped after execution, and when the producer of a pending event has already left the 256-entry trace. The old table kept the first text it saw at each PC.The report layout and elision limits do not change. Markers and trace boundaries match both the PC and captured encoding, and each printed trace row is disassembled from its own bytes. This distinguishes rewritten instructions at the same PC while preserving selection for identical encodings. The "(before trace window)" label means no matching producer appears in the reporting wave's trace; it does not establish how old the conflict is, since another wave may have executed an instruction the reporting wave never executed. Distinguishing repeated executions of identical bytes at the same PC, and giving each wave in a cross-wave race its own history, are left for follow-up work.
Related changes:
EventRegistry,RaceDetectorandWaveRaceStatetakes anInstructionSnapshotin place of a PC, and the event's PC comes from the snapshot. The snapshot's constructor rejects an empty or oversized encoding withstd::logic_error, so every recorded event can be disassembled.wf.pc.s_swappc_b64updates the PC before it writes its return-address registers, so a pending scalar load into those registers was previously reported at an address derived from the branch target instead of at thes_swappc_b64.std::logic_errorinstead of printing placeholder text. Every recorded instruction decoded successfully when it executed, so either failure indicates a bug in recording or decoding.Instructionno longer caches its disassembly string, and VOPD formats its two slots when asked (generator and generated sources updated). The C API still copies disassembly into the caller's buffer.Memory changes:
Instructionshrinks from 224 to 192 bytes, while each wave's trace grows from 2,064 bytes to as much as 8,192 bytes, allocated as the wave issues its first 256 instructions, and each pending event grows by 24 bytes.Performance
Median process wall time of three interleaved hipblaslt-bench runs on emulated gfx950, comparing develop with this PR rebased onto it (Release, race detection enabled, one GEMM iteration, no cold iterations, no async helper threads, no races reported):
The parallel cases improve by 1.71× and 2.02× because the lock is gone, and process CPU time falls from 129 s to 41 s and from 531 s to 85 s. With one thread there is no contention, and the difference is within run-to-run variation. Report formatting time is not measured.
Alternatives considered
Testing
New tests overwrite a load's code with
s_nopafter it executes and check that the report still shows the executedds_read_b32, including after the load has left the trace window. Repeated-PC regressions verify each row's captured bytes, marker placement and trace boundaries when producer or consumer encodings change. Other tests cover the scalar-load/s_swappc_b64race and its waited control, literal and maximum-width encodings, decoding with no code mapping, concurrent decoding on several CUs, event registration through every method, snapshot construction limits and equality, trace wraparound, and the failure path for missing history. The race detector, trace formatting, event registry, shared wait, disassembly and standalone plugin ABI checks pass locally, and all HIP race corpus cases on gfx950 and gfx1151 pass in local and daemon modes.ISSUE ID: #13640, #14172 (alternative proposals)