Fix: atomic acquire/release for sim register handshake gate (a2a3 + a5)#1081
Conversation
![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 introduces synchronizing register accessors (reg_load_acquire and reg_store_release) to enforce atomic acquire/release semantics in CPU simulation (__CPU_SIM) environments, preventing race conditions during AICPU<->AICore handshakes while preserving legacy volatile access on hardware. The feedback highlights that declaring these header functions as static inline violates the One Definition Rule (ODR) when called from inline functions with external linkage, and can cause binary size bloat. It is recommended to remove the static keyword and use inline alone.
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.
|
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:
📝 WalkthroughWalkthroughIntroduces ChangesSim Register Memory-Ordering Fix
Sequence Diagram(s)sequenceDiagram
participant AICore as AICore thread (inner_kernel.h)
participant RegCell as Simulated register cell
participant Scheduler as AICPU Scheduler<br/>(check_running_cores_for_completion)
note over AICore,RegCell: FIN publication
AICore->>RegCell: __atomic_store_n(cell, FIN_VALUE, __ATOMIC_RELEASE)
note over Scheduler,RegCell: Completion polling
Scheduler->>RegCell: reg_load_acquire(core.cond_ptr)
RegCell-->>Scheduler: reg_val (acquire load — sees FIN_VALUE)
Scheduler->>Scheduler: evaluate reg_val for FIN condition
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🧹 Nitpick comments (1)
src/a2a3/platform/shared/aicpu/platform_regs.cpp (1)
17-28: ⚡ Quick winUpdate the file-level ordering note for sim atomics.
Line 56 now delegates to
reg_load_acquire, which is atomic acquire under__CPU_SIM; the header still says a2a3sim uses volatile-only register access.📝 Proposed documentation sync
- * 2. Register read/write operations (volatile MMIO, no barrier) + * 2. Register read/write operations (hardware volatile MMIO; sim acquire/release register-cell atomics) @@ - * Ordering: read_reg / write_reg emit only the volatile MMIO load/store. - * ARM64 Device-nGnRnE memory orders accesses within the same region; cross - * Device <-> Normal-cacheable ordering is the caller's responsibility - * (wmb() before a publishing register write, rmb() after observing a - * register hand-off bit). + * Ordering: on hardware, read_reg / write_reg emit only the volatile MMIO + * load/store. ARM64 Device-nGnRnE memory orders accesses within the same + * region; cross Device <-> Normal-cacheable ordering is the caller's + * responsibility (wmb() before a publishing register write, rmb() after + * observing a register hand-off bit). In a2a3sim, platform_regs.h routes + * register cells through acquire/release atomics because simulated registers + * are host memory shared across threads. @@ - * - a2a3sim: Volatile pointer access to host-allocated simulated registers + * - a2a3sim: Acquire/release register-cell access to host-allocated simulated registersThis follows the helper contract in
src/a2a3/platform/include/aicpu/platform_regs.h:96-122.Also applies to: 56-58
🤖 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/platform/shared/aicpu/platform_regs.cpp` around lines 17 - 28, Update the file-level documentation comment in the Platform Support section (the comment block describing a2a3sim behavior) to reflect that a2a3sim register access now delegates to reg_load_acquire which uses atomic acquire semantics under __CPU_SIM, rather than stating it uses only volatile pointer access. The documentation should align with the actual implementation behavior at lines 56-58 where reg_load_acquire is now called.
🤖 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 @.github/workflows/sanitizers.yml:
- Around line 16-20: The `pull_request:` trigger at line 20 in the sanitizers
workflow with an inline TEMPORARY comment is a high-risk pattern that relies on
manual cleanup and could easily be merged by accident. Replace the
`pull_request:` trigger with `workflow_dispatch:` to make sanitizer jobs
on-demand only, ensuring this validation runs only when explicitly triggered via
the GitHub UI Actions tab rather than on every PR, which aligns with the
documented "Not a PR gate" design and eliminates the risk of accidental merge.
---
Nitpick comments:
In `@src/a2a3/platform/shared/aicpu/platform_regs.cpp`:
- Around line 17-28: Update the file-level documentation comment in the Platform
Support section (the comment block describing a2a3sim behavior) to reflect that
a2a3sim register access now delegates to reg_load_acquire which uses atomic
acquire semantics under __CPU_SIM, rather than stating it uses only volatile
pointer access. The documentation should align with the actual implementation
behavior at lines 56-58 where reg_load_acquire is now called.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e90869f6-ecb6-4fdf-a6d2-195715feb1f3
📒 Files selected for processing (11)
.github/workflows/sanitizers.ymlsrc/a2a3/platform/include/aicpu/platform_regs.hsrc/a2a3/platform/shared/aicpu/platform_regs.cppsrc/a2a3/platform/sim/aicore/inner_kernel.hsrc/a2a3/platform/sim/aicpu/CMakeLists.txtsrc/a2a3/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cppsrc/a5/platform/include/aicpu/platform_regs.hsrc/a5/platform/sim/aicore/inner_kernel.hsrc/a5/platform/sim/aicpu/CMakeLists.txtsrc/a5/platform/sim/aicpu/inner_platform_regs.cppsrc/a5/runtime/tensormap_and_ringbuffer/runtime/scheduler/scheduler_completion.cpp
dc04be0 to
8719a78
Compare
Sanitizer validation (temporary
|
| main (pre-fix) | this PR (fixed) | |
|---|---|---|
| ThreadSanitizer data races | 28 (5 at the COND/dispatch handshake lines) | 0 |
stall (TIMEOUT_EXIT / run_prepared -> -100) |
intermittently present | none |
| test result | flaky (nightly red) | 2 passed |
Flakiness probe — Sanitizers re-run 5x on this PR:
| run | tsan/a2a3sim | tsan/a5sim |
|---|---|---|
| 1 | ✅ 0 races | ✅ 0 races |
| 2 | ✅ 0 races | ✅ 0 races |
| 3 | ✅ 0 races | ✅ 0 races |
| 4 | ✅ 0 races | ✅ 0 races |
| 5 | ✅ 0 races | ✅ 0 races |
All four sanitizer jobs (asan/tsan × a2a3sim/a5sim) green every time.
Workflow run: https://github.com/hw-native-sys/simpler/actions/runs/27681815742
This is consistent by construction: once the register cell is an atomic
release/acquire pair on the same address, TSAN cannot report a race between the
AICore store and the AICPU load, and the missed-FIN stall (the downstream
failure mode) cannot occur.
3ea4089 to
f70dcc5
Compare
Update: implementation refactored to the per-variant splitFollowing review,
(the same sim/onboard split already used for Re-validated on x86 CI with the refactored code (sanitizers temporarily enabled
All four sanitizer jobs green. Run: |
f70dcc5 to
7674b75
Compare
The AICPU<->AICore handshake registers (COND, DATA_MAIN_BASE) were accessed in the sim platform as plain `volatile` loads/stores ordered only by standalone fences (OUT_OF_ORDER_*_BARRIER / rmb / wmb). In sim those "registers" are plain host memory shared across the AICore and AICPU host threads, so a bare fence beside a non-atomic access does not establish inter-thread happens-before in the C++ memory model -- and ThreadSanitizer correctly flags every COND/dispatch access as a data race. The unsynchronized COND poll can also miss the AICore's FIN publish, leaving a task stuck RUNNING (cond_reg_state=ack) until the scheduler idle watchdog fires (TIMEOUT_EXIT, sched_error_code=100, run_prepared -> -100) -- the nightly tsan/a2a3sim failure on test_dedup_shared_so_independent_unregister. Make the register cell itself the synchronizing object: - AICore sim read_reg/write_reg -> __atomic_load_n(ACQUIRE) / __atomic_store_n(RELEASE) (sim-only inner_kernel.h). - AICPU side: add reg_load_acquire/reg_store_release, declared in platform_regs.h and implemented per-variant -- atomic in sim/aicpu/inner_platform_regs.cpp, plain Device-MMIO in onboard/aicpu/inner_platform_regs.cpp (the same sim/onboard split already used for read_reg/write_reg, no new macro gate). write_reg, the shared read_reg (a2a3), and the hot-path `*core.cond_ptr` completion poll route through them. Onboard is unchanged: its per-variant accessors are the plain volatile MMIO load/store with the existing explicit rmb()/wmb() at call sites (atomics on Device-nGnRnE memory are not valid). The atomic release/acquire subsumes the now-removed per-access fences on the sim path. Validated on x86 CI (sanitizers): main reports 28 ThreadSanitizer data races (5 at the handshake lines) + the stall; this change reports zero across 5 tsan re-runs, all jobs green.
Onboard benchmark: no measurable performance impact (within noise)Since this is a sim-only change (the onboard accessor Setup: a2a3, Per-window breakdown, Base → HEAD (us), with Δ%. The changed line lives in the completion poll → Sched window; Orch (graph-build) does not run the poll and is byte-identical between builds — it's the control column.
The two >2% cases are contention noise, not the change — now visible directly in the table:
Verdict: stable cases sit in the −0.4% … +0.1% noise band on Sched/Effective; the short-kernel scatter is common-mode contention (Orch self-evidently moves with it). Consistent with the change being bit-identical onboard — no performance regression. |
2 participants
Problem
The nightly tsan / a2a3sim sanitizer job intermittently fails on
test_dedup_shared_so_independent_unregisterwith a scheduler stall:Root cause is not a too-small timeout and is unrelated to the
sync_start drain fix (#1074). The AICPU<->AICore handshake registers
(
COND,DATA_MAIN_BASE) are accessed in the sim platform as plainvolatileloads/stores ordered only by standalone fences(
OUT_OF_ORDER_*_BARRIER/rmb/wmb). In sim those "registers" areplain host memory shared across the AICore and AICPU host threads, so a
bare fence beside a non-atomic access establishes no inter-thread
happens-before in the C++ memory model — ThreadSanitizer correctly flags
every COND/dispatch access as a data race, and the unsynchronized COND
poll can miss the AICore's FIN publish, leaving a task stuck
RUNNING(
cond_reg_state=ack) until the idle watchdog fires.Fix
Make the register cell itself the synchronizing object on the sim path:
read_reg/write_reg→__atomic_load_n(ACQUIRE)/__atomic_store_n(RELEASE)(sim-onlyinner_kernel.h).reg_load_acquire/reg_store_releaseinplatform_regs.h, gated on__CPU_SIM(now defined on the sim aicputarget).
write_reg, the sharedread_reg(a2a3) /inner_platform_regs.cpp(a5), and the hot-path*core.cond_ptrcompletion poll route through them.
Onboard is unchanged:
__CPU_SIMis undefined there, so theaccessors stay plain Device-MMIO
volatile+ the existing explicitrmb()/wmb()(atomics on Device-nGnRnE memory are not valid). Theatomic release/acquire subsumes the now-removed per-access fences on the
sim path only.
Validation
Built with
SIMPLER_SANITIZER=tsanand ran the previously-failing test40x on a2a3sim under TSAN:
(previously dozens at these exact lines:
aicore_executor.cpp,handshake_all_cores,check_running_cores_for_completion,platform_regs).libaicpu_kernel.sonow contains__tsan_atomic_*calls,confirming the accesses became instrumented atomics.
Notes
src/{a2a3,a5}/platform/sim+the sim register-cell accessor and one completion-poll line.
bind_runtime/on_orchestration_doneseen in the original failing log are a separate pre-existing issue
(scheduler init/teardown, not the register handshake) and are not
addressed here.