Converge a5 host_build_graph onto the host-orchestrated runtime - #1661
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 Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds the A5 host-build-graph Runtime2 implementation. It introduces shared runtime contracts, host orchestration, TensorMap dependency tracking, scheduler dispatch, asynchronous completion handling, AICPU coordination, AICore execution, profiling, and runtime documentation. ChangesHost-build-graph Runtime2
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Host
participant AICPU
participant Scheduler
participant AICore
participant Runtime
Host->>Runtime: stage tensors and build runtime image
Host->>AICPU: invoke aicpu_execute
AICPU->>Scheduler: initialize shared runtime and classify tasks
Scheduler->>AICore: publish dispatch payload
AICore-->>Scheduler: acknowledge execution and completion
Scheduler-->>AICPU: report runtime status
AICPU-->>Host: return execution result
Possibly related issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (13)
src/a5/runtime/host_build_graph2/docs/RUNTIME_LOGIC.md-768-774 (1)
768-774: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAlign the API summary with the canonical declarations.
This table omits
PTO2Runtime*andnum_argsfromrt_submit_task,rt_submit_aic_task, andrt_submit_aiv_task.SUBMIT_BY_CLUSTER.mddocuments those parameters.If this table is shorthand, label it as shorthand. Otherwise, document the complete signatures to avoid inconsistent orchestration API usage.
🤖 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/a5/runtime/host_build_graph2/docs/RUNTIME_LOGIC.md` around lines 768 - 774, Update the API summary table to match the canonical declarations by including PTO2Runtime* and num_args in rt_submit_task, rt_submit_aic_task, and rt_submit_aiv_task. If the entries are intentionally abbreviated, explicitly label the table as shorthand; otherwise, document each complete signature consistently with SUBMIT_BY_CLUSTER.md.src/a5/runtime/host_build_graph2/docs/profiling_levels.md-420-426 (1)
420-426: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClarify the profiling log-count units.
Level 0 says diagnostic and progress
LOG_INFOmessages can still appear, but the summary table reportsLOG_INFO Count = 0. Levels 2 through 4 also omit counts even though earlier sections state counts of 18, 30, and 34.Specify whether these are profiling-only counts, per-thread counts, or total per-run counts. The current table is not consistent with the preceding sections.
🤖 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/a5/runtime/host_build_graph2/docs/profiling_levels.md` around lines 420 - 426, Update the profiling-level summary table to define the LOG_INFO Count unit explicitly, using the same profiling-only, total-per-run counting convention established in the preceding sections. Add the correct counts for levels 0 through 4, including the existing 18, 30, and 34 values, and clarify that level 0 excludes unrelated diagnostic or progress messages.src/a5/runtime/host_build_graph2/docs/SUBMIT_BY_CLUSTER.md-203-214 (1)
203-214: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPoint validation commands at the A5 host_build_graph2 scaffold.
These commands run the a2a3 tensormap_and_ringbuffer test and use
--platform a2a3. They do not validatesrc/a5/runtime/host_build_graph2, which the PR objective says remains undiscovered.Mark these commands as main-branch baseline tests, or add A5-specific checks for discovery, build exclusion, and documentation consistency.
🤖 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/a5/runtime/host_build_graph2/docs/SUBMIT_BY_CLUSTER.md` around lines 203 - 214, Update the validation section in SUBMIT_BY_CLUSTER.md so the existing a2a3 commands are clearly labeled as main-branch baseline tests, and add A5-specific checks covering host_build_graph2 discovery, build exclusion, and documentation consistency. Keep the current baseline commands unchanged while ensuring the added validation explicitly targets the A5 host_build_graph2 scaffold.src/a5/runtime/host_build_graph2/host/dep_gen_host_graph.cpp-176-195 (1)
176-195: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClamp the stored
ndims, not only the copy loop.
fill_consumerandfill_producerguard the copy loop withi < MAX_TENSOR_DIMS, but they store the unclampedt.ndims/entry.ndims.write_deps_jsonthen uses that value as the array length at Line 348, Line 351, Line 355 and Line 358. If a source value ever exceedsMAX_TENSOR_DIMS, the writer reads pastconsumer_shape/producer_shape. The same pattern exists forslot.ndimsat Line 419 and its use at Line 310 and Line 313.Either clamp the stored value, or drop the loop guard because the invariant already holds.
🛡️ Proposed fix: clamp the stored dimension count
void fill_consumer(EdgeAnnot &e, const Tensor &t) { e.consumer_dtype = static_cast<uint8_t>(t.dtype); - e.consumer_ndims = t.ndims; + e.consumer_ndims = t.ndims < MAX_TENSOR_DIMS ? t.ndims : MAX_TENSOR_DIMS; e.consumer_start_offset = t.start_offset; - for (uint32_t i = 0; i < t.ndims && i < MAX_TENSOR_DIMS; i++) { + for (uint32_t i = 0; i < e.consumer_ndims; i++) { e.consumer_shape[i] = t.shapes[i]; e.consumer_strides[i] = t.strides[i]; } } @@ void fill_producer(EdgeAnnot &e, const PTO2TensorMapEntry &entry) { - e.producer_ndims = entry.ndims; + e.producer_ndims = entry.ndims < MAX_TENSOR_DIMS ? entry.ndims : MAX_TENSOR_DIMS; e.producer_start_offset = entry.start_offset; - for (uint32_t i = 0; i < entry.ndims && i < MAX_TENSOR_DIMS; i++) { + for (uint32_t i = 0; i < e.producer_ndims; i++) { e.producer_shape[i] = entry.shapes[i]; e.producer_strides[i] = entry.strides[i]; } }🤖 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/a5/runtime/host_build_graph2/host/dep_gen_host_graph.cpp` around lines 176 - 195, Clamp the stored dimension counts in fill_consumer and fill_producer to MAX_TENSOR_DIMS before write_deps_json uses them for array lengths, while preserving the bounded copy loops. Apply the same clamping to slot.ndims at its construction site so the values consumed by the slot serialization paths cannot exceed the backing array capacity.src/a5/runtime/host_build_graph2/host/runtime_maker.cpp-607-633 (1)
607-633: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnlink the temp .so on the
dlopenanddlsymfailure paths.
create_orch_so_tempfilewrites a file under/tmp. The code unlinks it only at Line 633, after both symbol lookups succeed. Ifdlopenfails at Line 613, or eitherdlsymfails, the file stays on disk for the process lifetime. Repeated failed prepares accumulate files in/tmp.♻️ Proposed fix: unlink on every exit path
void *handle = dlopen(so_path.c_str(), RTLD_NOW | RTLD_LOCAL); if (handle == nullptr) { LOG_ERROR("host-orch: dlopen failed: %s", dlerror()); + unlink(so_path.c_str()); return -1; } + // Safe to unlink now: the handle keeps the .so mapped regardless of path. + unlink(so_path.c_str()); void *entry = dlsym(handle, orch_func_name); if (entry == nullptr) { LOG_ERROR("host-orch: dlsym('%s') failed: %s", orch_func_name, dlerror()); dlclose(handle); return -1; } @@ if (bind_sym == nullptr) { LOG_ERROR("host-orch: orch .so does not export framework_bind_runtime: %s", dlerror()); dlclose(handle); return -1; } - // Safe to unlink now: the handle keeps the .so mapped regardless of path. - unlink(so_path.c_str());🤖 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/a5/runtime/host_build_graph2/host/runtime_maker.cpp` around lines 607 - 633, Ensure the temporary orchestration .so created by create_orch_so_tempfile is unlinked before every failure return in the dlopen, orch_func_name dlsym, and framework_bind_runtime dlsym paths. Preserve the existing successful-path unlink and close handles as currently required.src/a5/runtime/host_build_graph2/runtime/scheduler/pto_scheduler.cpp-76-82 (1)
76-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a single
last_task_alivevalue inprint_stats().
ring_sched_stateis not an array; it has onelast_task_aliveand oneringmember. This loop prints the same shared value multiple times with differentRing %d:labels. Replace it with one check/print, or model per-ring state if the scheduler supports multiple rings.🤖 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/a5/runtime/host_build_graph2/runtime/scheduler/pto_scheduler.cpp` around lines 76 - 82, Update print_stats() to stop iterating over PTO2_MAX_RING_DEPTH for the shared ring_sched_state.last_task_alive value; perform one check and print the value once with an appropriate non-per-ring label, preserving the existing statistics output.src/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_completion.cpp-104-105 (1)
104-105: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
mailboxis computed with a null guard, then dereferenced without one.Line 104 tolerates
rt_ == nullptrand leavesmailboxnull. Lines 139 and 165 callmailbox->try_push_conditionandmailbox->try_push_normal_doneunconditionally. Ifrt_is ever null on this path, the AICPU faults. Either drop the conditional at Line 104 and assertrt_ != nullptr, or latchPTO2_ERROR_ASYNC_REGISTRATION_FAILEDwhenmailboxis null before the push loops.Also applies to: 137-143, 162-168
🤖 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/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_completion.cpp` around lines 104 - 105, Update the scheduler completion path around the mailbox initialization and push loops to handle a null mailbox consistently: either assert rt_ is non-null before deriving AICoreCompletionMailbox, or detect a null mailbox and latch PTO2_ERROR_ASYNC_REGISTRATION_FAILED before reaching the try_push_condition and try_push_normal_done calls. Ensure neither push loop can dereference mailbox when it is null.src/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_cold_path.cpp-976-1029 (1)
976-1029: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
deinit()does not release thesp_queues_buffers.
CompletedTaskQueue::initallocatesbufwithnew[], andon_orchestration_donecallsdestroy()only fort < active_sched_threads_of the run that is starting.deinit()resets every other owned field but never frees these buffers. Two consequences:
- The buffers stay allocated after the final teardown.
- If a later run uses fewer scheduler threads, the buffers of the higher indices are never freed and never reused.
Destroy all
MAX_AICPU_THREADSqueues here.🛡️ Proposed fix
regs_ = 0; sched_ = nullptr; rt_ = nullptr; func_id_to_addr_ = nullptr; + + // Release the per-S completed-task rings allocated in on_orchestration_done. + for (int32_t t = 0; t < MAX_AICPU_THREADS; t++) { + sp_queues_[t].destroy(); + } }🤖 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/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_cold_path.cpp` around lines 976 - 1029, Update SchedulerContext::deinit() to destroy every queue in sp_queues_, iterating across all MAX_AICPU_THREADS entries rather than only active_sched_threads_. Reuse each CompletedTaskQueue’s destroy() operation so all buffers allocated by init() are released, including queues from prior runs with higher thread counts.src/a5/runtime/host_build_graph2/runtime/pto_types.h-285-301 (1)
285-301: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
clear()does not resetlaunch_spec.
clear()resets every other per-submit field:explicit_deps_,explicit_dep_count_,allow_early_resolve_,task_timing_slot_, andpredicate_. It leaveslaunch_specuntouched. AnArgthat is reused afterclear()orreset()keeps the previousblock_numandrequire_sync_start, so the next submit launches with the wrong SPMD shape.L2TaskArgs::create_from_chip_argscallsreset()and depends on a clean state.If the persistence is intentional, state the reason in a comment.
🐛 Proposed fix
allow_early_resolve_ = false; task_timing_slot_ = TASK_TIMING_SLOT_NONE; predicate_ = L0TaskPredicate{}; + launch_spec = PTO2LaunchSpec{}; }🤖 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/a5/runtime/host_build_graph2/runtime/pto_types.h` around lines 285 - 301, Update Arg::clear() to reset launch_spec along with the other per-submit fields, ensuring reused arguments restore default block_num and require_sync_start values before the next submission. Keep reset()’s existing delegation to clear() so L2TaskArgs::create_from_chip_args receives clean state.src/a5/runtime/host_build_graph2/runtime/tensor_create_info.h-91-92 (1)
91-92: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThe defaulted default constructor leaves every field indeterminate.
The class documentation states that
start_offsetis pre-zeroed andis_contiguousis pre-set totrue, andinit_tensor_from_create_inforelies on both.TensorCreateInfo() = default;gives none of those guarantees. A default-constructed instance that reachesinit_tensor_from_create_infocopies 64 bytes of indeterminate data intoTensorcache line 1, which sets a garbageversion,is_contiguous,manual_dep, andstart_offset. Thealways_assert(ci.ndims > 0 && ci.ndims <= MAX_TENSOR_DIMS)catches only part of that.Either delete the default constructor, or value-initialize the members that the contract promises.
🛡️ Proposed change
- TensorCreateInfo() = default; + // No default state satisfies the line-1 contract (start_offset == 0, + // is_contiguous == true), so require the shapes constructor. + TensorCreateInfo() = delete;🤖 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/a5/runtime/host_build_graph2/runtime/tensor_create_info.h` around lines 91 - 92, Update TensorCreateInfo’s default construction so the documented defaults are guaranteed: initialize start_offset to zero and is_contiguous to true, while preserving valid initialization for all other fields consumed by init_tensor_from_create_info. Replace the defaulted constructor or explicitly value-initialize the relevant members in TensorCreateInfo.src/a5/runtime/host_build_graph2/runtime/backend/sdma/sdma_completion_kernel.h-94-96 (1)
94-96: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClarify
event.handle == 0and keep the fast path safe.This path does not register a completion on
handle == 0. The URMA/simulation tests callWaitAll()after synchronous async ops withhandle=0, but this wrapper only handles that at the invalidAsyncCtxguard. Add a short comment ifhandle == 0is an expected synchronous/result-null case afterTGET_ASYNC()/TPUT_ASYNC(), or deferPTO2_ERROR_ASYNC_COMPLETION_INVALIDwhenhandle == 0is unexpected.🤖 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/a5/runtime/host_build_graph2/runtime/backend/sdma/sdma_completion_kernel.h` around lines 94 - 96, Clarify the handle-zero branch in the completion wrapper around the existing event.handle check: document that handle == 0 is the expected synchronous/result-null case after TGET_ASYNC()/TPUT_ASYNC(), while preserving the safe early return. If handle == 0 is not valid, replace the silent return with deferred PTO2_ERROR_ASYNC_COMPLETION_INVALID handling instead.src/a5/runtime/host_build_graph2/runtime/pto_submit_types.h-57-75 (1)
57-75: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the sign-extension shift and correct the size comment.
Two points in
DispatchPredicate:
- If
op != PredicateOp::NONEandelem_size == 0, thenbits == 0andshift == 64. A shift of a 64-bit value by 64 is undefined behavior.elem_sizeis populated at submit, so a partly filled predicate reaches this path. Treatbits == 0as "no predicate" or assert the valid widths (1/2/4/8).- The comment states 18 bytes. The struct holds 18 bytes of members, but
sizeof(DispatchPredicate)is 24 because of trailing padding to the 8-byte alignment. State both numbers so payload-layout readers do not mis-size the field.🛡️ Proposed guard
bool pass() const { if (op == PredicateOp::NONE) return true; + if (elem_size == 0 || elem_size > sizeof(int64_t)) return true; int64_t v = 0; __builtin_memcpy(&v, reinterpret_cast<const void *>(addr), elem_size); uint32_t bits = static_cast<uint32_t>(elem_size) * 8u;🤖 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/a5/runtime/host_build_graph2/runtime/pto_submit_types.h` around lines 57 - 75, Update DispatchPredicate::pass to handle elem_size == 0 before computing or applying the sign-extension shift, treating it as no predicate or enforcing the valid widths 1, 2, 4, and 8; ensure no 64-bit shift occurs. Correct the DispatchPredicate layout comment to state that its members total 18 bytes while sizeof(DispatchPredicate) is 24 bytes due to trailing alignment padding.src/a5/runtime/host_build_graph2/runtime/pto_shared_memory.h-117-140 (1)
117-140: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude
<algorithm>forstd::max.
<atomic>does not definestd::max, and this header does not receive<algorithm>transitively. Add the include so the header remains self-contained.🔧 Proposed fix
`#pragma` once +#include <algorithm> `#include` <stddef.h> `#include` "utils/device_arena.h" `#include` "pto_runtime2_types.h"🤖 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/a5/runtime/host_build_graph2/runtime/pto_shared_memory.h` around lines 117 - 140, Add the <algorithm> header include to pto_shared_memory.h so std::max used in update_completed_watermark is defined independently of transitive includes.
🧹 Nitpick comments (15)
src/a5/runtime/host_build_graph2/orchestration/common.cpp (1)
56-69: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winQuote the executable path in the
addr2linecommand.
addr_to_lineinterpolatesdl_info.dli_fnamestraight into a shell command string and runs it withpopen. A module path that contains a space, a quote, or a shell metacharacter produces a wrong command or an unintended shell action. Wrap the path in single quotes so the shell treats it as one literal token.Also note
snprintftruncates at 512 bytes. A long module path then yields a malformed command. Truncation only degrades the diagnostic, so it is not a correctness risk.♻️ Proposed fix: quote the path
- snprintf(cmd, sizeof(cmd), "addr2line -e %s -f -C -p -i %p 2>/dev/null", executable, addr); + snprintf(cmd, sizeof(cmd), "addr2line -e '%s' -f -C -p -i %p 2>/dev/null", executable, addr);🤖 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/a5/runtime/host_build_graph2/orchestration/common.cpp` around lines 56 - 69, Update addr_to_line to shell-quote the executable path when constructing the addr2line command, ensuring paths containing spaces or shell metacharacters remain a single literal argument. Preserve the existing command behavior and accept the current snprintf truncation handling.src/a5/runtime/host_build_graph2/runtime/scheduler/pto_scheduler.cpp (1)
94-98: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winTie
shape_names[]size toPTO2_NUM_RESOURCE_SHAPESwith a static_assert.
shape_names[]is a fixed 3-element array, and the loop indexes it up toPTO2_NUM_RESOURCE_SHAPES. If that constant grows without updating this array, the loop reads out of bounds. This codebase already uses static_asserts elsewhere (for example inpto2_dispatch_payload.h) to guard exactly this kind of drift.🛡️ Proposed fix
const char *shape_names[] = {"AIC", "AIV", "MIX"}; + static_assert(sizeof(shape_names) / sizeof(shape_names[0]) == PTO2_NUM_RESOURCE_SHAPES, + "shape_names must match PTO2_NUM_RESOURCE_SHAPES");🤖 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/a5/runtime/host_build_graph2/runtime/scheduler/pto_scheduler.cpp` around lines 94 - 98, Add a compile-time static_assert next to shape_names in the scheduler logging code to verify its element count equals PTO2_NUM_RESOURCE_SHAPES, while preserving the existing loop and logging behavior.src/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_dispatch.cpp (1)
900-906: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated scheduler-timeout resolution.
run_resolution_threadandresolve_and_dispatchcomputescheduler_timeout_cyclesfromget_scheduler_timeout_ms()with identical code. Extract one small private helper so a future change to the override semantics stays in one place. This is not a request for a new configuration knob; the existing override stays as is.Also applies to: 1161-1170
🤖 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/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_dispatch.cpp` around lines 900 - 906, The scheduler timeout-cycle calculation is duplicated in run_resolution_thread and resolve_and_dispatch. Extract the shared default-and-override resolution into one small private helper, then have both methods call it while preserving SCHEDULER_TIMEOUT_CYCLES and the existing get_scheduler_timeout_ms() override semantics.Source: Learnings
src/a5/runtime/host_build_graph2/aicpu/aicpu_executor.cpp (1)
184-188: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAdd a spin hint to the init barriers.
These three spin loops poll shared atomics with no back-off. Every other spin in this cohort uses
SPIN_WAIT_HINT()(seerun()at Lines 314-330). Add the hint so the waiting AICPU threads do not saturate the interconnect during the parallel handshake.Also applies to: 196-196, 206-210
🤖 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/a5/runtime/host_build_graph2/aicpu/aicpu_executor.cpp` around lines 184 - 188, Add SPIN_WAIT_HINT() inside each init barrier polling loop, including the loops around hs_setup_done_ and the additional ranges at the referenced locations. Keep the existing atomic checks and return behavior unchanged, placing the hint on each iteration that continues waiting.src/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_cold_path.cpp (1)
723-726: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the error text and the stated limit.
The message says "more then" and names 64 cores. The real limit is
CoreTracker::MAX_CORE_PER_THREAD, which isMAX_CLUSTERS * PLATFORM_CORES_PER_BLOCKDIM. Report the computed and allowed values so the failure is actionable.♻️ Proposed fix
- LOG_ERROR("Can't assign more then 64 cores in per scheduler"); + LOG_ERROR( + "Cannot assign %d cores per scheduler thread (max=%d)", thread_cores_num, + CoreTracker::MAX_CORE_PER_THREAD + );🤖 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/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_cold_path.cpp` around lines 723 - 726, Update the validation error in the scheduler core-assignment path to use “than” and report both the requested thread_cores_num value and the allowed CoreTracker::MAX_CORE_PER_THREAD value. Remove the hardcoded 64-core wording while preserving the existing failure return.src/a5/runtime/host_build_graph2/runtime/scheduler/pto_scheduler.h (1)
161-199: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueThe profiling
pushoverload does not writetask_id_snapshot.
push_taggedsetsslot->task_id_snapshot, andpop_taggedreads it. This profiling overload writes onlyslot_stateandsequence, so the slot keeps the snapshot of a previous occupant. Anypop_taggedconsumer (for exampleearly_dispatch_shape) then compares against a stale id. Set the field to 0 here to match the non-profilingpush.♻️ Proposed fix
slot->slot_state = slot_state; + slot->task_id_snapshot = 0; slot->sequence.store(static_cast<int64_t>(pos + 1), std::memory_order_release);🤖 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/a5/runtime/host_build_graph2/runtime/scheduler/pto_scheduler.h` around lines 161 - 199, Update the profiling push overload in PTO2ReadyQueueSlot::push to assign slot->task_id_snapshot to 0 when populating the slot, alongside slot->slot_state and before publishing sequence. Preserve the existing profiling counters and queue synchronization logic.src/a5/runtime/host_build_graph2/runtime/backend/sdma/sdma_completion_kernel.h (1)
121-143: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlign the return value with the documented contract.
The comment states that the function records the error and returns false on failure.
register_pto_async_eventcan calldefer_error(ctx, PTO2_ERROR_ASYNC_COMPLETION_INVALID)for a non-SDMA engine or a failedPrepareEventCheck, butsend_request_entrystill returnstrue. A caller that branches on the return value treats a failed completion registration as success. Return the registration result, or restate the contract in the comment.♻️ Proposed change to propagate the registration result
-template <typename PtoAsyncEvent, typename PtoAsyncSession> -inline __aicore__ void -register_pto_async_event(AsyncCtx &ctx, const PtoAsyncEvent &event, const PtoAsyncSession &session) { +template <typename PtoAsyncEvent, typename PtoAsyncSession> +inline __aicore__ bool +register_pto_async_event(AsyncCtx &ctx, const PtoAsyncEvent &event, const PtoAsyncSession &session) { if (ctx.task_token.is_invalid() || ctx.completion_count == nullptr || ctx.completion_entries == nullptr) { (void)event.Wait(session); - return; + return true; } if (event.handle == 0) { - return; + return true; } const uint32_t engine = static_cast<uint32_t>(event.engine); if (engine != static_cast<uint32_t>(::pto::comm::DmaEngine::SDMA)) { defer_error(ctx, PTO2_ERROR_ASYNC_COMPLETION_INVALID); - return; + return false; }- pto2::detail::register_pto_async_event(ctx, event, session); + const bool registered = pto2::detail::register_pto_async_event(ctx, event, session); pto2::detail::defer_flush(ctx); - return true; + return registered;🤖 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/a5/runtime/host_build_graph2/runtime/backend/sdma/sdma_completion_kernel.h` around lines 121 - 143, Update send_request_entry to propagate the success/failure result from register_pto_async_event, ensuring registration failures return false in accordance with the documented contract while preserving the existing deferred flush behavior.src/a5/runtime/host_build_graph2/common/intrinsic.h (1)
98-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider binding the context indices to their derivation.
The comment states that the values derive from
MAX_TENSOR_ARGS(32) +MAX_SCALAR_ARGS(16), but the constants are hardcoded. If either limit changes, the kernel-side accessors read the wrong args slots and fail silently. Astatic_assertin a translation unit that already includes the args-limit header would pin the relation.🤖 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/a5/runtime/host_build_graph2/common/intrinsic.h` around lines 98 - 106, Bind SPMD_LOCAL_CONTEXT_INDEX and SPMD_GLOBAL_CONTEXT_INDEX to the MAX_TENSOR_ARGS and MAX_SCALAR_ARGS derivation instead of hardcoding 48 and 49. Add a static_assert in an appropriate translation unit that includes the args-limit definitions, verifying both context indices remain immediately after the combined argument limits.src/a5/runtime/host_build_graph2/runtime/pto_types.h (1)
443-453: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider rejecting a null
valuespointer.
add_scalarsguardscount < 0and the capacity, then callsmemcpy. A nullvalueswithcount > 0dereferences null insidememcpy. The sibling APIset_dependenciesalready rejects a null pointer withset_error. The same guard keeps the two entry points consistent.🤖 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/a5/runtime/host_build_graph2/runtime/pto_types.h` around lines 443 - 453, Update add_scalars to reject a null values pointer when count is greater than zero, alongside its existing count and capacity validation, by calling set_error and returning before memcpy; preserve the existing behavior for zero-count calls and valid inputs.src/a5/runtime/host_build_graph2/runtime/pto_tensormap.h (1)
183-198: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the
TensorCreateInfolayout mirror.
copy_tensor_create_infodoes a raw 64-bytememcpyfromTensorCreateInfointo cache line 1 of the entry. The equivalentTensormirror is protected by thestatic_assertblock at lines 337-345, but no assert coversTensorCreateInfo. If a field inTensorCreateInfomoves, this copy silently writes the wrongstart_offset,ndims,dtype, orshapes[], and overlap detection produces wrong dependencies. Add matchingstatic_asserts for the fields this memcpy relies on.🤖 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/a5/runtime/host_build_graph2/runtime/pto_tensormap.h` around lines 183 - 198, Add static_assert layout checks for TensorCreateInfo alongside the existing Tensor mirror assertions, covering the offsets of start_offset, ndims, dtype, and shapes[] plus the expected 64-byte copied layout. Keep copy_tensor_create_info’s raw memcpy unchanged and anchor the checks to the TensorCreateInfo and Tensor mirror definitions.src/a5/runtime/host_build_graph2/runtime/shared/pto_runtime2_init.cpp (2)
261-265: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the seen-epoch size from one source.
Line 262 sizes the memset from
layout.tensor_map.task_window_size, but line 251 does the SM address arithmetic with thetask_window_sizeparameter.reserve_layoutreserved the region from its owntask_window_sizeargument. The three values agree only because the current call path passes the same number everywhere. If they diverge, this memset clears past the reserved region. Use the layout-recorded value for both, or store the reserved byte count inPTO2OrchestratorLayout.🤖 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/a5/runtime/host_build_graph2/runtime/shared/pto_runtime2_init.cpp` around lines 261 - 265, Update the seen-epoch initialization around PTO2OrchestratorLayout so the memset size and SM address arithmetic both derive from the same layout-recorded task-window value used when reserving the region. Replace the independent task_window_size parameter usage in this path, or reuse a stored reserved byte count, ensuring memset never exceeds the reserved fanin_seen_epoch region.
347-388: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueFix the stale
prebuilt_layoutdoc comment.
src/a5/runtime/host_build_graph2/host/runtime_maker.cpp:880assignsrt->prebuilt_layout = layout;runtime_init_data_from_layoutandruntime_wire_arena_pointersonly populate it indirectly asrtis the image target. Update thepto_runtime2.hcomment so it describes the writer correctly instead of implying those two functions write it.🤖 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/a5/runtime/host_build_graph2/runtime/shared/pto_runtime2_init.cpp` around lines 347 - 388, Update the prebuilt_layout documentation in pto_runtime2.h to identify runtime_maker.cpp’s assignment to rt->prebuilt_layout as the writer. Remove the implication that runtime_init_data_from_layout or runtime_wire_arena_pointers populate this field indirectly, while preserving the rest of the field’s documented behavior.src/a5/runtime/host_build_graph2/runtime/shared/pto_shared_memory.cpp (1)
164-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
ring_segment_offsetsinstead of walking the layout by hand.Lines 164-171 recompute the segment walk manually. Only
task_descriptors_offsetis consumed; the threeoffset +=statements at lines 169-171 produce a value that nothing reads. The walk also omits thecompletion_flagssegment thatring_segment_offsetsincludes. The header comment inpto_shared_memory.h(lines 315-320) declaresring_segment_offsetsthe single source of truth for this layout, so this second walk can drift from it. Read the offset from the helper.♻️ Proposed refactor
- // Ring layout info - uint64_t offset = PTO2_ALIGN_UP(sizeof(PTO2SharedMemoryHeader), PTO2_ALIGN_SIZE); header->ring.task_window_size = task_window_sizes[0]; header->ring.task_window_mask = static_cast<int32_t>(task_window_sizes[0] - 1); header->ring.heap_size = heap_sizes[0]; - header->ring.task_descriptors_offset = offset; - offset += PTO2_ALIGN_UP(task_window_sizes[0] * sizeof(PTO2TaskDescriptor), PTO2_ALIGN_SIZE); - offset += PTO2_ALIGN_UP(task_window_sizes[0] * sizeof(PTO2TaskPayload), PTO2_ALIGN_SIZE); - offset += PTO2_ALIGN_UP(task_window_sizes[0] * sizeof(PTO2TaskSlotState), PTO2_ALIGN_SIZE); + header->ring.task_descriptors_offset = + pto2_sm_layout::ring_segment_offsets(task_window_sizes[0]).descriptors;🤖 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/a5/runtime/host_build_graph2/runtime/shared/pto_shared_memory.cpp` around lines 164 - 171, Update the shared-memory header initialization to obtain task_descriptors_offset from the existing ring_segment_offsets layout helper instead of manually advancing offset. Remove the unused offset increments for task descriptors, payloads, and slot state, while preserving the initial aligned offset only if required by ring_segment_offsets. Use the established helper as the single source of truth for the ring layout.src/a5/runtime/host_build_graph2/runtime/pto_runtime2_types.h (1)
25-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude guards are copied from a2a3 and do not identify the a5 tree. The scaffold reuses the a2a3 guard macros unchanged. Two variants appear: guards that name an a2a3 path, and guards with unqualified names. Both make an a5 header and its a2a3 twin mutually exclusive in one translation unit, so the first one included wins and the second expands to nothing. The comment in
pto_runtime2_types.h(lines 37-44) records that this cross-tree collision already happened in the shared host-dispatcher translation unit. The PR states path references are retained for later cleanup, so track these together in that cleanup.
src/a5/runtime/host_build_graph2/runtime/pto_runtime2_types.h#L25-L26: rename the guard fromSRC_A2A3_RUNTIME_TENSORMAP_AND_RINGBUFFER_RUNTIME_PTO_RUNTIME2_TYPES_H_to the a5host_build_graph2path, and update the matching#endifcomment on line 552.src/a5/runtime/host_build_graph2/runtime/pto_orchestrator.h#L28-L29: replacePTO_ORCHESTRATOR_Hwith a path-qualified guard, and update the#endifcomment on line 186.src/a5/runtime/host_build_graph2/runtime/pto_ring_buffer.h#L34-L35: replacePTO_RING_BUFFER_Hwith a path-qualified guard, and update the#endifcomment on line 500.src/a5/runtime/host_build_graph2/runtime/host_tensor_access.h#L50-L51: rename the guard fromSRC_A2A3_RUNTIME_HOST_BUILD_GRAPH_RUNTIME_HOST_TENSOR_ACCESS_H_to the a5 path, and update the#endifcomment on line 91.src/a5/runtime/host_build_graph2/runtime/pto_constants.h#L12-L13: rename the guard fromSRC_A2A3_RUNTIME_TENSORMAP_AND_RINGBUFFER_RUNTIME_PTO_CONSTANTS_H_to the a5 path, and update the#endifcomment on line 19.🤖 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/a5/runtime/host_build_graph2/runtime/pto_runtime2_types.h` around lines 25 - 26, Rename the include guards and matching `#endif` comments in src/a5/runtime/host_build_graph2/runtime/pto_runtime2_types.h (lines 25-26 and 552), pto_orchestrator.h (28-29 and 186), pto_ring_buffer.h (34-35 and 500), host_tensor_access.h (50-51 and 91), and pto_constants.h (12-13 and 19) to unique path-qualified a5 host_build_graph2 identifiers, replacing the copied a2a3 or unqualified guards while keeping each guard’s opening and closing names consistent.src/a5/runtime/host_build_graph2/runtime/shared/runtime.cpp (1)
97-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
<cstring>forstd::strncpyandmemset.
runtime.cppusesmemsetin the constructor andstd::strncpyinset_device_orch_func_name()/set_device_orch_config_name(), but it does not include<cstring>or<string.h>. Add the header after#include "runtime.h"so this translation unit does not rely on transitive includes.Proposed fix
`#include` "runtime.h" +#include <cstring> + `#include` "common/unified_log.h"🤖 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/a5/runtime/host_build_graph2/runtime/shared/runtime.cpp` around lines 97 - 113, Add the <cstring> header immediately after the runtime.h include in runtime.cpp so memset in the constructor and std::strncpy used by set_device_orch_func_name() and set_device_orch_config_name() are declared directly by this translation unit.
🤖 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/a5/runtime/host_build_graph2/aicore/aicore_executor.cpp`:
- Around line 192-206: Bound argument population in the tensor/scalar fill loops
near the existing SPMD context-index comment using a compile-time-derived
capacity equal to the space available before the context slots in
exec_payload->args. Ensure the combined tensor_count and scalar_count writes
never exceed that capacity, stopping or otherwise preventing further writes
while preserving the existing tensor-before-scalar ordering and barrier
behavior.
In `@src/a5/runtime/host_build_graph2/aicpu/aicpu_executor.cpp`:
- Around line 453-464: Make the cleanup decision exclusive in the flow around
AicpuExecutor::run and the finished_ check: return or otherwise claim a single
“was last thread” result from run(), then call deinit() only for that thread. Do
not let every thread independently observe finished_ and invoke deinit(),
preserving the existing cleanup behavior and shared-state reset.
In `@src/a5/runtime/host_build_graph2/docs/profiling_levels.md`:
- Around line 384-398: Update the compile-time command examples in “Setting
Profiling Macros” to use the documented SIMPLER_DFX, SIMPLER_ORCH_PROFILING, and
SIMPLER_TENSORMAP_PROFILING macros instead of the undefined PTO2_* names, while
preserving the examples’ intended profiling configurations.
In `@src/a5/runtime/host_build_graph2/docs/RUNTIME_LOGIC.md`:
- Around line 199-211: Revise the lifecycle guidance in RUNTIME_LOGIC.md so
reclaiming-ring behavior is explicitly scoped to tensormap_and_ringbuffer-only
and not presented as host_build_graph behavior. Update the sections covering
allocation, slot recycling, heap/TensorMap reclamation, scope-reference
deadlocks, CONSUMED transitions, and last_task_alive advancement, including the
referenced lines, to consistently describe host_build_graph as
whole-graph-resident with no slot recycling or runtime watermark advancement.
- Around line 139-149: Update the shared-memory layout documentation around
PTO2SharedMemoryHeader and pto2_sm_layout::ring_segment_offsets to represent the
single-ring contract used by host_build_graph, including one ring and its
task-window regions. Ensure the accompanying size formula and diagram
consistently describe one whole-graph-resident ring, or explicitly label the
presentation as generic multi-ring behavior where applicable.
- Around line 731-739: Runtime2 documentation incorrectly attributes
device-orchestrated SO loading and timing behavior to host_build_graph. In
src/a5/runtime/host_build_graph2/docs/RUNTIME_LOGIC.md lines 731-739, move the
AICPU dlopen and orchestration-entry flow into a tensormap_and_ringbuffer-only
section; in src/a5/runtime/host_build_graph2/docs/device_log_profiling.md lines
19-35, mark Block 1 as tensormap_and_ringbuffer-only and add host-side timing
for host_build_graph; in
src/a5/runtime/host_build_graph2/docs/profiling_levels.md lines 175-207, remove
device-orchestrator claims from host_build_graph profiling levels or label the
entire section device-orchestrated-only.
In `@src/a5/runtime/host_build_graph2/docs/SCALAR_DATA_ACCESS.md`:
- Around line 3-7: Update the get_tensor_data/set_tensor_data documentation and
examples to scope them to tensormap_and_ringbuffer runtimes that can execute
during orchestration, or explicitly document a separate host execution boundary
that makes blocking waits valid. Ensure the overview no longer implies
host_build_graph can obtain results from device tasks before graph construction
completes.
- Around line 38-45: Align the host_build_graph protocol documentation across
all three sites: in src/a5/runtime/host_build_graph2/docs/SCALAR_DATA_ACCESS.md
lines 38-45, replace the fanout_refcount consumer wait with the inline
host-build consumer-wait contract; in
src/a5/runtime/host_build_graph2/docs/SUBMIT_BY_CLUSTER.md lines 163-170, mark
dep_pool and fanout requirements as future or tensormap_and_ringbuffer-only; and
in src/a5/runtime/host_build_graph2/docs/device_log_profiling.md lines 119-124,
describe completion and readiness using completion_flags, wake lists, and
completed_watermark.
In
`@src/a5/runtime/host_build_graph2/runtime/orchestrator_core/pto_orchestrator.cpp`:
- Around line 430-433: Validate block_num in submit_task() before computing
total_required_subtasks, enforcing a maximum that keeps block_num multiplied by
the active core count within int16_t; reject invalid launch specifications
rather than allowing overflow. Preserve the existing positive-value assertion
and ensure logical_block_num and total_required_subtasks cannot represent
wrapped task counts.
In `@src/a5/runtime/host_build_graph2/runtime/orchestrator_core/pto_runtime2.cpp`:
- Around line 147-204: Guard slot.task before dereferencing it in both
wait_one_producer and wait_one_consumers. Resolve local_id defensively to a safe
diagnostic value when slot.task is null, while preserving the existing wait and
timeout behavior and avoiding null dereferences on entry.
In `@src/a5/runtime/host_build_graph2/runtime/pto_async_kernel_api.h`:
- Around line 76-83: Update defer_flush to derive the flushed byte range from
the end of the completion_entries slab rather than summing optional field sizes.
When completion_entries is present, compute the range from ctx.completion_count
through count DeferredCompletionEntry elements using the entries pointer;
preserve flushing the counter-only range when no entries exist and avoid relying
on the assumed counter/error-code/entries layout.
In `@src/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_completion.cpp`:
- Around line 659-665: Guard the pending-task load in the staging completion
path before dereferencing it: if drain_state_.pending_task.load(...) returns
nullptr, return immediately. Apply this around the logic using slot_state,
including the gated decision, stage_sync_start_cores, and finalize block, while
preserving the existing behavior for non-null task slots.
In `@src/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_dispatch.cpp`:
- Around line 394-408: Replace the 64-bit shift construction with
CoreTracker::BitStates::bit(cluster_offset) in both MIX cluster scan sites:
scheduler_dispatch.cpp lines 394-408 when setting selected_mix_clusters and
lines 745-754 when setting bucket. Use the existing 128-bit-aware helper for
both updates.
In `@src/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_types.h`:
- Around line 512-519: Initialize CoreTracker::cluster_count_ with a safe
default member value so a default-constructed tracker has a valid core count.
Update the cluster_count_ declaration in CoreTracker, preserving init()’s
behavior for explicitly assigned trackers and ensuring the uninitialized
P-thread tracker remains safe when core_num() and core_ids() are read.
---
Minor comments:
In `@src/a5/runtime/host_build_graph2/docs/profiling_levels.md`:
- Around line 420-426: Update the profiling-level summary table to define the
LOG_INFO Count unit explicitly, using the same profiling-only, total-per-run
counting convention established in the preceding sections. Add the correct
counts for levels 0 through 4, including the existing 18, 30, and 34 values, and
clarify that level 0 excludes unrelated diagnostic or progress messages.
In `@src/a5/runtime/host_build_graph2/docs/RUNTIME_LOGIC.md`:
- Around line 768-774: Update the API summary table to match the canonical
declarations by including PTO2Runtime* and num_args in rt_submit_task,
rt_submit_aic_task, and rt_submit_aiv_task. If the entries are intentionally
abbreviated, explicitly label the table as shorthand; otherwise, document each
complete signature consistently with SUBMIT_BY_CLUSTER.md.
In `@src/a5/runtime/host_build_graph2/docs/SUBMIT_BY_CLUSTER.md`:
- Around line 203-214: Update the validation section in SUBMIT_BY_CLUSTER.md so
the existing a2a3 commands are clearly labeled as main-branch baseline tests,
and add A5-specific checks covering host_build_graph2 discovery, build
exclusion, and documentation consistency. Keep the current baseline commands
unchanged while ensuring the added validation explicitly targets the A5
host_build_graph2 scaffold.
In `@src/a5/runtime/host_build_graph2/host/dep_gen_host_graph.cpp`:
- Around line 176-195: Clamp the stored dimension counts in fill_consumer and
fill_producer to MAX_TENSOR_DIMS before write_deps_json uses them for array
lengths, while preserving the bounded copy loops. Apply the same clamping to
slot.ndims at its construction site so the values consumed by the slot
serialization paths cannot exceed the backing array capacity.
In `@src/a5/runtime/host_build_graph2/host/runtime_maker.cpp`:
- Around line 607-633: Ensure the temporary orchestration .so created by
create_orch_so_tempfile is unlinked before every failure return in the dlopen,
orch_func_name dlsym, and framework_bind_runtime dlsym paths. Preserve the
existing successful-path unlink and close handles as currently required.
In
`@src/a5/runtime/host_build_graph2/runtime/backend/sdma/sdma_completion_kernel.h`:
- Around line 94-96: Clarify the handle-zero branch in the completion wrapper
around the existing event.handle check: document that handle == 0 is the
expected synchronous/result-null case after TGET_ASYNC()/TPUT_ASYNC(), while
preserving the safe early return. If handle == 0 is not valid, replace the
silent return with deferred PTO2_ERROR_ASYNC_COMPLETION_INVALID handling
instead.
In `@src/a5/runtime/host_build_graph2/runtime/pto_shared_memory.h`:
- Around line 117-140: Add the <algorithm> header include to pto_shared_memory.h
so std::max used in update_completed_watermark is defined independently of
transitive includes.
In `@src/a5/runtime/host_build_graph2/runtime/pto_submit_types.h`:
- Around line 57-75: Update DispatchPredicate::pass to handle elem_size == 0
before computing or applying the sign-extension shift, treating it as no
predicate or enforcing the valid widths 1, 2, 4, and 8; ensure no 64-bit shift
occurs. Correct the DispatchPredicate layout comment to state that its members
total 18 bytes while sizeof(DispatchPredicate) is 24 bytes due to trailing
alignment padding.
In `@src/a5/runtime/host_build_graph2/runtime/pto_types.h`:
- Around line 285-301: Update Arg::clear() to reset launch_spec along with the
other per-submit fields, ensuring reused arguments restore default block_num and
require_sync_start values before the next submission. Keep reset()’s existing
delegation to clear() so L2TaskArgs::create_from_chip_args receives clean state.
In `@src/a5/runtime/host_build_graph2/runtime/scheduler/pto_scheduler.cpp`:
- Around line 76-82: Update print_stats() to stop iterating over
PTO2_MAX_RING_DEPTH for the shared ring_sched_state.last_task_alive value;
perform one check and print the value once with an appropriate non-per-ring
label, preserving the existing statistics output.
In `@src/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_cold_path.cpp`:
- Around line 976-1029: Update SchedulerContext::deinit() to destroy every queue
in sp_queues_, iterating across all MAX_AICPU_THREADS entries rather than only
active_sched_threads_. Reuse each CompletedTaskQueue’s destroy() operation so
all buffers allocated by init() are released, including queues from prior runs
with higher thread counts.
In `@src/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_completion.cpp`:
- Around line 104-105: Update the scheduler completion path around the mailbox
initialization and push loops to handle a null mailbox consistently: either
assert rt_ is non-null before deriving AICoreCompletionMailbox, or detect a null
mailbox and latch PTO2_ERROR_ASYNC_REGISTRATION_FAILED before reaching the
try_push_condition and try_push_normal_done calls. Ensure neither push loop can
dereference mailbox when it is null.
In `@src/a5/runtime/host_build_graph2/runtime/tensor_create_info.h`:
- Around line 91-92: Update TensorCreateInfo’s default construction so the
documented defaults are guaranteed: initialize start_offset to zero and
is_contiguous to true, while preserving valid initialization for all other
fields consumed by init_tensor_from_create_info. Replace the defaulted
constructor or explicitly value-initialize the relevant members in
TensorCreateInfo.
---
Nitpick comments:
In `@src/a5/runtime/host_build_graph2/aicpu/aicpu_executor.cpp`:
- Around line 184-188: Add SPIN_WAIT_HINT() inside each init barrier polling
loop, including the loops around hs_setup_done_ and the additional ranges at the
referenced locations. Keep the existing atomic checks and return behavior
unchanged, placing the hint on each iteration that continues waiting.
In `@src/a5/runtime/host_build_graph2/common/intrinsic.h`:
- Around line 98-106: Bind SPMD_LOCAL_CONTEXT_INDEX and
SPMD_GLOBAL_CONTEXT_INDEX to the MAX_TENSOR_ARGS and MAX_SCALAR_ARGS derivation
instead of hardcoding 48 and 49. Add a static_assert in an appropriate
translation unit that includes the args-limit definitions, verifying both
context indices remain immediately after the combined argument limits.
In `@src/a5/runtime/host_build_graph2/orchestration/common.cpp`:
- Around line 56-69: Update addr_to_line to shell-quote the executable path when
constructing the addr2line command, ensuring paths containing spaces or shell
metacharacters remain a single literal argument. Preserve the existing command
behavior and accept the current snprintf truncation handling.
In
`@src/a5/runtime/host_build_graph2/runtime/backend/sdma/sdma_completion_kernel.h`:
- Around line 121-143: Update send_request_entry to propagate the
success/failure result from register_pto_async_event, ensuring registration
failures return false in accordance with the documented contract while
preserving the existing deferred flush behavior.
In `@src/a5/runtime/host_build_graph2/runtime/pto_runtime2_types.h`:
- Around line 25-26: Rename the include guards and matching `#endif` comments in
src/a5/runtime/host_build_graph2/runtime/pto_runtime2_types.h (lines 25-26 and
552), pto_orchestrator.h (28-29 and 186), pto_ring_buffer.h (34-35 and 500),
host_tensor_access.h (50-51 and 91), and pto_constants.h (12-13 and 19) to
unique path-qualified a5 host_build_graph2 identifiers, replacing the copied
a2a3 or unqualified guards while keeping each guard’s opening and closing names
consistent.
In `@src/a5/runtime/host_build_graph2/runtime/pto_tensormap.h`:
- Around line 183-198: Add static_assert layout checks for TensorCreateInfo
alongside the existing Tensor mirror assertions, covering the offsets of
start_offset, ndims, dtype, and shapes[] plus the expected 64-byte copied
layout. Keep copy_tensor_create_info’s raw memcpy unchanged and anchor the
checks to the TensorCreateInfo and Tensor mirror definitions.
In `@src/a5/runtime/host_build_graph2/runtime/pto_types.h`:
- Around line 443-453: Update add_scalars to reject a null values pointer when
count is greater than zero, alongside its existing count and capacity
validation, by calling set_error and returning before memcpy; preserve the
existing behavior for zero-count calls and valid inputs.
In `@src/a5/runtime/host_build_graph2/runtime/scheduler/pto_scheduler.cpp`:
- Around line 94-98: Add a compile-time static_assert next to shape_names in the
scheduler logging code to verify its element count equals
PTO2_NUM_RESOURCE_SHAPES, while preserving the existing loop and logging
behavior.
In `@src/a5/runtime/host_build_graph2/runtime/scheduler/pto_scheduler.h`:
- Around line 161-199: Update the profiling push overload in
PTO2ReadyQueueSlot::push to assign slot->task_id_snapshot to 0 when populating
the slot, alongside slot->slot_state and before publishing sequence. Preserve
the existing profiling counters and queue synchronization logic.
In `@src/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_cold_path.cpp`:
- Around line 723-726: Update the validation error in the scheduler
core-assignment path to use “than” and report both the requested
thread_cores_num value and the allowed CoreTracker::MAX_CORE_PER_THREAD value.
Remove the hardcoded 64-core wording while preserving the existing failure
return.
In `@src/a5/runtime/host_build_graph2/runtime/scheduler/scheduler_dispatch.cpp`:
- Around line 900-906: The scheduler timeout-cycle calculation is duplicated in
run_resolution_thread and resolve_and_dispatch. Extract the shared
default-and-override resolution into one small private helper, then have both
methods call it while preserving SCHEDULER_TIMEOUT_CYCLES and the existing
get_scheduler_timeout_ms() override semantics.
In `@src/a5/runtime/host_build_graph2/runtime/shared/pto_runtime2_init.cpp`:
- Around line 261-265: Update the seen-epoch initialization around
PTO2OrchestratorLayout so the memset size and SM address arithmetic both derive
from the same layout-recorded task-window value used when reserving the region.
Replace the independent task_window_size parameter usage in this path, or reuse
a stored reserved byte count, ensuring memset never exceeds the reserved
fanin_seen_epoch region.
- Around line 347-388: Update the prebuilt_layout documentation in
pto_runtime2.h to identify runtime_maker.cpp’s assignment to rt->prebuilt_layout
as the writer. Remove the implication that runtime_init_data_from_layout or
runtime_wire_arena_pointers populate this field indirectly, while preserving the
rest of the field’s documented behavior.
In `@src/a5/runtime/host_build_graph2/runtime/shared/pto_shared_memory.cpp`:
- Around line 164-171: Update the shared-memory header initialization to obtain
task_descriptors_offset from the existing ring_segment_offsets layout helper
instead of manually advancing offset. Remove the unused offset increments for
task descriptors, payloads, and slot state, while preserving the initial aligned
offset only if required by ring_segment_offsets. Use the established helper as
the single source of truth for the ring layout.
In `@src/a5/runtime/host_build_graph2/runtime/shared/runtime.cpp`:
- Around line 97-113: Add the <cstring> header immediately after the runtime.h
include in runtime.cpp so memset in the constructor and std::strncpy used by
set_device_orch_func_name() and set_device_orch_config_name() are declared
directly by this translation unit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ebb3edf to
70564dd
Compare
ChaoWao
changed the title
|
Thanks @coderabbitai — 14 inline findings plus the 13 minor ones in the review summary, all answered inline and resolved. Every one of them lands on code or docs this PR copies verbatim from They are deferred, not skipped, and will be fixed on both trees in a follow-up so the two stay in lockstep — patching only the a5 copy would fork them immediately, which is exactly what this PR exists to prevent. One correction worth recording: the |
|
Tip For best results, initiate chat on the files or code changes.
The parity requirement makes deferral appropriate for this scaffold PR. A local fix in I also confirm the correction to the Critical finding. Would you like me to create a GitHub issue to track the shared follow-up? 🐇✅ ✏️ Learnings added
You are interacting with an AI system. |
c6c0c80 to
24e734a
Compare
a5 host_build_graph was the original standalone graph builder — add_task / add_successor, fixed-size task tables, no TensorMap, no ring, no scopes — while a2a3 host_build_graph is the host-orchestrated variant of tensormap_and_ringbuffer. The two shared a name and nothing else, so every improvement to the host-orchestration path stopped at a2a3. a5 now runs the same tree. The runtime is a byte-for-byte copy of a2a3 host_build_graph except for six files, because most hardware differences are already isolated in src/a5/platform/: - host/runtime_compile_info.cpp — a host-orchestrated runtime dlopens the orchestration .so on the host, so its toolchain is the host g++ on every platform, with no aarch64 cross-compile branch. - runtime/scheduler/scheduler_completion.cpp — a5's PMU collector keys a record by the 32-bit register token AICore wrote into dual_issue_slots, so it takes pmu_aicpu_complete_record with that token alongside the PTO2 task id, where a2a3 takes pmu_aicpu_record_task with the id alone. - runtime/backend/sdma/sdma_completion_scheduler.h — a2a3 invalidates the cache line before each read of the SDMA-written SdmaEventRecord and flushes it after clearing the head, because rtMemcpy is not cache-coherent there. a5's is, so those three calls are dead weight and its copy omits them. - runtime/backend/sdma/sdma_completion_kernel.h — the pto-isa intrinsic header is pto/comm/async/sdma/, which is where it lives; a2a3 still spells it pto/npu/comm/async/sdma/, a path that no longer exists. Nothing includes this header today, which is why that has gone unnoticed. - runtime/runtime.h and runtime/scheduler/scheduler_context.h — RUNTIME_MAX_WORKER is 108 (36 AIC + 72 AIV) rather than a2a3's 72 (24 + 48). It sizes Handshake workers[], so the a2a3 value made prepare_launch_shape reject every run on a5 with "block_dim (36) exceeds RUNTIME_MAX_WORKER (72)". Both values compile, so only execution on a5 silicon exposes this. The SDMA engine itself is identical on both, so nothing else in that backend changes. URMA exists only on a5 and is not carried over: it sits behind PTO_URMA_SUPPORTED, which is defined nowhere in this repo, so a5 host_build_graph registers COUNTER + SDMA completion ops where a5 tensormap_and_ringbuffer registers COUNTER + SDMA + URMA. Nothing else needed porting. The 2257-line diff between the two tensormap_and_ringbuffer trees is 57% comment wording and, in the rest, API refactors, field renames and style cleanups that issue hw-native-sys#1582 already tracks as divergence to reconcile — none of it is an architecture requirement. Rather than review that diff, the tree was copied verbatim and compiled: the compiler named the single real difference. The scene tests move from the old orchestration ABI (add_task / add_successor / record_tensor_pair / orchestration_api.h) to submit_task / Tensor / pto_orchestration_api.h. dump_args, prepared_callable and the new vector_example take the a2a3 host_build_graph versions; paged_attention takes the a5 tensormap_and_ringbuffer one, because AICore kernels are not architecture-neutral in two ways the a2a3 sources trip over: - `Stride` is ambiguous under a5's ccec/pto-isa and must be spelled `pto::Stride`, which g++-15 accepts either way — so a5sim compiles the bare form and only a5 rejects it. - a5 synchronises AIV stages with set_flag/wait_flag; pipe_barrier(PIPE_V) is rejected outright ("the range of 1st parameter must be [4, 6]"), and the a5 paged_attention kernels already carry the correct shape. task_timing_a5hbg existed only to cover the legacy add_task API on a5 and is removed; test_task_timing_e2e.py drops the skip that excluded a5 host_build_graph and now runs its two host_build_graph cases on a5 too. paged_attention's batch=256 Case1 is marked manual, matching the a2a3 host_build_graph case of the same size: host orchestration populates the whole graph before the device schedules, so the ring cannot reclaim mid-orchestration and 16384 slots do not hold ~64K tasks. The a5 tensormap_and_ringbuffer case it came from runs unmarked because a device-side orchestrator reclaims as it goes. Verified: a5sim and a5 onboard both compile clean; all nine a5 scene-test kernels compile under a5 ccec + pto-isa locally (the step CI failed on); a5sim scene tests 35 passed, host_build_graph 8; a2a3sim host_build_graph 17 passed / 4 skipped with task_timing green; cpput 75 passed; pre-commit clean. a5 onboard scene tests are CI-only — this box is a2a3 silicon. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
a5
host_build_graphwas the original standalone graph builder —add_task/add_successor, fixed-size task tables, no TensorMap, no ring, no scopes — while a2a3host_build_graphis the host-orchestrated variant oftensormap_and_ringbuffer. They shared a name and nothing else, so every improvement to the host-orchestration path stopped at a2a3.a5 now runs the same tree.
The runtime differs from a2a3 in exactly four files
Most hardware differences are already isolated in
src/a5/platform/. What remains:host/runtime_compile_info.cpp.soon the host, so its toolchain is the host g++ on every platform — no aarch64 cross-compile branch.runtime/scheduler/scheduler_completion.cppdual_issue_slots, so it takespmu_aicpu_complete_recordwith that token alongside the PTO2 task id; a2a3 takespmu_aicpu_record_taskwith the id alone.runtime/backend/sdma/sdma_completion_scheduler.hSdmaEventRecord, and flushes after clearing the head, becausertMemcpyis not cache-coherent on a2a3. It is on a5, so those three calls are dead weight and a5's copy omits them.runtime/backend/sdma/sdma_completion_kernel.hpto/comm/async/sdma/. a2a3 still spells itpto/npu/comm/async/sdma/, a path that no longer exists — unnoticed because nothing includes this header today.The SDMA engine itself is identical on both chips, so nothing else in that backend changes.
URMA exists only on a5 and is not carried over. It sits behind
PTO_URMA_SUPPORTED, defined nowhere in this repo, so a5host_build_graphregisters COUNTER + SDMA completion ops where a5tensormap_and_ringbufferregisters COUNTER + SDMA + URMA. Adding it would mean shipping code that cannot be exercised; it belongs with whatever change enables the URMA workspace overlay.Why nothing else needed porting
The diff between the two
tensormap_and_ringbuffertrees is 2257 lines, which looks like a large porting job. It isn't:get_ready_tasks_batch(queues, shape, …)→(shape, …)), field renames (block_idx→s_block_idx), and style cleanups — exactly the divergence issue [Code Health] Reconcile a2a3 vs a5 tensormap_and_ringbuffer runtime implementation divergence #1582 already tracks. None of it is an architecture requirement.runtime/carries no a2a3-vs-a5 conditional compilation, and the one arch-specific helper it references (compute_allowed_cpus) lives insrc/a5/platform/onboard/host/aicpu_topology_probe.*.So the tree was copied verbatim and compiled. The compiler reported exactly one error — the PMU difference. A three-way merge was tried first and rejected: it silently applied 1529 lines of the a5 tree's own refactors with no conflict marker, including deleting comment blocks the host-orchestration path still needs.
Compile-driven porting has a blind spot, though: it cannot see code behind a disabled CMake option. Both SDMA differences above sit in that blind spot (
SIMPLER_ENABLE_PTO_SDMA_WORKSPACEisOFFon a5) and were found by inspection, not by the compiler.AICore kernels are not architecture-neutral
The first push assumed they were, and a5 onboard caught two ways that is false. Both are invisible to a5sim, which compiles kernels with g++-15 rather than ccec:
Strideis ambiguous under a5's ccec/pto-isa and must be spelledpto::Stride. g++-15 accepts the bare form, so only a5 rejects it.set_flag/wait_flag.pipe_barrier(PIPE_V)is rejected outright —error: the range of 1st parameter must be [4, 6].So
paged_attentiontakes the a5tensormap_and_ringbufferkernels, which already carry the correct shape;dump_args,prepared_callableand the newvector_exampletake the a2a3host_build_graphversions withStridequalified. All nine kernels now compile under a5 ccec + pto-isa, verified locally by drivingKernelCompiler(platform="a5")directly — the exact step CI failed on.task_timing
tests/st/task_timing/task_timing_a5hbg/existed only to cover the legacyadd_taskAPI on a5, andtest_task_timing_e2e.pyskipped a5host_build_graphfor the same reason. With the ABI unified, the dedicated test is removed and the skip dropped — the twohost_build_graphe2e cases now run on a5 as well.Testing
KernelCompiler(platform="a5"))tests/st/a5+examples/a5+task_timing) — 35 passed, of whichhost_build_graph8host_build_graph+task_timing— 17 passed / 4 skippeda5 onboard scene tests run in CI only; this development box is a2a3 silicon.