[Compile] Fix noop_elimination pass and add tests for noop_elimination #24880
Conversation
Signed-off-by: zjy0516 <[email protected]>
Signed-off-by: zjy0516 <[email protected]>
|
No ciflow labels are configured for this repo. |
ZJY0516
requested review from
ProExpertProg,
youkaichao and
zou3519
as code owners
|
No ciflow labels are configured for this repo. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new test for the NoOpEliminationPass, which is a great addition to improve test coverage. The test correctly verifies the elimination of no-op reshapes and slices. I've identified an opportunity to further improve the test coverage by adding a case for slice_scatter elimination, which is also handled by this pass. My feedback includes a suggestion for a new test function to cover this scenario.
Signed-off-by: zjy0516 <[email protected]>
|
No ciflow labels are configured for this repo. |
|
@ProExpertProg PTAL |
ProExpertProg
left a comment
ProExpertProg
left a comment
There was a problem hiding this comment.
Thanks for writing this test! While you're at it, could you fix the bug with eliminating noop slices?
The issue is that we use the dims_equivalent function for bth reshapes and slices, and while -1 means "infer" for reshapes, it actually means len-1 for slices, so currently we might incorrectly eliminate .slice(x, 0, -1). The easiest way to fix it would be to just directly look at input and output shapes of the meta tensors (originally I avoided that because I didn't want to rely on meta shapes too much but if we rely on one we can also rely on the other).
|
@ProExpertProg I am glad to help. Should I fix it based on this PR or make a new one? Where can I find related issue about this? |
|
@ZJY0516 I don't remember where the issue was and I couldn't find it, sorry! But basically, this slice will get (incorrectly) eliminated: |
Signed-off-by: zjy0516 <[email protected]>
|
No ciflow labels are configured for this repo. |
ZJY0516
changed the title
|
@ProExpertProg I think I have fixed it. PTAL |
Signed-off-by: zjy0516 <[email protected]>
github-actions
bot
added
the
ready
vllm-project#24880) Signed-off-by: zjy0516 <[email protected]>
Signed-off-by: bbartels <[email protected]> [gpt-oss] Add IncompleteDetails to ResponsesRepsonse (vllm-project#24561) Signed-off-by: Andrew Xia <[email protected]> [gpt-oss][1a] create_responses stream outputs BaseModel type, api server is SSE still (vllm-project#24759) Signed-off-by: Andrew Xia <[email protected]> [Performance] Remove redundant clone() calls in cutlass_mla (vllm-project#24891) [Bug] Fix Cutlass Scaled MM Compilation Error (vllm-project#24887) Signed-off-by: yewentao256 <[email protected]> [ci] fix wheel names for arm wheels (vllm-project#24898) Signed-off-by: simon-mo <[email protected]> [Tests] fix initialization of kv hash in tests (vllm-project#24273) Signed-off-by: Mickael Seznec <[email protected]> [Compile] Fix noop_elimination pass and add tests for noop_elimination (vllm-project#24880) Signed-off-by: zjy0516 <[email protected]> Propagate entire tokens to connector for resumed preemptions Signed-off-by: Qier Li <[email protected]> Fix pre-commit Signed-off-by: Qier Li <[email protected]> Rename field and nullify empty lists Signed-off-by: Qier Li <[email protected]> Update vllm/v1/core/sched/scheduler.py Co-authored-by: Nick Hill <[email protected]> Signed-off-by: Qier Li <[email protected]> Add unit test for preemption resumption Signed-off-by: Qier Li <[email protected]>
vllm-project#24880) Signed-off-by: zjy0516 <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
vllm-project#24880) Signed-off-by: zjy0516 <[email protected]>
vllm-project#24880) Signed-off-by: zjy0516 <[email protected]> Signed-off-by: xuebwang-amd <[email protected]>
2 participants
Purpose
Add a separate test for noop_elimination.
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.