[feat]: add kunpeng kml support #1606
Conversation
Summary of ChangesHello @Alisehen, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a bug related to the Kunpeng KML integration, primarily focusing on ensuring the correct build environment setup and refining the MOE (Mixture of Experts) kernel implementation. The changes streamline the KML-specific build process, refactor MOE kernel class names for clarity, and introduce a fix for the scaling logic within the MOE kernel, enhancing its stability and correctness. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 support for Kunpeng Math Library (KML) and fixes related bugs. The changes include updating the CMake build system to locate KML compilers and libraries, and refactoring MoE kernel APIs for better consistency. My review focuses on improving the portability and maintainability of the build system. I've identified hardcoded paths in CMakeLists.txt that should be replaced with variables to allow for different installation locations. I've also noted an opportunity to reduce code duplication in kernel.hpp by refactoring common logic between GemmKernelInt8 and GemmKernelInt4.
| set(CMAKE_C_COMPILER "/opt/HPCKit/25.1.0/compiler/gcc/bin/gcc" CACHE FILEPATH "C compiler" FORCE) | ||
| set(CMAKE_CXX_COMPILER "/opt/HPCKit/25.1.0/compiler/gcc/bin/g++" CACHE FILEPATH "C++ compiler" FORCE) |
There was a problem hiding this comment.
Hardcoding absolute paths for compilers makes the build configuration fragile and not portable. This will cause build failures on any machine with a different setup. It's better to define a variable for the root path of the toolkit and use it here.
You should define HPCKIT_ROOT before this block, for example, near the other options:
set(HPCKIT_ROOT "/opt/HPCKit/25.1.0" CACHE PATH "Root directory of the Kunpeng HPC Kit")Then you can use it in this block and for the include/link directories later.
set(CMAKE_C_COMPILER "${HPCKIT_ROOT}/compiler/gcc/bin/gcc" CACHE FILEPATH "C compiler" FORCE)
set(CMAKE_CXX_COMPILER "${HPCKIT_ROOT}/compiler/gcc/bin/g++" CACHE FILEPATH "C++ compiler" FORCE)
| include_directories("/opt/HPCKit/25.1.0/kml/gcc/include") | ||
| link_directories(/opt/HPCKit/25.1.0/kml/gcc/lib) |
There was a problem hiding this comment.
These paths are also hardcoded, which harms portability. Please use the HPCKIT_ROOT variable suggested for the compiler paths. This makes it much easier to adapt the build to different environments.
Also, note that link_directories() is a legacy command. It's better to use target_link_libraries() with the full path to libraries for specific targets to avoid polluting the link path for all targets.
include_directories("${HPCKIT_ROOT}/kml/gcc/include")
link_directories(${HPCKIT_ROOT}/kml/gcc/lib)
| static void apply_scale(int m, int n, float *c, BufferA *ba, BufferB *bb, BufferC *bc, int ith, int nth, int block, int jth = -1) { | ||
| // printf("use split apply scale\n"); | ||
| auto [n_start, n_end] = split_range_n_block(n, ith, nth, block); | ||
| int m_start = 0, m_end = m; | ||
| if (jth != -1) { | ||
| auto tmp = split_range_m(m, jth); | ||
| m_start = tmp.first; | ||
| m_end = tmp.second; | ||
| } | ||
| // TODO: 后续用 SVE 来加速 | ||
| for (int m_begin = 0; m_begin < m; m_begin += M_STEP) { | ||
| for (int i = 0; i < M_STEP && m_begin + i < m; i++) { | ||
| for (int m_begin = m_start; m_begin < m_end; m_begin += M_STEP) { | ||
| for (int i = 0; i < M_STEP && m_begin + i < m_end; i++) { | ||
| float *scale_a = ba->get_scale(m, m_begin + i); |
There was a problem hiding this comment.
This change is also present in GemmKernelInt8 (lines 692-705). The structs GemmKernelInt8 and GemmKernelInt4 share a lot of duplicated code. While this PR makes them consistent, it also propagates the duplication. To improve maintainability, consider refactoring the common code into a base class or a template. This would reduce redundancy and make future changes easier.
1 participant
No description provided.