Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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; tests skip that gate through `InstrumentorTestAccess` to exercise the prologue path, either supplying the wrapper themselves (the simulator tests) or only inspecting the planned prologue (the unit tests). Skipping supplies no wrapper, and the prologue planner's own checks still run.

`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,32 @@ 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. This is a
// runtime-support gate: a test may skip it to exercise the prologue path, and
// plan_entry_prologue()'s checks below still run.
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 +1180,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,13 @@ class Instrumentor {
std::vector<InstrumentationPoint> points_;
bool patched_ = false;

// The patch gates refuse points that need runtime support no runtime provides
// yet. Tests skip them through the InstrumentorTestAccess friend to exercise
// the gated path, whether or not they provide that support themselves; the
// planner's checks after the gates still run. Production callers cannot skip.
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
27 changes: 27 additions & 0 deletions emulation/rocjitsu/tests/instrumentor_test_access.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// 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, which refuse points that need
// runtime support no runtime provides yet, such as the kernarg wrapper the entry
// prologue loads through. skip_patch_gates() lets a test exercise the gated
// path. A simulator test may supply that support itself (the harness hands the
// CP the wrapper); a unit test may only inspect the planned prologue and never
// dispatch it. Skipping supplies nothing the gates check for, and the planner's
// own fail-closed checks still run after them.
//
// 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