Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions emulation/rocjitsu/docs/dbi-design.md
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ A point may carry `probe_args`, a list of 32-bit values handed to the probe, eac

**Verification is the live-in analysis, unchanged.** `supplied_registers()` widens to cover the argument VGPRs, and the existing "residual live-in set must be empty" rule then does the work: a declared argument the body reads subtracts away; one it reads but the caller did not declare survives and fails the site by name. Acceptance is "nothing left over" rather than "reads exactly what was declared", so a probe that ignores an argument it was handed is not rejected — wasteful, not unsafe. There is no argument-specific code in the analysis.

**The framework's entry storage can be passed as a value.** `ProbeArgSource::LogBufferPtrLo` / `LogBufferPtrHi` name argument slots filled from `TrampolinePlan::entry_storage_base`, the persistent SGPR pair the kernel-entry prologue loaded the DBI payload pointer into. Unlike the `EXEC` sources, these read a pair that is live across the whole kernel rather than one the envelope just saved, so `plan_probe_call` keeps every dead-register choice off it at every site of such a kernel, not only the ones passing the pointer on. Liveness cannot supply that exclusion: the prologue's write lives in the cave, so the pair reads dead at every anchor. Three things fail the site closed: a plan naming no pair, a pair outside the kernel's ordinary SGPRs, and a probe body that writes the pair (nothing saves and restores it, so the value would be gone for every later site). `s_movrel*` anywhere in the kernel fails the entry prologue closed, since the storage floor is an operand scan that an M0-displaced write can land above. The prologue is not idempotent, so it runs from a stub only dispatch reaches (see *Placement*). `Instrumentor::patch()` refuses every `LogBufferPtr*` argument, because no runtime builds the kernarg wrapper the prologue loads through; `patch_with_debug_summaries()` still plans the prologue, for callers that supply the wrapper themselves.
**The framework's entry storage can be passed as a value.** `ProbeArgSource::LogBufferPtrLo` / `LogBufferPtrHi` name argument slots filled from `TrampolinePlan::entry_storage_base`, the persistent SGPR pair the kernel-entry prologue loaded the DBI payload pointer into. Unlike the `EXEC` sources, these read a pair that is live across the whole kernel rather than one the envelope just saved, so `plan_probe_call` keeps every dead-register choice off it at every site of such a kernel, not only the ones passing the pointer on. Liveness cannot supply that exclusion: the prologue's write lives in the cave, so the pair reads dead at every anchor. Three things fail the site closed: a plan naming no pair, a pair outside the kernel's ordinary SGPRs, and a probe body that writes the pair (nothing saves and restores it, so the value would be gone for every later site). `s_movrel*` anywhere in the kernel fails the entry prologue closed, since the storage floor is an operand scan that an M0-displaced write can land above. The prologue is not idempotent, so it runs from a stub only dispatch reaches (see *Placement*). `Instrumentor::patch()` and `patch_with_debug_summaries()` refuse every `LogBufferPtr*` argument, because no runtime builds the kernarg wrapper the prologue loads through; a test that supplies the wrapper itself skips that patch gate through `InstrumentorTestAccess`.

`kMaxProbeArgVgprs` is 16, an arbitrary cap. The convention fills `v0` upward through `v30` (`v31` is the packed workitem id), so 31 dwords is the most that arrive in registers; the cap sits well inside that because the free-register search would refuse an argument block near 31 long before the convention did, and only single-dword integer arguments have been measured.

