Skip to content

Conversation

@zhoux77899
Copy link
Contributor

@zhoux77899 zhoux77899 commented Oct 24, 2025

What this PR does / why we need it?

  • The implemention of aclnnApplyRotaryPosEmbV2 ([main] Optimize rope in Qwen Models #2571) didn't consider UB size
  • Fixes aclnnApplyRotaryPosEmbV2 op tiling error due to required UB size is too large, exceeding the UB size of NPU
  • The benchmark results indicate that the aclnnApplyRotaryPosEmbV2 delivers the best performance, followed by the ATB RopeOperation, while invoking aclnnRotaryPositionEmbedding twice yields the poorest results
  • Therefore, when the usage conditions for the aclnnApplyRotaryPosEmbV2 are not met, we fall back to using the ATB RopeOperation as an alternative

Does this PR introduce any user-facing change?

None

How was this patch tested?

Performance tested on Qwen3-Coder-480B-A35B-Instruct with tp=1, dp=16, ep=16 and tp=2, dp=8, ep=16 on A3
880D1FC1-73F5-439E-D0CD-F3547CBBAC1B

…f required UB size exceeding

Signed-off-by: zhoux77899 <[email protected]>
@github-actions
Copy link

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a bugfix for an operator tiling error on Ascend NPUs by checking for potential Unified Buffer (UB) size overflow and providing a fallback path. The core logic of the fix is sound. My review includes suggestions to improve the maintainability and efficiency of the newly added code, specifically by simplifying a complex calculation and reordering operations to avoid unnecessary work.

Comment on lines +43 to +45
ub_required = (q_n + k_n) * 128 * cast_size * 2 + 128 * dtype_size * 4 + (
q_n + k_n) * 128 * cast_size + (
q_n + k_n) * 128 * cast_size * 2 + cast * 128 * 4 * 2
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The calculation for ub_required is complex and contains repeated terms, making it hard to read and verify. It can be mathematically simplified to a more concise form. This improves readability and maintainability.

The original expression:
(q_n + k_n) * 128 * cast_size * 2 + 128 * dtype_size * 4 + (q_n + k_n) * 128 * cast_size + (q_n + k_n) * 128 * cast_size * 2 + cast * 128 * 4 * 2

can be simplified to:
(q_n + k_n) * 128 * cast_size * 5 + 128 * dtype_size * 4 + cast * 128 * 8

    ub_required = (q_n + k_n) * 128 * cast_size * 5 + 128 * dtype_size * 4 + cast * 128 * 8

@wangxiyuan wangxiyuan added the ready read for review label Oct 24, 2025
… for avoiding repeated executing

Signed-off-by: zhoux77899 <[email protected]>
Signed-off-by: zhoux77899 <[email protected]>
@whx-sjtu whx-sjtu added the ready-for-test start test by label for PR label Oct 25, 2025
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions
Copy link

github-actions bot commented Dec 4, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-conflicts module:ops ready read for review ready-for-test start test by label for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants