Repository navigation
Refactor: consolidate per-task markers into one TaskAttrs byte (a2a3 + a5 tmr) - #1410
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 PR introduces a packed ChangesTask marker consolidation
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 |
There was a problem hiding this comment.
Code Review
This pull request refactors per-task scheduling flags by consolidating them into a new 1-byte TaskMarkers struct on PTO2TaskSlotState. This change moves the selective timing tag off the PTO2TaskDescriptor (preserving its 40-byte shared-memory ABI size) and onto the scheduler's hot cache line. It also simplifies ActiveMask to be a pure subtask-slot mask (AIC/AIV0/AIV1) without carrying unrelated flag bits. Corresponding updates are made across the orchestrator, scheduler, documentation, and unit tests. I have no feedback to provide as there are no review comments.
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.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_runtime2_types.h (1)
207-209: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a compile-time offset guard alongside the size guard.
The comment states both descriptor size and
packed_buffer_baseoffset are part of the ABI, but onlysizeofis enforced with astatic_asserthere — the offset is only verified in the unit test (DescriptorDoesNotGrow). A header-level assert catches an accidental offset shift even without running tests.♻️ Proposed addition
+// packed_buffer_base sits after the 4-byte alignment pad following kernel_id[3]; +// the shared-memory ABI depends on this offset staying fixed too. +static_assert( + offsetof(PTO2TaskDescriptor, packed_buffer_base) == 24, + "PTO2TaskDescriptor::packed_buffer_base offset is part of the shared-memory ABI" +); static_assert(sizeof(PTO2TaskDescriptor) == 40, "PTO2TaskDescriptor size 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/tensormap_and_ringbuffer/runtime/pto_runtime2_types.h` around lines 207 - 209, Add a header-level static_assert alongside the existing PTO2TaskDescriptor size assertion to verify that packed_buffer_base remains at its ABI-defined offset. Use the established descriptor layout and offsetof-based check, preserving the existing size guard and avoiding changes to the structure layout.
🤖 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/tensormap_and_ringbuffer/runtime/pto_runtime2_types.h`:
- Around line 207-209: Add a header-level static_assert alongside the existing
PTO2TaskDescriptor size assertion to verify that packed_buffer_base remains at
its ABI-defined offset. Use the established descriptor layout and offsetof-based
check, preserving the existing size guard and avoiding changes to the structure
layout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0eaa1daf-c7d0-4b68-8137-71d26eb8c867
📒 Files selected for processing (11)
docs/dfx/device-phases.mdsrc/a2a3/runtime/tensormap_and_ringbuffer/docs/RUNTIME_LOGIC.mdsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_orchestrator.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_runtime2_types.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_submit_types.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/pto_types.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/pto_scheduler.hsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cppsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_dispatch.cpptests/ut/cpp/a2a3/test_task_timing_slots.cpptests/ut/cpp/a2a3/test_wiring.cpp
fbaf109 to
eea58ae
Compare
eea58ae to
4c92922
Compare
0d47644 to
0aa3142
Compare
…+ a5 tmr) Four independent per-task markers were scattered across three storage sites: sync_start / has_predicate squatted in active_mask's spare high bits, the timing tag lived in the PTO2TaskDescriptor pad (a cold cache line), and allow_early_resolve used a whole byte for one bool. Consolidate all four into a single TaskAttrs byte on PTO2TaskSlotState, reusing the byte allow_early_resolve already occupied (slot_state stays 64 bytes), and revert active_mask to a pure subtask-slot mask. Applied to both tensormap_and_ringbuffer runtimes (a2a3 baseline + a5 1:1 port, which was structurally identical). - Add TaskAttrs (pto_submit_types.h): bit0 allow_early_resolve, bit1 sync_start, bit2 has_predicate, bit3 is_timed, bits4-7 timing_tag. is_timed distinguishes "untagged" from the 16 valid tag values. - Drop SYNC_START/HAS_PREDICATE flag bits and their accessors from ActiveMask; it now carries only core_mask (bits 0-2) and can be OR/==-combined for MIX clusters without dragging flag bits. - 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; task_attrs coexists with the atomic lifecycle_flags byte. - 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: getters are predicates (allow_early_resolve / requires_sync_start / has_predicate / is_timed); setters are set_<concept> (set_early_resolve / set_sync_start / set_predicate / set_timing_slot). Behavior-neutral refactor; a2a3 same-device A/B benchmark is within the run-to-run noise floor. Updated test_task_timing_slots (TaskAttrs roundtrip replaces the descriptor-field test), test_wiring, and the DFX/RUNTIME_LOGIC docs. Mirroring to a2a3 host_build_graph and a5 host_build_graph is left as follow-up. Fixes hw-native-sys#1402.
The TaskAttrs migration (#1410) moved has_predicate off ActiveMask but missed this SIMPLER_DFX-only call site in the a5 tmr scheduler (the a2a3 twin was missed too). DFX-enabled builds fail with 'ActiveMask has no member named has_predicate'.
…p path (#1445) #1410 consolidated the per-task dispatch predicates into the TaskAttrs byte, moving has_predicate() from ActiveMask to TaskAttrs and updating the scheduler call sites. It missed the two dummy-drain swimlane-skip calls that #1419 added: a2a3 and a5 tensormap_and_ringbuffer scheduler_dispatch.cpp still call dummy_slot.active_mask.has_predicate(). ActiveMask no longer has that member, so the default build (the calls sit under SIMPLER_DFX, which is on by default) fails to compile a2a3sim / a5sim. The two PRs were each green against their own base; the break only appears once both are on main. Point both call sites at dummy_slot.task_attrs.has_predicate(), matching every other has_predicate() caller in the scheduler. a2a3sim and a5sim build clean.
The TaskAttrs migration (#1410) moved has_predicate off ActiveMask but missed this SIMPLER_DFX-only call site in the a5 tmr scheduler (the a2a3 twin was missed too). DFX-enabled builds fail with 'ActiveMask has no member named has_predicate'.
PR hw-native-sys#1410 moved has_predicate() off ActiveMask onto TaskAttrs for the tensormap_and_ringbuffer runtime, but hw-native-sys#1419's predicated-skip swimlane call site in scheduler_dispatch.cpp (a2a3 + a5) still read active_mask.has_predicate(), which fails to compile under SIMPLER_DFX ('class ActiveMask' has no member has_predicate). Point it at dummy_slot.task_attrs.has_predicate(), matching every other TMR call site. host_build_graph is unaffected (hw-native-sys#1410 was TMR-only). Build-required on this branch (the broken call site is inherited from main). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summary
Consolidates the four scattered per-task markers in
tensormap_and_ringbufferinto a singleTaskAttrsbyte onPTO2TaskSlotState, and revertsactive_maskto a pure subtask-slot mask. Addresses code-health issue #1402. Covers both a2a3/tmr (baseline) and a5/tmr (1:1 port — a5/tmr was structurally identical).Before, the four markers lived in three places with inconsistent rationale:
sync_start/has_predicatesquatted inactive_mask's spare high bits (bits 3-4)PTO2TaskDescriptorpad — a cold cache line, read via a pointer hop on the completion pathallow_early_resolveused a whole byte for one boolChanges (per runtime, a2a3 + a5)
TaskAttrs(pto_submit_types.h) — one byte:bit0allow_early_resolve,bit1sync_start,bit2has_predicate,bit3is_timed,bits4-7timing_tag (0..15).is_timeddistinguishes "untagged" from the 16 valid tag values.ActiveMaskreverts to a pure subtask-slot mask — dropped theSYNC_START/HAS_PREDICATEflag bits and accessors; it carries onlycore_mask(bits 0-2) and can beOR/==-combined for MIX clusters without dragging flag bits.task_timing_slotremoved fromPTO2TaskDescriptor— the tag now rides the scheduler's hotslot_stateline (better completion-path locality). Descriptor size (40B) andpacked_buffer_baseoffset unchanged (shared-memory ABI preserved).PTO2TaskSlotStatestays exactly 64 bytes —task_attrsreuses the byteallow_early_resolveoccupied, alongside the atomiclifecycle_flags.prepare_task(alongsideactive_mask), assembled at each submit entry point. All scheduler read sites read the attributes offslot_state.static_assertcouples the 4-bit tag field toNUM_TASK_TIMING_SLOTS <= 16/TASK_TIMING_SLOT_NONE == -1.Accessor naming: getters are predicates (
allow_early_resolve/requires_sync_start/has_predicate/is_timed); setters areset_<concept>(set_early_resolve/set_sync_start/set_predicate/set_timing_slot).Performance is neutral (measured): pulling flags out of
active_maskcosts at most one extra L1-hitldrb, dwarfed by the ~95 ns MMIOwrite_regon the dispatch path; the timing tag improves completion-path locality. An a2a3 same-device A/B (both fresh worktrees, 100 rounds × 6 examples) showed deltas within the ±5% run-to-run noise floor with no consistent direction; the most stable workload (qwen3 decode) was flat (+0.3%).Commits
TaskAttrsbyte — a2a3/tmr (baseline; includes UT + DFX/RUNTIME_LOGIC docs).TaskAttrsbyte — a5/tmr (1:1 port; a5 has no marker UT, so runtime src + a5 RUNTIME_LOGIC.md only).Testing
test_task_timing_slots+test_wiring(a2a3, 60/60 non-hardware),test_a5_wiringRelation to #1402
a2a3/tmr + a5/tmr done here. Remaining per the issue: a2a3
host_build_graphand a5host_build_graph(legacyRuntime::Taskpath) — follow-up. Fixes #1402.