Refactor: consolidate per-task markers into one TaskAttrs byte (a2a3/hbg)#1441
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughTask scheduling metadata is consolidated into a packed ChangesTaskAttrs scheduling metadata migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant PTO2OrchestratorState
participant PTO2TaskSlotState
participant PTO2Scheduler
participant AICPUTaskTiming
PTO2OrchestratorState->>PTO2TaskSlotState: prepare task with TaskAttrs
PTO2TaskSlotState->>PTO2Scheduler: route using TaskAttrs
PTO2Scheduler->>PTO2Scheduler: evaluate predicate, sync-start, and early dispatch
PTO2Scheduler->>AICPUTaskTiming: finish timed task using timing slot
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/a2a3/runtime/host_build_graph/runtime/pto_runtime2_types.h (1)
206-220: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider restoring an
offsetofcheck alongside thesizeofassertion.The comment explicitly states the scheduler/shared-memory ABI depends on
packed_buffer_base's offset staying fixed, but onlysizeof(PTO2TaskDescriptor) == 40is asserted now.sizeofalone doesn't lock down wherepacked_buffer_baselands within the struct — a future insertion elsewhere in the struct that happens to preserve total size (e.g., another compensating pad removal) would pass this assertion while silently moving the offset the ABI comment says must stay fixed.🛡️ Suggested addition
static_assert(sizeof(PTO2TaskDescriptor) == 40, "PTO2TaskDescriptor size is part of the shared-memory ABI"); +static_assert( + offsetof(PTO2TaskDescriptor, packed_buffer_base) == 24, + "packed_buffer_base offset is part of the shared-memory ABI" +);🤖 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/a2a3/runtime/host_build_graph/runtime/pto_runtime2_types.h` around lines 206 - 220, Add a static_assert using offsetof(PTO2TaskDescriptor, packed_buffer_base) to enforce the documented fixed offset, alongside the existing size assertion. Keep the assertion aligned with the current ABI layout and retain the sizeof check for the overall descriptor size.
🤖 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.
Nitpick comments:
In `@src/a2a3/runtime/host_build_graph/runtime/pto_runtime2_types.h`:
- Around line 206-220: Add a static_assert using offsetof(PTO2TaskDescriptor,
packed_buffer_base) to enforce the documented fixed offset, alongside the
existing size assertion. Keep the assertion aligned with the current ABI layout
and retain the sizeof check for the overall descriptor size.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a9f6e83f-948d-4ad0-ab9c-08d8ccda8f39
📒 Files selected for processing (7)
src/a2a3/runtime/host_build_graph/runtime/orchestrator_core/pto_orchestrator.cppsrc/a2a3/runtime/host_build_graph/runtime/pto_runtime2_types.hsrc/a2a3/runtime/host_build_graph/runtime/pto_submit_types.hsrc/a2a3/runtime/host_build_graph/runtime/pto_types.hsrc/a2a3/runtime/host_build_graph/runtime/scheduler/pto_scheduler.hsrc/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_completion.cppsrc/a2a3/runtime/host_build_graph/runtime/scheduler/scheduler_dispatch.cpp
…hbg) Mirror of the tensormap_and_ringbuffer consolidation (hw-native-sys#1402) to the a2a3 host_build_graph runtime: fold the four scattered per-task markers into a single TaskAttrs byte on PTO2TaskSlotState, and revert active_mask to a pure subtask-slot mask. hbg gained predicated dispatch in hw-native-sys#1434, so it now carries the same four markers as tmr (allow_early_resolve, sync_start, has_predicate, timing tag) and this is a 1:1 port of the settled tmr design. - Add TaskAttrs (pto_submit_types.h): bit0 allow_early_resolve, bit1 sync_start, bit2 has_predicate, bit3 is_timed, bits4-7 timing_tag. - Drop SYNC_START/HAS_PREDICATE flag bits and accessors from ActiveMask; it now carries only core_mask (bits 0-2). - Remove task_timing_slot from PTO2TaskDescriptor; the tag now rides the scheduler's hot slot_state line. Descriptor size/ABI (40B, packed_buffer_base offset) unchanged; PTO2TaskSlotState stays 64B (task_attrs reuses the byte allow_early_resolve occupied, alongside the atomic lifecycle_flags). - prepare_task writes task_attrs per submit alongside active_mask; attributes are assembled at each submit entry point (submit_task, submit_dummy_task, alloc_tensors). - Update all scheduler read sites (dispatch/completion/scheduler.h) to read the attributes off slot_state. - Static-assert NUM_TASK_TIMING_SLOTS <= 16 and TASK_TIMING_SLOT_NONE == -1 so the 4-bit tag field and the untagged sentinel stay coupled. Accessor naming matches the settled tmr baseline: predicate getters (allow_early_resolve / requires_sync_start / has_predicate / is_timed), set_<concept> setters (set_early_resolve / set_sync_start / set_predicate / set_timing_slot). Behavior-neutral refactor. Verified: a2a3sim st (predicated_dispatch, matmul, bgemm, paged_attention, vector_example). The a2a3 cpput timing/wiring tests build against tmr headers (unchanged here). Part of hw-native-sys#1402.
ad9d409 to
2f7cadc
Compare
Summary
Mirrors the
tensormap_and_ringbufferconsolidation (#1402) to the a2a3host_build_graphruntime. Folds the four scattered per-task markers into a singleTaskAttrsbyte onPTO2TaskSlotStateand revertsactive_maskto a pure subtask-slot mask.hbg gained predicated dispatch in #1434, so it now carries the same four markers as tmr (
allow_early_resolve,sync_start,has_predicate, timing tag) — this is a 1:1 port of the settled tmr design. Based on latestmain(includes #1434).Changes
TaskAttrs(pto_submit_types.h) — one byte:bit0allow_early_resolve,bit1sync_start,bit2has_predicate,bit3is_timed,bits4-7timing_tag (0..15).ActiveMaskreverts to pure subtask-slot mask — droppedSYNC_START/HAS_PREDICATEbits/accessors; onlycore_mask(bits 0-2) remains.task_timing_slotremoved fromPTO2TaskDescriptor— tag now rides the hotslot_stateline. Descriptor 40B /packed_buffer_baseoffset unchanged (ABI preserved).PTO2TaskSlotStatestays 64B (task_attrsreuses the byteallow_early_resolveoccupied, alongside the atomiclifecycle_flags).prepare_task; all scheduler read sites updated.static_assertcouples the 4-bit tag toNUM_TASK_TIMING_SLOTS <= 16/TASK_TIMING_SLOT_NONE == -1.allow_early_resolve/requires_sync_start/has_predicate/is_timed),set_<concept>setters.Testing
add_a2a3_testinclude path), unchanged here — no UT edits needed.Relation to #1402
a2a3 host_build_graph mirror. Companion PR #1410 covers a2a3/tmr + a5/tmr. Remaining: a5
host_build_graph(legacyRuntime::Taskpath). Part of #1402.