Fix: state the post-B4 runner geometry, stream, and TLS contracts - #1653
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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 |
hw-native-sys#1587 moved three contracts without moving the text that described them, and left one failure mode expressed as a noexcept violation. Geometry. resolve_block_dim() and prepare_launch_shape() no longer write block_dim_ or worker_count_; activate_launch_shape() latches both on the executor thread immediately before run(). The comment and the LOG_ERROR in each onboard run() still named prepare_launch_shape, so the one diagnostic a future reader greps pointed at a function that latches nothing. hw-native-sys#1521 later edited the line directly below that comment and left it standing, which is how a stale comment survives. The simulation runners keep their wording: SimDeviceRunnerBase::prepare_launch_shape does still assign block_dim_. Streams. RunStreamSlots became a two-thread class when native prepare started provisioning the successor's slot while the executor retires the predecessor's. Per-slot handles are safe — admission gives each slot one owner — but created_count_ is shared across owners and is also read from an unrelated thread through get_run_stream_set_create_count, so it is now atomic and the ownership rule is stated on the class. Thread selection. restore_native_run_thread_selection was noexcept while run_selection() could throw: on a thread created by create_thread the per-thread block does not exist yet, so installation allocates. Split out a non-throwing try_run_selection() and let restore abort with a message on the unrecoverable path. Returning instead would leave the thread on the default slot and bank, addressing storage another lease owns, and a freshly started thread has no channel to report the failure through. B6c removes the mechanism outright; until then the failure is diagnosable rather than a bare terminate. Symbol loading. Since every required pipeline symbol became a strict load, the dominant cause of a dlsym failure is a host runtime out of sync with the tree that consumes it. Say so in the error, which otherwise reports only the missing name. Also spell the successor-already-staged test as occupied > 1, since the loop above it has already rejected every predecessor that may not carry one, and record that simulation discards native-run identity by design.
098e55c to
90fb9e5
Compare
Rebase the common active-plus-prepared ownership on current main without weakening the uniform pipeline ABI from hw-native-sys#1587 or the runner geometry, stream, and TLS contracts from hw-native-sys#1653. Add generation-bound direct L2 RunHandles, bounded two-slot admission, launch-only acceptance waiting, and deterministic depth-one fallback while preserving HBG inactive-bank preparation. Remove timing-dependent endpoint assertions and keep RequestSession absent.
1 participant
Summary
Follow-up to #1587. It moved three contracts without moving the text that described them, and left one failure mode expressed as a
noexceptviolation.Geometry.
resolve_block_dim()andprepare_launch_shape()no longer writeblock_dim_/worker_count_;activate_launch_shape()latches both on the executor thread immediately beforerun(). Both onboardrun()bodies still carried a comment and aLOG_ERRORnamingprepare_launch_shape, so the one diagnostic a future reader greps pointed at a function that latches nothing. #1521 later edited the line directly below that comment and left it standing.The simulation runners are deliberately untouched:
SimDeviceRunnerBase::prepare_launch_shapedoes still assignblock_dim_, so their wording is correct. The two remaining references —call_config.hand the HBGruntime_maker.cpp— are also correct, sinceprepare_launch_shapestill publishesworker_countonto theRuntime.Streams.
RunStreamSlotsbecame a two-thread class when native prepare started provisioning the successor's slot while the executor retires the predecessor's. Per-slot handles are safe — admission gives each slot one owner — butcreated_count_is shared across owners and is also read from an unrelated thread throughget_run_stream_set_create_count(and asserted byrun_stream_reuse). It is nowstd::atomic<size_t>, and the ownership rule is stated on the class.Thread selection.
restore_native_run_thread_selectionwasnoexceptwhilerun_selection()could throw: on a thread created bycreate_threadthe per-thread block does not exist yet, so installation allocates. Split out a non-throwingtry_run_selection()and let restore abort with a message on the unrecoverable path. Returning instead would leave the thread on the default slot and bank, addressing storage another lease owns, and a freshly started thread has no channel to report the failure through.Symbol loading. Since every required pipeline symbol became a strict load in #1587, the dominant cause of a
dlsymfailure is a host runtime out of sync with the tree that consumes it. The error now says so instead of reporting only the missing name.Two readability items alongside:
occupied != 0 && occupied != 1becomesoccupied > 1(the loop above it has already rejected every predecessor that may not carry a successor), and the simulationset_native_run_identity_ctxrecords that it discards identity by design.No behavior change outside the abort path.
B6cin the pipeline plan removes the TLS mechanism outright; until then the failure is diagnosable rather than a bare terminate.Testing
pre-commiton every changed file — all hooks pass (clang-format, clang-tidy, cpplint, headers, English-only)tests/st/a2a3: HBG 17 passed / 4 skipped, TMR 38 passed, L3 resource phase all PASStask-submit, devices 3+5: 7 passed acrossworker_async_fifo,worker_async_endpoint,native_run_lifecycle, andrun_stream_reuse— the last is the direct consumer ofcreated_countst-onboard-a5(the local a5 pool was unavailable); the a5 change is a comment and a log string in a path a5 shares with a2a3