Skip to content

fix(ascend): correct rotary embedding handling - #952

Open
baominghelly wants to merge 5 commits into
feat/ascend-llama-opsfrom
fix/ascend-llama-kernels
Open

fix(ascend): correct rotary embedding handling#952
baominghelly wants to merge 5 commits into
feat/ascend-llama-opsfrom
fix/ascend-llama-kernels

Conversation

@baominghelly

@baominghelly baominghelly commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Correct the Ascend RoPE implementation by using separate aclnnRotaryPositionEmbedding calls for query and optional key tensors.
  • Extend the Ascend RoPE test coverage for the supported forward full-dimension NeoX configuration.

Motivation

The previous Ascend RoPE implementation used an ACLNN layout and semantic combination that produced incorrect query and key values. It also required a key tensor even though the operator interface permits query-only execution.

Type of Change

  • feat - new feature / new operator / new platform
  • fix - bug fix
  • perf - performance improvement (no behavioral change)
  • refactor - code restructuring without behavior change
  • test - adding or fixing tests only
  • docs - documentation only
  • build / ci - build system or CI configuration
  • chore - tooling, formatting, or other non-code changes
  • Breaking change (requires a ! in the Conventional Commits prefix or a BREAKING CHANGE: footer)

Platforms Affected

  • CPU (WITH_CPU)
  • NVIDIA (WITH_NVIDIA)
  • Iluvatar (WITH_ILUVATAR)
  • MetaX (WITH_METAX)
  • Cambricon (WITH_CAMBRICON)
  • Moore (WITH_MOORE)
  • Ascend (WITH_ASCEND)
  • PyTorch C++ bindings (WITH_TORCH)
  • Build system / CMake / CI
  • Python bindings / user-facing API

Smoke Test Result

$ cmake -S /workspace/InfiniOps \
    -B /workspace/InfiniOps/build-rope-pr952 \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_INSTALL_PREFIX=/workspace/InfiniOps/test-rope-pr952/infini \
    -DGENERATE_PYTHON_BINDINGS=ON \
    -DWITH_ASCEND=ON \
    -DBUILD_ASCEND_CUSTOM=ON \
    -DSOC_VERSION=Ascend910_9382 \
    -DINFINI_RT_ROOT=/workspace/InfiniRT-6b256e0-prefix \
    -DINFINI_OPS_OPS=rotary_embedding
$ cmake --build /workspace/InfiniOps/build-rope-pr952 -j8
[ 55%] Built target infiniops
[100%] Built target ops

$ ASCEND_LAUNCH_BLOCKING=1 python -S -m pytest \
    tests/test_rotary_embedding.py --devices ascend -q -rs --tb=short -x
6 passed, 18 skipped, 14 warnings in 1.28s

Test Results on Supported Platforms

Platform Affected Build / Smoke Result Full Result / Notes
NVIDIA No N/A - not affected N/A - not affected
Iluvatar No N/A - not affected N/A - not affected
MetaX No N/A - not affected N/A - not affected
Cambricon No N/A - not affected N/A - not affected
Moore No N/A - not affected N/A - not affected
Ascend Yes build passed RoPE targeted suite passed: 6 passed, 18 skipped; full suite not rerun
Full `pytest` output
.sss.sss.sss.sss.sss.sss

SKIPPED [18] tests/test_rotary_embedding.py:45:
Ascend supports forward full-dimension NeoX RoPE

6 passed, 18 skipped, 14 warnings in 1.28s

Benchmark / Performance Impact

N/A. This is a correctness fix; performance was not benchmarked.

Notes for Reviewers

  • Ascend currently supports forward full-dimension NeoX RoPE; unsupported variants remain explicit test skips.
  • Query and optional key tensors are processed independently through ACLNN, then copied back to preserve the operator's in-place behavior.
  • Validation used InfiniRT 6b256e0ab0853d9e2a7ef059bac22c847bd02251 and SOC_VERSION=Ascend910_9382.

wooway777 and others added 5 commits August 25, 2026 10:26
@baominghelly
baominghelly force-pushed the fix/ascend-llama-kernels branch from 8adfcd1 to 223f9d4 Compare August 25, 2026 06:26
@baominghelly baominghelly changed the title fix(ascend): correct Llama operator edge cases fix(ascend): correct rotary embedding handling Aug 25, 2026
@baominghelly
baominghelly requested a review from wooway777 August 25, 2026 07:53
@wooway777
wooway777 force-pushed the feat/ascend-llama-ops branch 2 times, most recently from 53d4408 to 1f3d182 Compare August 25, 2026 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants