Add: select A5 AICPU cores from runtime topology - #1643
Open
yanghaoran29 wants to merge 1 commit into
Open
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:
📝 WalkthroughWalkthroughThe PR adds a production AICPU topology query, occupancy-aware host probing, scenario-specific CPU selection, unknown-topology fallback, runtime thread-count adjustment, and diagnostic JSON output for A5. ChangesA5 AICPU topology flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DeviceRunner
participant AICPUQuery
participant AscendHAL
participant TopologyProbe
participant AICPUThreads
DeviceRunner->>AICPUQuery: query device occupancy
AICPUQuery->>AscendHAL: request occupancy metrics
AscendHAL-->>AICPUQuery: return topology values
AICPUQuery-->>DeviceRunner: return occupancy result
DeviceRunner->>TopologyProbe: classify topology and select CPUs
TopologyProbe-->>DeviceRunner: return affinity and effective count
DeviceRunner->>AICPUThreads: launch full OCCUPY population
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: 2
🧹 Nitpick comments (4)
src/a5/platform/onboard/host/aicpu_topology_probe.cpp (2)
483-485: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturn a value tuple from
topology_key.
std::tiereturnsstd::tuple<const int32_t&, ...>bound to the members ofcpu. All current callers compare the result inside the same full expression, so the references stay valid. The signature is still fragile. A future caller that stores the result, or that passes a temporaryAicpuLogicalCpu, gets dangling references with no compiler diagnostic. The members are fiveint32_t, so a value tuple costs nothing.♻️ Proposed change to return a value tuple
auto topology_key(const AicpuLogicalCpu &cpu) { - return std::tie(cpu.die_id, cpu.cluster_id, cpu.phy_cpu_id, cpu.hyperthread_id, cpu.cpu_id); + return std::make_tuple(cpu.die_id, cpu.cluster_id, cpu.phy_cpu_id, cpu.hyperthread_id, cpu.cpu_id); }🤖 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/platform/onboard/host/aicpu_topology_probe.cpp` around lines 483 - 485, Update topology_key to return a value tuple containing the five int32_t topology fields instead of using std::tie, ensuring results remain valid when stored or when the input is temporary.
676-694: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEscape
soc_namebefore you write it into the JSON string.Line 683 interpolates
topology.soc_namedirectly between quotes. The value comes fromaclrtGetSocName()throughquery_soc_name(), so this code does not control its content. A"or\in that value produces malformed JSON for every consumer of--jsonoutput. Observed SoC names are alphanumeric, so this is a robustness gap and not a current failure.A related point on line 679-681: the ternary chain uses
"sequential_fallback"as the catch-all. If a fourthAicpuSelectionPolicyenumerator is added, the output silently reports the wrong policy. Aswitchgives you a compiler warning instead.♻️ Proposed fix for escaping and the policy mapping
+std::string json_escape(const std::string &value) { + std::string out; + out.reserve(value.size()); + for (char c : value) { + if (c == '"' || c == '\\') { + out += '\\'; + out += c; + } else if (static_cast<unsigned char>(c) < 0x20) { + char buf[7]; + std::snprintf(buf, sizeof(buf), "\\u%04x", static_cast<unsigned char>(c)); + out += buf; + } else { + out += c; + } + } + return out; +} + std::string format_aicpu_topology_json( const AicpuTopology &topology, AicpuSelectionPolicy policy, const std::vector<int32_t> &allowed_cpus ) { - const char *policy_name = policy == AicpuSelectionPolicy::kScenario ? "scenario" : - policy == AicpuSelectionPolicy::kGeneric ? "generic" : - "sequential_fallback"; + const char *policy_name = "sequential_fallback"; + switch (policy) { + case AicpuSelectionPolicy::kScenario: + policy_name = "scenario"; + break; + case AicpuSelectionPolicy::kGeneric: + policy_name = "generic"; + break; + case AicpuSelectionPolicy::kSequentialFallback: + policy_name = "sequential_fallback"; + break; + } std::ostringstream out; - out << "{\n \"architecture\": \"a5\",\n \"soc_name\": \"" << topology.soc_name << "\",\n \"scenario_type\": \"" + out << "{\n \"architecture\": \"a5\",\n \"soc_name\": \"" << json_escape(topology.soc_name) + << "\",\n \"scenario_type\": \""🤖 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/platform/onboard/host/aicpu_topology_probe.cpp` around lines 676 - 694, Update format_aicpu_topology_json to JSON-escape topology.soc_name before inserting it into the quoted "soc_name" field, including quotes, backslashes, and other required control characters. Replace the policy_name ternary in format_aicpu_topology_json with an exhaustive switch over AicpuSelectionPolicy so newly added enumerators are diagnosed rather than silently mapped to sequential_fallback.tools/cann-examples/aicpu-device-query/host/CMakeLists.txt (1)
44-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the driver library directory overridable.
Line 47 hardcodes
/usr/local/Ascend/driver/lib64/driverwhile every other search path in this file derives from${ASCEND_HOME_PATH}. On a host with a non-default driver install, theascend_hallink on line 52 fails and the tool cannot be built. A cache variable keeps the default and lets the builder override it.♻️ Proposed change
+set(ASCEND_DRIVER_LIB_DIR "/usr/local/Ascend/driver/lib64/driver" + CACHE PATH "Directory containing libascend_hal.so") + target_link_directories(query_device_hal PRIVATE ${ASCEND_HOME_PATH}/lib64 ${ASCEND_HOME_PATH}/runtime/lib64 - /usr/local/Ascend/driver/lib64/driver + ${ASCEND_DRIVER_LIB_DIR} )🤖 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 `@tools/cann-examples/aicpu-device-query/host/CMakeLists.txt` around lines 44 - 48, Update the target_link_directories configuration for query_device_hal to replace the hardcoded driver path with a CMake cache variable that defaults to /usr/local/Ascend/driver/lib64/driver, allowing builders to override the driver library directory while preserving the current default.src/a5/platform/onboard/host/device_runner.cpp (1)
282-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the
5that selects the 4S+1O policy.The literal couples this dispatch to
compute_scenario_allowed_cpus, which always returns exactly five CPUs in[S0, S1, S2, S3, O]order. The relationship is not visible at the call site. A named constant documents why any other requested count falls through to the genericcompute_allowed_cpuspath.♻️ Proposed change
+// compute_scenario_allowed_cpus implements the fixed 4-scheduler + 1-orchestrator +// policy, so it applies only when the caller requests exactly that many threads. +constexpr int kScenarioPolicyThreadCount = 5;- } else if (requested_aicpu_num == 5) { + } else if (requested_aicpu_num == kScenarioPolicyThreadCount) {🤖 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/platform/onboard/host/device_runner.cpp` around lines 282 - 289, Replace the literal requested_aicpu_num == 5 check in the device runner dispatch with a named constant representing the 4S+1O policy CPU count, defined in the appropriate nearby scope. Use that constant when selecting compute_scenario_allowed_cpus so the fixed five-CPU relationship is explicit while leaving the generic fallback unchanged.
🤖 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/platform/onboard/host/device_runner.cpp`:
- Around line 162-172: Update the preflight AICPU launch and synchronization
failure branches in the device occupancy query to call
recover_device_or_mark_unusable(rc) before returning. Apply this to both
launch_aicpu_payload and aclrtSynchronizeStreamWithTimeout failures, preserving
the existing error logging and return behavior.
In `@tools/cann-examples/aicpu-device-query/host/query_device_hal.cpp`:
- Around line 498-505: Replace the magic indices used in the JSON occupancy
mapping within the query-device flow with named constants representing the
OS_SCHED, OCCUPY, and PF_OCCUPY request positions. Use those constants
consistently for both value and validity assignments, and define them alongside
the requests list so changes to request ordering remain explicit and
synchronized.
---
Nitpick comments:
In `@src/a5/platform/onboard/host/aicpu_topology_probe.cpp`:
- Around line 483-485: Update topology_key to return a value tuple containing
the five int32_t topology fields instead of using std::tie, ensuring results
remain valid when stored or when the input is temporary.
- Around line 676-694: Update format_aicpu_topology_json to JSON-escape
topology.soc_name before inserting it into the quoted "soc_name" field,
including quotes, backslashes, and other required control characters. Replace
the policy_name ternary in format_aicpu_topology_json with an exhaustive switch
over AicpuSelectionPolicy so newly added enumerators are diagnosed rather than
silently mapped to sequential_fallback.
In `@src/a5/platform/onboard/host/device_runner.cpp`:
- Around line 282-289: Replace the literal requested_aicpu_num == 5 check in the
device runner dispatch with a named constant representing the 4S+1O policy CPU
count, defined in the appropriate nearby scope. Use that constant when selecting
compute_scenario_allowed_cpus so the fixed five-CPU relationship is explicit
while leaving the generic fallback unchanged.
In `@tools/cann-examples/aicpu-device-query/host/CMakeLists.txt`:
- Around line 44-48: Update the target_link_directories configuration for
query_device_hal to replace the hardcoded driver path with a CMake cache
variable that defaults to /usr/local/Ascend/driver/lib64/driver, allowing
builders to override the driver library directory while preserving the current
default.
🪄 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 Plus
Run ID: 82f4f0d8-f319-482a-9e81-bffd2ebdee27
📒 Files selected for processing (16)
docs/design/a5-fg-pg-core-selection.en.mdsrc/a5/docs/hardware.mdsrc/a5/platform/include/common/kernel_args.hsrc/a5/platform/onboard/aicpu/CMakeLists.txtsrc/a5/platform/onboard/aicpu/kernel.cppsrc/a5/platform/onboard/host/aicpu_topology_probe.cppsrc/a5/platform/onboard/host/aicpu_topology_probe.hsrc/a5/platform/onboard/host/device_runner.cppsrc/a5/platform/onboard/host/device_runner.hsrc/a5/runtime/host_build_graph/host/runtime_maker.cppsrc/a5/runtime/tensormap_and_ringbuffer/host/runtime_maker.cppsrc/common/aicpu_loader/host/load_aicpu_op.htests/ut/cpp/a5/test_aicpu_topology_fallback.cpptools/cann-examples/aicpu-device-query/README.mdtools/cann-examples/aicpu-device-query/host/CMakeLists.txttools/cann-examples/aicpu-device-query/host/query_device_hal.cpp
yanghaoran29
force-pushed
the
add/a5-fg-pg-core-selection
branch
9 times, most recently
from
August 3, 2026 11:35
2b60f55 to
b704cbd
Compare
yanghaoran29
changed the title
yanghaoran29
force-pushed
the
add/a5-fg-pg-core-selection
branch
from
August 3, 2026 13:13
b704cbd to
dcbf182
Compare
Contributor
Author
|
@coderabbitai Addressed the remaining review suggestions:
Validation passed: the 16 A5 topology unit tests, pre-commit, and a separate clang-tidy run. |
|
Your plan includes PR reviews subject to rate limits. Reviews are available now. |
yanghaoran29
force-pushed
the
add/a5-fg-pg-core-selection
branch
3 times, most recently
from
August 4, 2026 02:14
497c7f9 to
c83b46b
Compare
yanghaoran29
changed the title
yanghaoran29
force-pushed
the
add/a5-fg-pg-core-selection
branch
6 times, most recently
from
August 5, 2026 09:27
fbcc1d6 to
49872b1
Compare
Contributor
Author
|
Clarification on the A5 AICPU launch count and the constant 14:
|
Build one launch plan for active roles, full reachable launch coverage, warnings, and FG/PG affinity. Default PG1 to minimum SMT sharing, consolidate topology constants, and document verified live FG topology. Cover the documented Compute-only layouts with deterministic topology unit tests.
yanghaoran29
force-pushed
the
add/a5-fg-pg-core-selection
branch
from
August 5, 2026 10:47
49872b1 to
bfbcc5f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Scope
This change adds topology-aware AICPU core selection for A5.
The runtime combines the authoritative device-side AICPU scheduler pool with Host CPU_TOPO metadata, classifies known FG, PG1, and PG2 topologies, and applies scenario placement for every supported active thread count from 2 through 5. Scheduler SMT availability is recorded as an independent topology property; it does not define a separate FG scenario.
A5 now supports at most five active AICPU threads. The independent launch population remains capped at 14 so the affinity gate can cover the device-visible scheduler pool.
Changes
0x1f8on either Host architecture; Ascend950PR_9579 also constrains Host architecture.aicpu-device-querywith A5 classification and diagnostic JSON output.Verification
pre-commit run --from-ref upstream/main --to-ref HEAD: passed.host_build_graphandtensormap_and_ringbuffer.aicpu-device-queryHost build passed.