Fix: preserve explicit dependency flags in dep_gen replay - #2057
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (18)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChangesDependency capture now stores per-explicit-dependency Dependency kind capture and replay
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: 🟡 Moderate · up to The change preserves dependency flags but also reduces overflow capacity, which can expose a timeout path that reuses a buffer after it has been published, potentially corrupting dependency metadata used for host replay. Merge should wait for this ownership issue to be fixed or explicitly accepted by the responsible owner. Sequence Diagram(s)sequenceDiagram
participant TaskSubmission
participant AICPUCollector
participant DepGenRecord
participant DepGenReplay
participant DepsJson
TaskSubmission->>AICPUCollector: submit explicit dependency kinds
AICPUCollector->>DepGenRecord: store dependency IDs and kind bytes
DepGenRecord->>DepGenReplay: provide base and overflow records
DepGenReplay->>DepsJson: emit validated, merged edge flags
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The changes satisfy issue Full details: Docstring CoverageExplanation Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 16 files. (2 skipped: 2 unsupported.)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
09f9959 to
a1ce5e6
Compare
|
@coderabbitai Addressed the published-buffer ownership risk in a1ce5e6: once the ready-queue tail advances, a push-gate timeout no longer reports enqueue failure, so callers cannot clear or reuse a buffer already owned by the host. Added a DeviceProfilerEngine regression test for this timeout path. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
doraemonmj
left a comment
doraemonmj
left a comment
There was a problem hiding this comment.
Reviewed against merge-base 89ba2a3f. Verified the wire-layout arithmetic by hand, diffed the a2a3 and a5 trees against each other for parity, and traced each item back to #1827. Two blocking items, both small; no technical objection to the approach.
What this gets right
Not just "it works" — a few things here required actually understanding the subsystem rather than following the issue's Proposed Fix:
- The layout arithmetic holds together. 4736 B,
tensorsat offset 640, andDEP_GEN_OVERFLOW_DEPS_PER_RECORD = 524are mutually consistent (16 + 9N <= 4736), and 640 preserves the original intent that each 128 B tensor blob covers exactly two cache lines instead of straddling three. Thestatic_assertset was updated in step rather than left to rot. - The new
is_trivially_copyable_v/is_standard_layout_vguards close a pre-existing gap on these host/device wire structs — that is required by the codestyle rule for device-copied structs and was missing before this PR. - The OR-dedup handling is the substantive part. Once an explicit edge can be
wait-only,emit_creator's!firstearly-return silently drops the creator'sretain. Introducingexplicit_edge_indexto fold the creator flags back into the explicit edge keeps replay aligned withFaninBuilder's collapse semantics — and it is documented indocs/dfx/dep-gen.mdrather than left as folklore. I checked the.at()on that map: it is reachable only from thefirst == falsebranch, andannot_predsis cleared per record with STEP 1 as its only writer, so it cannot throw today. - a2a3/a5 are strictly symmetric. I diffed the two subtrees' patches against each other; the only differences are
block_numvscore_numand line offsets. - The tests are real. The scene-test cases are
manualonly ona2a3sim, so the new flag assertions genuinely execute inst-onboard-a2a3andst-onboard-a5. Pinning the UT atkDepCount = 64 + 524 + 1anchors the new constant, and the trailing creator+explicit probe covers the dedup path that theexplicit_edge_indexchange exists for.
Blocking
1. docs/dfx/global-backpressure-design.md now contradicts the code.
The "Timeout backstop" section still states:
When it expires the writer breaks to its single failure exit (
return false), and the caller (switch_buffer) accounts the dropped records.
After this change that is only true for the pop side (wait_for_ready_queue_space / wait_for_free_queue_entry). On the push side the timeout is now discarded and nothing is accounted as dropped. Per the repo's doc-consistency rule the fix lands in the same commit as the code change. Splitting the paragraph to distinguish the two sides is enough.
2. return -5 collides with an existing code in the same function.
dep_gen_replay.cpp already returns -5 for write_deps_json() failure (near the end of dep_gen_replay_emit_deps_json). The new invalid-flags path reuses it. Callers in all four device_runner.cpp sites only log dep_gen replay failed (%d), and the header documents codes as "see source", so -5 becomes genuinely ambiguous between "a record carried a corrupt flags byte" and "deps.json could not be written". -7 in both the a2a3 and a5 copies.
Non-blocking, but worth addressing
3. The DFX-engine fix deserves more than one bullet. The commit message does carry Preserve host ownership when a published DFX buffer hits a gate timeout, and the rationale is in this PR's conversation — so this is not an undeclared change. The gap is scope visibility: DeviceProfilerEngine is shared by pmu, scope_stats, l2 phase, l2 AicoreTask, dep_gen and args_dump, so this is a behavior change across six subsystems riding in a PR whose title and body are entirely about dep_gen. The PR body's Summary omits it, and once this squash-merges the conversation context does not follow into git history. A short paragraph in the body — what the bug was, that it affects every DFX module, and that it is independent of #1827 — costs little and is what a future bisect will actually find. Splitting it into its own PR would be cleaner still, since it is independently testable and independently valuable.
4. #1827's third item is dropped silently. The issue also asks to "make the differential gate explicit-kind-aware". Both oracle_preds and annot_preds are still seeded from the same kinds_data[i] in STEP 1, so an explicit-kind divergence remains undetectable by construction — the shared hard-coded constant was replaced by a shared recorded byte. I think this is genuinely infeasible in the current design: the oracle is compute_task_fanin, which does not process explicit deps at all, so there is no independent second source to compare against. That is a fine answer, but it should be stated, because Fixes #1827 will close the issue and take the item with it. Either say so in the body (and ideally in a comment next to STEP 1), or leave a follow-up issue.
5. Rejecting the whole trace over one bad byte is inconsistent with the surrounding code. The same function handles the same class of input — values read back from device shared memory that could be corrupt — by clamping and continuing:
// ...we clamp on read too so a corrupted record never drives an OOB read
if (base_dc > DEP_GEN_MAX_EXPLICIT_DEPS) { LOG_ERROR(...); base_dc = DEP_GEN_MAX_EXPLICIT_DEPS; }The new raw_kind check instead tears down both tensormaps and discards the entire deps.json. Unknown bits cannot cause an OOB read, and this is a DFX artifact rather than a correctness path, so LOG_ERROR plus raw_kind &= kKnownDepFlags (or falling back to WAIT|RETAIN) would match the established posture and still surface the anomaly.
6. The push-gate timeout is now entirely unobservable. A 30 s expiry means "the host is gone" — previously it at least showed up as an inflated dropped count; now there is no counter, no log, no sticky flag. Worth an on_push_gate_timeout trait hook or a one-shot log (not per-iteration — the AICPU hot-path logging rule applies).
Consider
- The scene test maps alternating kinds back to producers via
sorted(barrier_explicit_edges.items()), i.e. it assumesTaskId::rawordering equals submission order. True today for a single ring, but a change in id allocation would surface as a confusing "flags mismatch" rather than an obvious failure. Deriving the producer sequence from the order ofdeps["tasks"]would be sturdier. - No test covers the new rejection path. A host-side case feeding a record with
explicit_dep_kinds[0] = 0xFFis cheap. (If item 5 is adopted, assert the clamped flags instead.) reinterpret_cast<const uint8_t *>(args.explicit_dep_kinds_data())relies onenum DepFlags : uint8_t. Astatic_assert(sizeof(DepFlags) == 1)next to the cast beats making the reader go look it up.- The
WAIT|RETAINdefault is now spelled in three places (Arg::explicit_dep_kind(), both orchestrator call sites, and the UT's bare3u). A named constant would help; at minimum the UT could useDEP_WAIT | DEP_RETAINinstead of3u. - The
flagsrow in thedeps.jsonschema table is markedalways, buthost_build_graph's writer (dep_gen_host_graph.cpp) emits noflagsfield at all. Pre-existing, not introduced here — but since that row is already being rewritten, noting "tensormap_and_ringbuffer only" is nearly free.
Notes
DepGenRecord's layout changed with no version field on the host/device boundary, andstatic_assertonly guards one side at compile time. A partially-rebuilt tree (host runtime on 4672, AICPU on 4736) would produce a plausible-looking but garbagedeps.jsonwith nothing to detect it. Not a defect in this PR — the format never had a version — but worth a line in the body telling reviewers to do a full rebuild rather than an incremental one.- Capacity impact is real but bounded: overflow slots drop 582 -> 524 (-10%), so a submit with e.g.
dc = 600now needs 3 slots where 2 sufficed. The theoretical per-submit ceiling falls from ~595k to ~536k deps, whichCHIP_DEP_LIST_POOL_SIZE(16384) puts far out of reach anyway. Record memory grows 1.37% and only when--enable-dep-genis set, sinceDepGenCollector::init()is gated on it. The audit doc's added sentence is an accurate characterization. - The scene test now makes half the barrier's deps ordering-only, which is a real runtime semantic change and not just an assertion change. Safe here because the producers write the external
ext_Xrather than a runtime-owned output, so there is no lifetime to lose — worth keeping in mind if that kernel is ever reworked to use owned outputs. pto_isa.pinis untouched and no pto-isa header references changed, so no pin bump is implied.
Items 1 and 2 are the only things holding this up; both are a few lines. The core change is well-built and I have no objection to the approach.
|
A few additional review notes after tracing the updated paths:
None of these changes the earlier conclusion on the core implementation. Also, the include-guard conversions to |
- Capture per-dependency flags in base and overflow dep_gen records - Replay explicit edges with runtime WAIT/RETAIN dedup semantics - Reject unknown recorded flag bits with a distinct replay error - Preserve host ownership when a published DFX buffer hits a gate timeout - Cover inline and chained records on a2a3 and a5 and update docs Fixes hw-native-sys#1827
a1ce5e6 to
4725432
Compare
|
@doraemonmj Addressed the blocking items and the follow-up notes:
I kept invalid flags fail-fast rather than masking them. An unknown semantics byte makes the dependency artifact untrustworthy; emitting a plausible graph with altered flags would be more misleading than suppressing the artifact. The new return code keeps that failure distinguishable from JSON I/O errors. I did not add push-gate timeout telemetry in this PR: the engine is shared across six DFX consumers, so choosing a logging/counter contract and ownership should be handled as a separate cross-module change. I also left out Validation completed: all 125 non-hardware C++ tests passed, A2/A3 and A5 simulation dep_gen scenes passed, A2/A3 onboard dep_gen scenes passed, and the full PR pre-commit suite passed. |
3 participants
Summary
DeviceProfilerEngineis shared by PMU, scope stats, L2 phase/task profiling, dep_gen, and args dump; this independent correctness fix was uncovered while investigating [Code Health] dep_gen: DepGenRecord carries no per-dep DepFlags, so explicit add_dep_wait edges are mis-recorded in deps.json #1827.Differential Gate Scope
Explicit dependencies are captured input shared by the oracle and annotated replay passes, so the differential gate cannot independently validate their recorded kind bytes. The A2/A3 and A5 dep_gen chain scene tests validate the capture/replay round trip across inline and overflow records.
DepGenRecordchanges from 4672 to 4736 bytes. Host and device runtime artifacts must be rebuilt together rather than mixed with partial incremental outputs.Testing
test_dep_gen_replay,test_dep_gen_collector_aicpu, andtest_profiler_device_engine.pre-commit run --from-ref upstream/main --to-ref HEAD.Fixes #1827