Expand Down Expand Up @@ -489,7 +489,7 @@ The offsets the prologue bakes in come from `make_kernarg_extension_layout(desc.

On the success path the instrumentor writes a `.rocjitsu.kernarg` record under variant `kDbiKernargVariantName` (`"dbi"`) with one payload named `kDbiEntryPayloadName` (`"dbi-log-buffer"`), so a runtime can build a conforming wrapper without reproducing the prologue's arithmetic. Records are keyed by `(kernel_name, variant_name)`, which discriminates within one serialized payload rather than across sections: the loader-side reader stops at the first section of this name, so the instrumentor refuses an object that already carries one.

**The patched descriptor's `kernarg_size` is deliberately left at the guest's own size**, unlike DBT, which grows it to the wrapper size when it declares an extension. Nothing here allocates a wrapper either: the record states the contract, and the runtime that honours it is not in this change. Until it lands, both prologue loads address past the end of a real kernarg allocation, so `Instrumentor::patch()` refuses every `LogBufferPtr*` argument. `patch_with_debug_summaries()` still plans the prologue for callers that supply the wrapper themselves, which is how the simulator tests work: the harness hands the CP a wrapper directly.
**The patched descriptor's `kernarg_size` is deliberately left at the guest's own size**, unlike DBT, which grows it to the wrapper size when it declares an extension. Nothing here allocates a wrapper either: the record states the contract, and the runtime that honours it is not in this change. Until it lands, both prologue loads address past the end of a real kernarg allocation, so both patch entry points refuse every `LogBufferPtr*` argument. Tests that supply the wrapper themselves skip that patch gate through `InstrumentorTestAccess`, which is how the simulator tests work: the harness hands the CP a wrapper directly.

`kernarg_segment_ptr_slot` and `CommandProcessor::init_wavefront_regs` walk the user-SGPR enable-bit order independently, and the CP interleaves register writes with its walk so it cannot call the helper. `tests/dbi/kernarg_sgpr_slot_test.cpp` dispatches every combination to pin the agreement; a drift would have the prologue load from an unrelated pair with no fault and every unit test still green.

Expand All @@ -504,7 +504,7 @@ Each aborts the whole patch with its own diagnostic. Those raised while planning
- a code object that already declares a kernarg extension
- a stub out of `s_branch` range of the kernel entry
- any `s_movrel*` in the kernel
- from `patch()`, any `LogBufferPtr*` argument at all
- any `LogBufferPtr*` argument at all, unless a test skips the patch gates
- no aligned run left after excluding the probes' clobbers
- storage that is misaligned, out of SMEM field range, not one contiguous run, or overlapping the kernarg pair
- a wrapper offset past the target's SMEM immediate range
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1051,32 +1051,6 @@ Instrumentor::ValidationResult Instrumentor::validate_points() {
}

InstrumentedCodeObject Instrumentor::patch() {
// The entry prologue loads through a kernarg wrapper that only a dispatch-time
// runtime can build, and no runtime builds one for DBI. Without it both loads
// read past the kernarg allocation and the guest's kernarg pointer is
// overwritten, so patch() refuses any point that would need the prologue.
// patch_with_debug_summaries() still plans it, for callers that supply the
// wrapper themselves.
std::string readers;
for (const InstrumentationPoint &pt : points_) {
if (std::none_of(pt.probe_args.begin(), pt.probe_args.end(),
[](const ProbeArgValue &arg) { return reads_entry_storage(arg.source); }))
continue;
if (readers.find("'" + pt.probe_symbol + "'") != std::string::npos)
continue;
if (!readers.empty())
readers += ", ";
readers += "'" + pt.probe_symbol + "'";
}
if (!patched_ && !readers.empty()) {
patched_ = true;
InstrumentedCodeObject result;
result.errors.emplace_back("a probe reads the framework's entry storage, which needs a kernarg "
"wrapper that no runtime builds yet; requested by " +
readers);
return result;
}

// Slice off the debug summaries; move the base subobject into the return.
auto debug = patch_with_debug_summaries();
return std::move(static_cast<InstrumentedCodeObject &>(debug));
Expand All @@ -1097,6 +1071,31 @@ InstrumentedCodeObjectDebug Instrumentor::patch_with_debug_summaries() {
return result;
}

// The entry prologue loads through a kernarg wrapper that only a dispatch-time
// runtime can build, and no runtime builds one for DBI. Without it both loads
// read past the kernarg allocation and the guest's kernarg pointer is
// overwritten, so any point that would need the prologue is refused unless a
// test that supplies the wrapper skips the patch gates.
if (!skip_patch_gates_) {
std::string readers;
for (const InstrumentationPoint &pt : points_) {
if (std::none_of(pt.probe_args.begin(), pt.probe_args.end(),
[](const ProbeArgValue &arg) { return reads_entry_storage(arg.source); }))
continue;
if (readers.find("'" + pt.probe_symbol + "'") != std::string::npos)
continue;
if (!readers.empty())
readers += ", ";
readers += "'" + pt.probe_symbol + "'";
}
if (!readers.empty()) {
result.errors.emplace_back("a probe reads the framework's entry storage, which needs a "
"kernarg wrapper that no runtime builds yet; requested by " +
readers);
return result;
}
}

ResolvedPoints resolved = resolve_points();
if (!resolved.errors.empty()) {
result.errors = std::move(resolved.errors);
Expand Down Expand Up @@ -1180,7 +1179,7 @@ InstrumentedCodeObjectDebug Instrumentor::patch_with_debug_summaries() {
// The kernel-entry prologue, planned only for kernels whose probes ask for the
// framework's entry storage. It runs before every site because dispatch
// enters at it. The prologue needs a kernarg wrapper that no runtime
// builds yet, which patch() enforces by refusing these probes.
// builds yet, which the patch gate above enforces by refusing these probes.
std::optional<EntryProloguePatch> entry_patch;
std::optional<uint16_t> entry_storage_base;
if (probes_read_entry_storage(resolved.probes)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -455,8 +455,7 @@ class Instrumentor {

/// @brief Same as patch(), plus per-site InstrumentationPatch summaries.
///
/// Unlike patch(), accepts `LogBufferPtr*` arguments, for callers that supply
/// the kernarg wrapper themselves.
/// Refuses the same points patch() does.
///
/// Currently intended for tests and debugging, although could eventually
/// be used to communicate important information for the host (an ordering
Expand All @@ -470,6 +469,12 @@ class Instrumentor {
std::vector<InstrumentationPoint> points_;
bool patched_ = false;

// The patch gates refuse points no runtime can support yet. A test that
// supplies the missing piece itself (e.g. the kernarg wrapper) skips them
// through the InstrumentorTestAccess friend; production callers cannot.
friend struct InstrumentorTestAccess;
bool skip_patch_gates_ = false;
Comment thread
accauble marked this conversation as resolved.
Resolved

// Lazily populated.
std::unique_ptr<Decoder> decoder_;
std::vector<std::unique_ptr<BasicBlock>> blocks_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
/// nothing here exercises a non-NULL value. Both need hardware.

#include "../dbi_test_util.h"
#include "../instrumentor_test_access.h"
#include "dbi_sim.h"
#include "scalar_builders.h"

Expand Down Expand Up @@ -308,6 +309,8 @@ class DbiEntryPrologueSimBase : public ::testing::Test {
pt.probe_symbol = "rj_test_log_probe";
pt.probe_args = {{ProbeArgSource::LogBufferPtrLo, 0}, {ProbeArgSource::LogBufferPtrHi, 0}};
instr.add_point(pt);
// The harness hands the CP the kernarg wrapper itself.
InstrumentorTestAccess::skip_patch_gates(instr);

auto result = instr.patch_with_debug_summaries();
ASSERT_TRUE(result.errors.empty())
Expand Down
3 changes: 3 additions & 0 deletions emulation/rocjitsu/tests/dbi/dbi_log_record_sim_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
/// the immediate zero.

#include "../dbi_test_util.h"
#include "../instrumentor_test_access.h"
#include "../log_buffer_test_access.h"
#include "dbi_sim.h"
#include "log_record_probe.h"
Expand Down Expand Up @@ -162,6 +163,8 @@ class DbiLogRecordSimBase : public ::testing::Test {
pt.force_full_exec = force_full_exec;
instr.add_point(pt);
}
// The harness hands the CP the kernarg wrapper itself.
InstrumentorTestAccess::skip_patch_gates(instr);

const auto result = instr.patch_with_debug_summaries();
ASSERT_TRUE(result.errors.empty()) << result.errors.front();
Expand Down
21 changes: 21 additions & 0 deletions emulation/rocjitsu/tests/instrumentor_test_access.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// Copyright (c) 2026 Advanced Micro Devices, Inc.
// SPDX-License-Identifier: MIT

/// @file instrumentor_test_access.h
/// @brief Test-only access to the Instrumentor's patch-gate skip.

#pragma once

#include "rocjitsu/code/patch/instrumentor.h"

namespace rocjitsu {

// Both patch entry points run the patch gates. A test that supplies what a gate
// requires, such as the kernarg wrapper the entry prologue loads through, skips
// them here. Must live in namespace rocjitsu (not an anonymous namespace) so it
// names the same type as the friend declaration in Instrumentor.
struct InstrumentorTestAccess {
static void skip_patch_gates(Instrumentor &instr) { instr.skip_patch_gates_ = true; }
};

} // namespace rocjitsu
Loading
Loading