Refactor: extract sim host shared base + c_api glue (-2800 lines)#932
Refactor: extract sim host shared base + c_api glue (-2800 lines)#932hw-native-sys-bot wants to merge 1 commit into
Conversation
Mirror the onboard host refactor (hw-native-sys#880…hw-native-sys#928) on the sim path. Pulls ~2800 duplicate lines out of src/{a2a3,a5}/platform/sim/host/ into a shared SimDeviceRunnerBase + c_api_shared.cpp + memory_allocator.cpp under src/common/platform/sim/host/. Per-arch DeviceRunner keeps only the bits that genuinely differ between a2a3 and a5 sim: - aicore_execute signature (a5 has extra aicore_pmu_ring_addrs arg) - dlsym'd function-pointer table (a2a3 has dep_gen / pmu_reg_addrs / aicore_rotation_table setters; a5 doesn't) - init_* alloc strategy (a2a3 uses mem_alloc_ via captured lambdas; a5 uses std::malloc via prof_alloc_cb static functions — preserved as-is, no behavior change) - finalize() collector semantics (a2a3 releases shm to mem_alloc_ per-run; a5 stop()s per-run, full finalize at run-end via prof_free_cb — preserved as-is) - run() middle (dep_gen gating on a2a3 only; different SIM_REG_* constants) SimDeviceRunnerBase hosts the byte-identical methods + their state: setup_static_arena, acquire_pooled_*, create_thread, attach_current_thread, allocate_tensor / free_tensor / copy_*, register_callable[_host_orch], unregister_callable, has_callable, bind_callable_to_runtime, prepare_orch_so, upload_chip_callable_buffer, print_handshake_results, release_callable_state, ensure_device_initialized, set_*_enabled / output_prefix accessors, last_device_wall_ns, the shared mem_alloc_ / gm_*_arena_ / callable maps / chip_callable buffer pool / collector instances / kernel_args_ / device_wall ptr / log/dlopen counters. Mechanical-fix: setup_static_arena standardized to "release all arenas on any failure" (matches a2a3 sim + onboard PR hw-native-sys#922). a5 sim had been keeping earlier-committed peers alive on later-region failure; the new common impl drops that to match the onboard invariant. Latent-fix carried over from onboard hw-native-sys#928: c_api_shared's run_prepared wraps the placement-new'd Runtime in RAIIScopeGuard so its dtor fires on every exit path (manual r->~Runtime() in the prior sim c_api was bypassed by catch(...) on exception). Polymorphism via SimDeviceRunnerBase virtuals: ~SimDeviceRunnerBase (public), run(), finalize(), set_dep_gen_enabled() (default no-op, a2a3 overrides). c_api_shared.cpp works through SimDeviceRunnerBase * and dispatches via those virtuals — same pattern as the onboard hw-native-sys#928 split. Per-arch pto_runtime_c_api.cpp shrinks from ~420 lines to ~55 (just create_device_context + ACL stubs). memory_allocator.cpp was byte-identical, deleted from both arch subdirs and lives once in common/. Both arches built clean. nm -D verifies all 17 ChipWorker dlsym targets exported on both sim libhost_runtime.so. ST passes 38/38 on a2a3sim L1+L2 (devices 0,1) and 22/22 on a5sim L1+L2 (devices 0,1); examples spot-checked (scalar_data_test, vector_example, benchmark_bgemm on a2a3sim; vector_example, bgemm on a5sim). Co-authored-by: Chao Wang <26245345+ChaoWao@users.noreply.github.com>
|
Warning Review limit reached
More reviews will be available in 59 minutes and 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
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 simulation host runtime for both the a2a3 and a5 architectures by extracting shared logic—including the DeviceRunner base class, C API wrappers, and memory allocation—into a common directory. This reduces code duplication and aligns the simulation platform with the onboard refactoring pattern. Feedback on the changes points out a potential null pointer dereference in the shared C API implementation where current_runner() is dereferenced without a defensive null check, which could lead to crashes in unbound threads.
| static void *device_malloc(size_t size) { | ||
| try { | ||
| return current_runner()->allocate_tensor(size); | ||
| } catch (...) { | ||
| return NULL; | ||
| } | ||
| } |
There was a problem hiding this comment.
If current_runner() returns nullptr (e.g., if called from an unbound thread or during unexpected execution states), dereferencing it directly on line 73 will result in a null pointer dereference and cause a crash.
Add a defensive null check to ensure that current_runner() is valid before calling allocate_tensor on it. The same defensive check should be applied to other static callbacks such as device_free, copy_to_device, copy_from_device, etc.
static void *device_malloc(size_t size) {
try {
auto *runner = current_runner();
if (runner == nullptr) {
LOG_ERROR("device_malloc: current_runner is null");
return NULL;
}
return runner->allocate_tensor(size);
} catch (...) {
return NULL;
}
}2 participants
Summary
Mirror the onboard host refactor (#880…#928) on the sim path. Pulls ~2800 duplicate lines out of
src/{a2a3,a5}/platform/sim/host/into a sharedSimDeviceRunnerBase+c_api_shared.cpp+memory_allocator.cppundersrc/common/platform/sim/host/. Per-archDeviceRunnerkeeps only the bits that genuinely differ between a2a3 and a5 sim.What moved to
src/common/platform/sim/host/device_runner_base.{h,cpp}— shared base class hosting:setup_static_arena,acquire_pooled_*create_thread,attach_current_thread,ensure_device_initializedallocate_tensor/free_tensor/copy_to_device/copy_from_deviceregister_callable[_host_orch],unregister_callable,has_callable,bind_callable_to_runtimeprepare_orch_so,upload_chip_callable_bufferprint_handshake_results,release_callable_statemem_alloc_/gm_*_arena_/ callable maps / chip_callable buffer pool / collector instances /kernel_args_/ device_wall ptr / log/dlopen countersc_api_shared.cpp— TSD glue + static wrappers + public C ABI (simpler_init,prepare_callable,run_prepared,device_*_ctx,finalize_device, etc.) all written againstSimDeviceRunnerBase *. Same polymorphic pattern as onboard PR Refactor: extract pto_runtime_c_api shared glue into common (-303 lines) #928.memory_allocator.cpp— was byte-identical between arches.What stays per-arch
aicore_executesignature — a5 has an extraaicore_pmu_ring_addrsargpmu_reg_addrs/aicore_rotation_tablesetters; a5 doesn'tinit_*alloc strategy — a2a3 usesmem_alloc_via captured lambdas; a5 usesstd::mallocviaprof_alloc_cbstatics. Preserved as-is; no behavior change in this PR.finalize()collector semantics — a2a3 releases shm tomem_alloc_per-run; a5stop()s per-run, full finalize at run-end viaprof_free_cb. Preserved as-is.run()middle — dep_gen gating on a2a3 only; differentSIM_REG_*constantsPer-arch
pto_runtime_c_api.cppShrinks from ~420 lines to ~55 — just
create_device_context(which needs the concreteDeviceRunnertype) + ACL no-op stubs.Mechanical fixes carried with the move
setup_static_arenastandardized to "release all arenas on any failure" (matches a2a3 sim + onboard PR Refactor: extract setup_static_arena + launch_aicore_kernel into base #922). a5 sim had been keeping earlier-committed peers alive on later-region failure; the common impl drops that to match the onboard invariant.c_api_shared'srun_preparedwraps the placement-new'dRuntimeinRAIIScopeGuardso its dtor fires on every exit path (manualr->~Runtime()in the prior sim c_api was bypassed bycatch(...)on exception).Polymorphism
SimDeviceRunnerBasevirtuals:~SimDeviceRunnerBase(public),run,finalize,set_dep_gen_enabled(default no-op, a2a3 overrides).c_api_shared.cppworks throughSimDeviceRunnerBase *and dispatches via those virtuals — same pattern as onboard #928.Test plan
nm -Dverifies all 17 ChipWorker dlsym targets exported on both sim libhost_runtime.so