[Performance][310p] use recurrent gdn custom op with aclgraph - #9106
Tflowers-0129 wants to merge 9 commits into
Conversation
Summary of ChangesHello, 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 introduces performance optimizations for the 310P platform by integrating a custom recurrent GDN operator with ACLGraph. The changes involve updating core operator signatures to use optional tensors, replacing fallback PyTorch implementations with specialized NPU kernels for decode and spec-decode operations, and cleaning up test code to align with these API updates. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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 the 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 counterproductive. 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. Footnotes
|
|
👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:
If CI fails, you can run linting and testing checks locally according Contributing and Testing. |
There was a problem hiding this comment.
Code Review
This pull request introduces a custom recurrent Gated Delta Net (GDN) operator for Ascend 310P to optimize performance and support graph capture, replacing the previous PyTorch-based implementation. It also updates the npu_causal_conv1d_310 operator to accept Tensors instead of array references for metadata, facilitating aclgraph compatibility. Feedback from the reviewer indicates that the PR title and summary do not adhere to the repository's style guide. Furthermore, a high-severity issue was identified regarding a missing in-place marker for the conv_states tensor in the operator registration, which could lead to incorrect behavior during graph capture.
| " Tensor weight, " | ||
| " Tensor? bias, " | ||
| " Tensor conv_states, " | ||
| " int[] query_start_loc, " | ||
| " int[] cache_indices, " | ||
| " int[] initial_state_mode, " | ||
| " int[] num_accepted_tokens, " | ||
| " Tensor? query_start_loc, " | ||
| " Tensor? cache_indices, " | ||
| " Tensor? initial_state_mode, " | ||
| " Tensor? num_accepted_tokens, " | ||
| " int activation_mode, " | ||
| " int pad_slot_id, " | ||
| " int run_mode) -> (Tensor output)"); |
There was a problem hiding this comment.
Suggested PR Title:
[310p][Ops][Feature] use recurrent gdn custom op with aclgraphSuggested PR Summary:
### What this PR does / why we need it?
This PR introduces a custom recurrent GDN operator for Ascend 310P to improve performance and support `aclgraph` capture. It replaces the existing PyTorch-based recurrent rule with a fused kernel (`npu_recurrent_gated_delta_rule_310`) and updates the `causal_conv1d_310` operator to use tensors for metadata instead of array refs, facilitating graph capture.
### Does this PR introduce _any_ user-facing change?
No. This is an internal performance optimization for Ascend 310P.
### How was this patch tested?
Tested with existing E2E nightly tests for `causal_conv1d_310` and GDN attention.The current PR title and summary do not follow the required format specified in the repository style guide. I have provided a suggested title and summary above.
Additionally, the schema for npu_causal_conv1d_310 is missing the in-place marker for the conv_states tensor. Since this operator updates the convolution states in-place (as verified by the tests), the schema should use Tensor! conv_states to ensure correct behavior with the PyTorch dispatcher and graph capture mechanisms.
" Tensor weight, "
" Tensor? bias, "
" Tensor! conv_states, "
" Tensor? query_start_loc, "
" Tensor? cache_indices, "
" Tensor? initial_state_mode, "
" Tensor? num_accepted_tokens, "
" int activation_mode, "
" int pad_slot_id, "
" int run_mode) -> (Tensor output)");References
- The PR title and summary must follow the specific format: [Branch][Module][Action] Title, and include sections for 'What this PR does', 'User-facing change', and 'How was this patch tested'. (link)
315e9a5 to
59f4378
Compare
…r ops Signed-off-by: Tflowers-0129 <2906339855@qq.com>
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
fc55af2 to
1f8438e
Compare
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
Signed-off-by: Tflowers-0129 <2906339855@qq.com>
|
我调试不出来了,causal-conv1d一直出现bad-alloc,先关闭吧,重头开始弄 |
What this PR does / why we need it?
[310P] Fix linear attention graph capture and dtype issues
Does this PR introduce any user-facing change?
NA
How was this patch tested?
some UT and local test