refactor(dfx): unify collector headers - #1342
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe change centralizes L2 swimlane, dependency-generation, and tensor-dump definitions under common platform headers. A2A3 and A5 headers become forwarding wrappers, while shared AICore and AICPU collector interfaces are introduced. ChangesShared platform profiling and capture
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the platform headers by consolidating duplicate definitions from the a2a3 and a5 platform directories into a unified common/platform directory. Specifically, headers for AICore/AICPU swimlane collectors, dependency generation, profiling, and tensor dumping are moved to the common location, and the platform-specific headers are updated to simply include these common files. This reduces code duplication and improves maintainability across platforms. As there are no review comments provided, I have no feedback to provide on the review itself.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/common/platform/include/common/tensor_dump.h (1)
142-145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify DumpMetaBuffer count placement is intentional; consider matching DepGenBuffer's header-first layout.
DepGenBufferplacesvolatile uint32_t countin the first 64-byte header so the host "copies this alone first to learn count" before reading records.DumpMetaBufferplacescountat the end of the buffer (after all records), forcing the host to seek to the tail to learn how many records are valid. If the host-side dump collector uses a two-phase read (count first, then records), this layout degrades that pattern.If the tail placement is intentional (e.g., the writer fills records then publishes count naturally), please document the rationale. Otherwise, consider moving
countto a 64-byte header at the front for consistency.♻️ Suggested header-first layout (if tail placement is not intentional)
struct DumpMetaBuffer { - TensorDumpRecord records[PLATFORM_DUMP_RECORDS_PER_BUFFER]; - volatile uint32_t count; // Current record count + // Header (first 64 bytes) — host copies this alone first to learn count. + volatile uint32_t count; // Current record count + uint32_t _pad0[15]; // Pad count to 64 B; isolates count's cache line. + + // Records (up to PLATFORM_DUMP_RECORDS_PER_BUFFER) + TensorDumpRecord records[PLATFORM_DUMP_RECORDS_PER_BUFFER]; } __attribute__((aligned(64))); + +static_assert(offsetof(DumpMetaBuffer, records) == 64, "DumpMetaBuffer header must be exactly 64 bytes");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/common/platform/include/common/tensor_dump.h` around lines 142 - 145, Verify the intended host read pattern for DumpMetaBuffer and update the struct accordingly: if the dump collector reads count before records, move volatile uint32_t count into a 64-byte header at the beginning, matching DepGenBuffer’s layout, while preserving the existing record storage and alignment. If tail placement is required by the writer protocol, retain it and document that rationale near DumpMetaBuffer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/common/platform/include/common/l2_swimlane_profiling.h`:
- Around line 662-665: Update calc_perf_data_size_with_phases so the
scheduler-phase allocation always reserves PLATFORM_MAX_AICPU_THREADS
L2SwimlaneAicpuSchedPhasePool entries, matching the fixed stride used by
get_orch_phase_buffer_states(); keep the orchestration-phase sizing based on
num_orch_phase_threads.
In `@src/common/platform/include/common/tensor_dump.h`:
- Around line 104-129: Add compile-time layout checks alongside the existing
structure definitions: assert that DumpReadyQueueEntry is exactly 32 bytes, and
assert that offsetof(TensorDumpRecord, start_offset) equals 64 to preserve the
documented cache-line boundary. Use static_assert with the existing type and
field symbols, without changing the structures’ layout.
---
Nitpick comments:
In `@src/common/platform/include/common/tensor_dump.h`:
- Around line 142-145: Verify the intended host read pattern for DumpMetaBuffer
and update the struct accordingly: if the dump collector reads count before
records, move volatile uint32_t count into a 64-byte header at the beginning,
matching DepGenBuffer’s layout, while preserving the existing record storage and
alignment. If tail placement is required by the writer protocol, retain it and
document that rationale near DumpMetaBuffer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e541ff1d-099c-4068-a42c-b3aba9b10230
📒 Files selected for processing (18)
src/a2a3/platform/include/aicore/l2_swimlane_collector_aicore.hsrc/a2a3/platform/include/aicpu/dep_gen_collector_aicpu.hsrc/a2a3/platform/include/aicpu/l2_swimlane_collector_aicpu.hsrc/a2a3/platform/include/common/dep_gen.hsrc/a2a3/platform/include/common/l2_swimlane_profiling.hsrc/a2a3/platform/include/common/tensor_dump.hsrc/a5/platform/include/aicore/l2_swimlane_collector_aicore.hsrc/a5/platform/include/aicpu/dep_gen_collector_aicpu.hsrc/a5/platform/include/aicpu/l2_swimlane_collector_aicpu.hsrc/a5/platform/include/common/dep_gen.hsrc/a5/platform/include/common/l2_swimlane_profiling.hsrc/a5/platform/include/common/tensor_dump.hsrc/common/platform/include/aicore/l2_swimlane_collector_aicore.hsrc/common/platform/include/aicpu/dep_gen_collector_aicpu.hsrc/common/platform/include/aicpu/l2_swimlane_collector_aicpu.hsrc/common/platform/include/common/dep_gen.hsrc/common/platform/include/common/l2_swimlane_profiling.hsrc/common/platform/include/common/tensor_dump.h
820dc4b to
db58469
Compare
|
Regarding the |
6168f2e to
acea2a4
Compare
|
Doc nit — arch-specific comment in a now-shared header ( This line is unchanged by the diff (it came over verbatim with the a5→common rename), so it does not show up as a changed line — but this PR is what makes the header shared by a2a3 and a5, which turns the comment into arch-specific text living in a common header. The Not a correctness problem (32-bit suffices on both), but per the |
acea2a4 to
16743a5
Compare
Move shared dep_gen, tensor_dump, and l2_swimlane collector headers under src/common/platform/include. Keep the a2a3 and a5 platform include paths as forwarding headers so existing includes continue to resolve while the shared layouts and interfaces have a single source of truth. Add layout offset checks for DumpBufferState and refresh common comments that previously referenced platform-specific names or paths.
2 participants
Summary
Unify the duplicated ArgsDump, dependency-generation, and L2 Swimlane collector headers under
src/common/platform/include.The obsolete a2a3/a5 platform-local headers are removed. Repository call sites already use the logical
common/...,aicpu/..., andaicore/...include paths, and the affected build targets include the common platform directory directly.Changes
src/common/platform/include/{common,aicpu,aicore}.PLATFORM_MAX_AICPU_THREADS.Validation
git diff --checkpython -m pytest examples/a5/tensormap_and_ringbuffer/paged_attention/test_paged_attention.py --platform a5sim --device 0-1 --dump-args 2 --pto-session-timeout 600 -v(1 passed in 21.98s)python -m pytest examples/a2a3/tensormap_and_ringbuffer/paged_attention/test_paged_attention.py --platform a2a3sim --device 0-1 --dump-args 2 --pto-session-timeout 600 -v(1 passed in 21.31s)task-submit --device auto --timeout 1800 --max-time 1800 --run "... --platform a2a3 --device {} --dump-args 2 --pto-session-timeout 600 -v"(1 passed in 18.67s)