-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
Make sure that vectorize_with_alignment produced vectorized global loads #23182
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make sure that vectorize_with_alignment produced vectorized global loads #23182
Conversation
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
There was a problem hiding this 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 effectively addresses a performance issue where vectorize_with_alignment
failed to produce vectorized global loads. By explicitly creating a local copy of the data, you've successfully guided the compiler to generate the desired vectorized instructions. The provided PTX output and ncu metrics clearly demonstrate the improvement. My review includes one suggestion to apply this same optimization to the vectorize_read_with_alignment
function to ensure consistent performance improvements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an excellent fix that correctly encourages the compiler to generate vectorized load instructions. A similar performance issue exists in the vectorize_read_with_alignment
function within this file (on lines 132 and 157), where data is also passed directly from a global pointer. To ensure consistent vectorization, you should apply the same pattern there.
For example:
// In vectorize_read_with_alignment's fast path (line 132)
for (int i = tid; i < num_vec; i += stride) {
vin_t src = v_in[i];
vec_op(src);
}
This change should be applied to the vectorized loop in the slow path as well (line 157).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the optimization!
Could you also test on E2E model performance & accuracy?
Commands could be seen here as an example #21961
Signed-off-by: elvircrn <[email protected]>
868649b
to
1dc7136
Compare
Signed-off-by: elvircrn <[email protected]>
Converted to draft until I test out lm_eval from #21961. |
Nice find, looking forward for e2e results to validate |
Related Documentation No published documentation to review for changes on this repository. |
@mgoin @yewentao256 @ProExpertProg I removed the draft tag. |
I also ran Proposed changes:
Base branch:
|
Re-ran accuracy tests via This branch:
Base branch:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks for the work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great find! Please put the perf and eval in the PR description
…ads (vllm-project#23182) Signed-off-by: root <[email protected]>
…ads (vllm-project#23182) Signed-off-by: Xiao Yu <[email protected]>
Purpose
Vectorize with alignment generated scalar instructions where vectorization was possible.
This PR fixes this, nudging the compiler to actually produce a vectorized load.
I ran
vllm bench throughput --model nm-testing/DeepSeek-Coder-V2-Lite-Instruct-FP8 --input-len 1000 --output-len 100 --trust_remote_code --enforce_eager
Proposed changes:
Base branch:
Accuracy tests via
lm_eval --model vllm --model_args "pretrained=nm-testing/DeepSeek-Coder-V2-Lite-Instruct-FP8,max_model_len=32768,enable_expert_parallel=True,enforce_eager=True" --trust_remote_code --tasks gsm8k --num_fewshot 5 --batch_size auto
This branch:
Base branch:
====
Given the following source:
compiled with
-lineinfo -g -O3 -arch=sm_90 -ptx
we see the generated code for the old function:
vs the newly-updated vectorize_with_alignment
does not emit vectorized load instructions (see
ld.v4.f32
for this).Furthermore, tests were done in a broader context and ncu now reports that the updated code has significantly less uncoalesced global loads:
Thank you @LucasWilkinson for the find.