feat(ascend): implement element-wise ops #608
Conversation
📝 WalkthroughWalkthroughThe changes add support for two new elementwise operations (subtraction and multiplication) to the Ascend backend, including operation implementations, factory registrations, CMake build configuration updates, and corresponding unit tests with FP16 validation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 1
🤖 Fix all issues with AI agents
In `@mllm/backends/ascend/CMakeLists.txt`:
- Around line 37-40: The fallback block that configures ATB when EXISTS
"${PROJECT_SOURCE_DIR}/../libs/atb" must guard against partial installs: before
calling target_include_directories and target_link_directories for target
MllmAscendBackend, check that both "${PROJECT_SOURCE_DIR}/../libs/atb/include"
and "${PROJECT_SOURCE_DIR}/../libs/atb/lib" exist (use if(EXISTS ...) or
if(IS_DIRECTORY ...)); only add the include and link directories when both are
present, otherwise emit a message(WARNING ...) and skip wiring the paths so
CMake doesn't get invalid search paths.
🧹 Nitpick comments (3)
mllm/backends/ascend/ops/AscendElewiseOps.hpp (1)
27-55: Add brief API comments for new public ops/factories.These are public-facing classes in a header; consider short comments mirroring the existing Add op style so their purpose/usage is explicit. As per coding guidelines, public APIs should be documented.
As per coding guidelines, add brief API docs for these new public classes/factories.mllm/backends/ascend/ops/AscendElewiseOps.cpp (1)
109-195: Consider extracting shared elementwise forward logic.Sub and Mul closely mirror Add’s forward path; a small helper (e.g., taking the elewise type + trace name) would reduce duplication and future drift.
tests/ascend/AscendKernelTest.hpp (1)
52-120: Replace repeated FP16 tolerance literals with a named constant.This keeps the new tests consistent and avoids spreading magic numbers. As per coding guidelines, use named constants instead of literals.
As per coding guidelines, use a named constant for the tolerance.♻️ Suggested tweak
bool SubFloat16Test(const std::vector<mllm::Tensor::shape_t>& shapes) { using namespace mllm; // NOLINT + constexpr float kTol = 1e-2f; for (auto& shape : shapes) { @@ - auto result = mllm::test::allClose(z_cpu, ref_cpu, 1e-2f, 1e-2f); + auto result = mllm::test::allClose(z_cpu, ref_cpu, kTol, kTol); @@ bool MulFloat16Test(const std::vector<mllm::Tensor::shape_t>& shapes) { using namespace mllm; // NOLINT + constexpr float kTol = 1e-2f; for (auto& shape : shapes) { @@ - auto result = mllm::test::allClose(z_cpu, ref_cpu, 1e-2f, 1e-2f); + auto result = mllm::test::allClose(z_cpu, ref_cpu, kTol, kTol);
| elseif(EXISTS "${PROJECT_SOURCE_DIR}/../libs/atb") | ||
| message(STATUS "Found ATB in ${PROJECT_SOURCE_DIR}/../libs/atb") | ||
| target_include_directories(MllmAscendBackend PUBLIC "${PROJECT_SOURCE_DIR}/../libs/atb/include") | ||
| target_link_directories(MllmAscendBackend PUBLIC "${PROJECT_SOURCE_DIR}/../libs/atb/lib") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine its structure
find . -name "CMakeLists.txt" -path "*/backends/ascend/*" | head -20Repository: UbiquitousLearning/mllm
Length of output: 104
🏁 Script executed:
# Get the full CMakeLists.txt file to verify lines 37-40 and understand context
if [ -f "mllm/backends/ascend/CMakeLists.txt" ]; then
echo "=== File exists, showing content with line numbers ==="
cat -n "mllm/backends/ascend/CMakeLists.txt" | head -50
else
echo "File not found at mllm/backends/ascend/CMakeLists.txt"
fiRepository: UbiquitousLearning/mllm
Length of output: 2355
🏁 Script executed:
# Check for similar ATB-related paths in the codebase
rg -n "libs/atb" --type cmakeRepository: UbiquitousLearning/mllm
Length of output: 531
Guard the ATB fallback path against partial installs.
If ../libs/atb exists but the include/ or lib/ subdirectories are missing, CMake will add invalid search paths that fail later during compilation. Check for required subdirectories before wiring the paths.
🔧 Suggested guard
-elseif(EXISTS "${PROJECT_SOURCE_DIR}/../libs/atb")
- message(STATUS "Found ATB in ${PROJECT_SOURCE_DIR}/../libs/atb")
- target_include_directories(MllmAscendBackend PUBLIC "${PROJECT_SOURCE_DIR}/../libs/atb/include")
- target_link_directories(MllmAscendBackend PUBLIC "${PROJECT_SOURCE_DIR}/../libs/atb/lib")
+elseif(EXISTS "${PROJECT_SOURCE_DIR}/../libs/atb/include" AND EXISTS "${PROJECT_SOURCE_DIR}/../libs/atb/lib")
+ message(STATUS "Found ATB in ${PROJECT_SOURCE_DIR}/../libs/atb")
+ target_include_directories(MllmAscendBackend PUBLIC "${PROJECT_SOURCE_DIR}/../libs/atb/include")
+ target_link_directories(MllmAscendBackend PUBLIC "${PROJECT_SOURCE_DIR}/../libs/atb/lib")
+elseif(EXISTS "${PROJECT_SOURCE_DIR}/../libs/atb")
+ message(WARNING "ATB fallback path exists but include/lib are missing; skipping")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| elseif(EXISTS "${PROJECT_SOURCE_DIR}/../libs/atb") | |
| message(STATUS "Found ATB in ${PROJECT_SOURCE_DIR}/../libs/atb") | |
| target_include_directories(MllmAscendBackend PUBLIC "${PROJECT_SOURCE_DIR}/../libs/atb/include") | |
| target_link_directories(MllmAscendBackend PUBLIC "${PROJECT_SOURCE_DIR}/../libs/atb/lib") | |
| elseif(EXISTS "${PROJECT_SOURCE_DIR}/../libs/atb/include" AND EXISTS "${PROJECT_SOURCE_DIR}/../libs/atb/lib") | |
| message(STATUS "Found ATB in ${PROJECT_SOURCE_DIR}/../libs/atb") | |
| target_include_directories(MllmAscendBackend PUBLIC "${PROJECT_SOURCE_DIR}/../libs/atb/include") | |
| target_link_directories(MllmAscendBackend PUBLIC "${PROJECT_SOURCE_DIR}/../libs/atb/lib") | |
| elseif(EXISTS "${PROJECT_SOURCE_DIR}/../libs/atb") | |
| message(WARNING "ATB fallback path exists but include/lib are missing; skipping") |
🤖 Prompt for AI Agents
In `@mllm/backends/ascend/CMakeLists.txt` around lines 37 - 40, The fallback block
that configures ATB when EXISTS "${PROJECT_SOURCE_DIR}/../libs/atb" must guard
against partial installs: before calling target_include_directories and
target_link_directories for target MllmAscendBackend, check that both
"${PROJECT_SOURCE_DIR}/../libs/atb/include" and
"${PROJECT_SOURCE_DIR}/../libs/atb/lib" exist (use if(EXISTS ...) or
if(IS_DIRECTORY ...)); only add the include and link directories when both are
present, otherwise emit a message(WARNING ...) and skip wiring the paths so
CMake doesn't get invalid search paths.
1 participant
Description
This PR adds support for Subtraction and Multiplication element-wise operations on the Ascend backend.
Changes
AscendSubOpandAscendMulOp.SubFloat16andMulFloat16) inAscendKernelTest.Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.