Adaptation of the Deepep A5 normal and low-latency operators, support PD disaggregated deployment.#474
Conversation
- Add A5-adapted normal communication operator implementation - Add cam_moe_dispatch_normal_a5.h and cam_moe_combine_normal_a5.h - Support high-throughput communication for large EP scenario - Optimize dispatch and combine operator performance - Add mc2_hcom_topo_info.h for topology information - Implement moe_distribute_dispatch_v2_a5.h and moe_distribute_combine_v2_a5.h - Add notify_dispatch_a5.h for notification mechanism
- Modify communication domain initialization using HcclCommInitRootInfo - Support mixed deployment of Prefill and Decode nodes - Add dispatch_ffn_combine operator optimization - Update buffer.py to support PD separation scenario - Improve documentation for PD disaggregated configuration
- Add A2 chip low-latency operator support - Add soc_version_compat.h for CANN 8.5 compatibility - Support low-latency communication for Decode stage - Optimize layered and non-layered operator implementation - Update LOW_LATENCY_API documentation
- Add automatic CANN version detection - Create compatibility layer for CANN 8.5 API changes - Update build scripts and CI configurations - Fix platform compatibility issues - Add CANN_8.5_ADAPTATION.md and PLATFORM_COMPAT_FIX.md documentation - Add diagnose_cann.sh script for environment diagnosis - Update other modules and tests
![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 adapts the codebase for CANN 8.5 and introduces support for the Ascend 950 (A5) chip, featuring new kernel implementations for MoE operations and a modernized CMake build system. Key updates include A5-specific tiling logic, updated HCCL communication structures, and environment diagnostic scripts. Feedback identifies critical issues, including flawed HCCL initialization for multi-rank setups, hardcoded chip versions in the build script, and incomplete platform detection for A5. Furthermore, a security vulnerability was found in a script handling GitHub tokens, and redundant ccache configuration exists in the build files.
| HcclRootInfo rootInfo; | ||
| HcclGetRootInfo(&rootInfo); | ||
| ACL_CHECK(aclrtGetDevice(&device_id)); | ||
|
|
||
| // ep domain | ||
| HCCL_CHECK(HcclCommInitClusterInfo(ranktable_file, device_id, &ep_comm)); | ||
| // 初始化通信域 | ||
| HcclCommInitRootInfo(num_ranks, &rootInfo, device_id, &ep_comm); |
There was a problem hiding this comment.
The new HCCL initialization logic is incorrect for multi-rank environments. HcclGetRootInfo generates a unique ID locally. If every rank calls this independently, they will each create their own communication domain instead of joining a shared one. Typically, rank 0 should call HcclGetRootInfo and broadcast the rootInfo to all other ranks via an out-of-band mechanism (like MPI or Gloo) before all ranks call HcclCommInitRootInfo with the same info.
| inline ge::graphStatus GetEpWinSize(const gert::TilingContext *context, const char *nodeName, | ||
| uint64_t &hcclBufferSizeEp, uint64_t &maxWindowSizeEp, uint32_t attrGroupEpIndex) | ||
| { | ||
| auto attrs = context->GetAttrs(); | ||
| if (mc2tiling::GetSocVersion(context) == "Ascend910_95") { | ||
| // A5 暂不支持 Hccl CommGetBufSizeCfg 接口，此处暂作规避 | ||
| hcclBufferSizeEp = Mc2TilingUtils::GetMaxWindowSize(); | ||
| // A5 上前 1MB 作为状态区，剩余空间用作数据区 | ||
| maxWindowSizeEp = hcclBufferSizeEp - MTE_STATE_ZONE_SIZE; | ||
| } else { | ||
| // auto groupEpHccl = attrs->GetAttrPointer<char>(static_cast<int>(attrGroupEpIndex)); | ||
| // OP_TILING_CHECK(GetCclBufferSize(groupEpHccl, &hcclBufferSizeEp, nodeName) != ge::GRAPH_SUCCESS, | ||
| // OP_LOGE(nodeName, "Get Ep HcclBufferSizeEP failed, HcclBufferSizeEP is %lu", maxWindowSizeEp), | ||
| // return ge::GRAPH_FAILED); | ||
| // maxWindowSizeEp = hcclBufferSizeEp; | ||
| } | ||
| return ge::GRAPH_SUCCESS; | ||
| } |
There was a problem hiding this comment.
The GetEpWinSize function has its logic for non-A5 chips commented out. This causes hcclBufferSizeEp and maxWindowSizeEp to remain uninitialized for A2/A3 chips, leading to undefined behavior in tiling. Furthermore, the MTE_STATE_ZONE_SIZE (1MB) used here for A5 is inconsistent with A5_MTE_STATE_WIN_SIZE (4MB) defined in moe_distribute_base.h.
| -DASCEND_HOME_PATH=$ASCEND_HOME_PATH \ | ||
| -DASCEND_INCLUDE_DIR=$ASCEND_INCLUDE_DIR \ | ||
| -DSOC_VERSION=$SOC_VERSION \ | ||
| -DSOC_VERSION="Ascend910_9382" \ |
There was a problem hiding this comment.
The SOC_VERSION is hardcoded to Ascend910_9382, which breaks the build for other supported chips (e.g., Ascend910B, Ascend950). It should use the $SOC_VERSION variable as it did previously.
| -DSOC_VERSION="Ascend910_9382" \ | |
| -DSOC_VERSION=$SOC_VERSION \ |
| switch (socVersionEnum) { | ||
| case platform_ascendc::SocVersion::ASCEND910B: | ||
| return "Ascend910B"; | ||
| case platform_ascendc::SocVersion::ASCEND910_9382: | ||
| return "Ascend910_9382"; | ||
| default: | ||
| return "Unknown"; | ||
| } |
There was a problem hiding this comment.
The GetSocVersion function does not handle the Ascend 950 (A5) chip in the platform_ascendc API path. This will cause it to return Unknown, breaking tiling logic that depends on version detection (e.g., in cam_moe_combine_normal_tiling.cc). Additionally, ensure the returned string matches the expected value in tiling files (e.g., Ascend950).
switch (socVersionEnum) {
case platform_ascendc::SocVersion::ASCEND910B:
return "Ascend910B";
case platform_ascendc::SocVersion::ASCEND910_9382:
return "Ascend910_9382";
case platform_ascendc::SocVersion::ASCEND950:
return "Ascend950";
default:
return "Unknown";
}| inline std::string GetSocVersionCompat() | ||
| { | ||
| #if defined(USE_CANN83_PATH) || defined(USE_CANN82_PATH) | ||
| // For CANN 8.2/8.3, use PlatFormInfos (需要从context获取，这里作为fallback) | ||
| return "Unknown"; | ||
| #else | ||
| // For CANN 8.5+, use platform_ascendc API | ||
| auto ascendcPlatform = platform_ascendc::PlatformAscendCManager::GetInstance(); | ||
| platform_ascendc::SocVersion socVersionEnum = ascendcPlatform->GetSocVersion(); | ||
|
|
||
| switch (socVersionEnum) { | ||
| case platform_ascendc::SocVersion::ASCEND910B: | ||
| return "Ascend910B"; | ||
| case platform_ascendc::SocVersion::ASCEND910_9382: | ||
| return "Ascend910_9382"; | ||
| default: | ||
| return "Unknown"; | ||
| } | ||
| #endif | ||
| } |
There was a problem hiding this comment.
The GetSocVersionCompat function returns Unknown for CANN 8.2/8.3 paths. This will break any tiling logic that relies on this function to identify the chip version. It should attempt to retrieve the version from the context or a global state if possible, or at least handle the A5 enum in the 8.5+ path.
| #!/bin/bash | ||
| # Push Instructions for feature/add-three-functions branch | ||
| # Execute in environment with direct GitHub access | ||
|
|
||
| echo "=== Branch Information ===" | ||
| echo "Branch: feature/add-three-functions" | ||
| echo "Commits: 4 (English messages)" | ||
| echo | ||
|
|
||
| git log --oneline --decorate | head -5 | ||
| echo | ||
|
|
||
| echo "=== Please enter GitHub Token (with repo permission): ===" | ||
| read -s TOKEN | ||
|
|
||
| git remote set-url origin https://${TOKEN}@github.com/1329009851/sgl-kernel-npu.git | ||
|
|
||
| echo "=== Pushing to GitHub ===" | ||
| git push -u origin feature/add-three-functions | ||
|
|
||
| if [ $? -eq 0 ]; then | ||
| echo | ||
| echo "✅ Success!" | ||
| echo "Branch: https://github.com/1329009851/sgl-kernel-npu/tree/feature/add-three-functions" | ||
| echo "Create PR: https://github.com/1329009851/sgl-kernel-npu/pull/new/feature/add-three-functions" | ||
| fi | ||
|
|
||
| # Clean token | ||
| git remote set-url origin https://github.com/1329009851/sgl-kernel-npu.git |
There was a problem hiding this comment.
This script encourages users to input a GitHub Token and then embeds it into the remote URL. This is a security risk as the token might be leaked via shell history or process listings. Such scripts should not be committed to the repository.
| find_program(CCACHE_PROGRAM ccache) | ||
| if(CCACHE_PROGRAM) | ||
| message(STATUS "Found ccache: ${CCACHE_PROGRAM}") | ||
| set(CMAKE_CXX_COMPILER_LAUNCHER "${CCACHE_PROGRAM}") | ||
| set(CMAKE_C_COMPILER_LAUNCHER "${CCACHE_PROGRAM}") | ||
| else() | ||
| message(STATUS "ccache not found, proceeding with normal compilation") | ||
| endif() | ||
|
|
||
| find_program(CCACHE_PROGRAM ccache) | ||
| if(CCACHE_PROGRAM) |
There was a problem hiding this comment.
The detection and configuration of ccache is redundant as it is repeated twice in the same block. The second find_program and its associated if block (lines 60-61) should be removed.
find_program(CCACHE_PROGRAM ccache)
if(CCACHE_PROGRAM)
message(STATUS "Found ccache: ${CCACHE_PROGRAM}")
set(CMAKE_CXX_COMPILER_LAUNCHER "${CCACHE_PROGRAM}")
set(CMAKE_C_COMPILER_LAUNCHER "${CCACHE_PROGRAM}")
else()
message(STATUS "ccache not found, proceeding with normal compilation")
endif()
1 participant
Adaptation of the Deepep A5 normal and low-latency operators, support PD disaggregated deployment.