Add: select A5 AICPU cores on verified 9599 and 9579 devices - #1643
Add: select A5 AICPU cores on verified 9599 and 9579 devices#1643yanghaoran29 wants to merge 1 commit into
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
2b60f55 to
b704cbd
Compare
yanghaoran29
changed the title
Select the default 1O+4S affinity from device-side occupancy and host CPU_TOPO for A5 layouts covered by the probe. - Apply FG, FG+SMT, PG1, and PG2 placement when complete topology metadata is available; keep unknown layouts on a CPU-ID-ordered OCCUPY-only fallback. - Use the embedded Ascend950PR_9599 topology when live CPU_TOPO is unavailable, without a behavior-changing environment override. - Recover the device after topology-query launch or synchronization failures so the persistent stream cannot remain poisoned. - Emit robust diagnostic JSON and allow the driver library directory to be overridden when building aicpu-device-query.
b704cbd to
dcbf182
Compare
|
@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. |
1 participant
Scope
This change is scoped to the A5 devices whose topology inputs were verified:
Ascend950PR_9599: use device-side occupancy plus the documented9-logical-CPU topology to select the FG 1O+4S affinity.
Ascend950PR_9579withOCCUPY=0x3e: when CPU_TOPO is unavailable,treat the five occupied bits as the complete schedulable pool and select
all five threads without assuming SMT relationships.
Other complete A5 topology shapes use the structural FG, FG+SMT, PG1, and
PG2 policies covered by C++ unit tests. Unknown shapes retain the
topology-ordered fallback; this PR does not claim hardware validation for
unlisted device shapes.
Changes
Ascend950PR_9599table onlywhen the driver topology query is unavailable.
changes runtime topology behavior.
aicpu-device-querychoose the a2a3 or A5 dispatcher with--platform, defaulting to A5. JSON topology classification remains A5-only.five-thread OCCUPY fallback in C++ unit tests.
Verification
ctest --test-dir tests/ut/cpp/build-pr1643 -LE requires_hardware --output-on-failure(75/75 passed)pre-commit run --files <changed files>