-
Notifications
You must be signed in to change notification settings - Fork 11
fix: correct Vulkan Linear output for batches smaller than 4 (garbage decode) #17
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
Open
maci0
wants to merge
2
commits into
ericcurtin:main
Choose a base branch
from
maci0:fix-gemm-small-m-decode
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| """Vulkan Linear dispatch correctness for small batches. | ||
|
|
||
| The Vulkan matmul kernel tiles 4 rows and returns incorrect results for M < 4 | ||
| (the M=1 decode step), which produced garbage tokens during generation. | ||
| ``_wrap_linear`` pads small batches up to 4 rows and slices back; these tests | ||
| verify the wrapped Linear matches a plain ``nn.Linear`` across M=1..8. | ||
| """ | ||
|
|
||
| import pytest | ||
| import torch | ||
| from torch import nn | ||
|
|
||
| _rs = pytest.importorskip("vllm_vulkan._rs", exc_type=ImportError) | ||
|
|
||
|
|
||
| def _require_device(): | ||
| if not _rs.is_available(): | ||
| pytest.skip("no Vulkan device available") | ||
|
|
||
|
|
||
| def _ready_ops(): | ||
| from vllm_vulkan import vulkan_ops | ||
| from vllm_vulkan._rs import VulkanContext | ||
|
|
||
| if not vulkan_ops.is_ready(): | ||
| vulkan_ops.set_context(VulkanContext(0)) | ||
| return vulkan_ops | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("m", [1, 2, 3, 4, 8, 17]) | ||
| @pytest.mark.parametrize("bias", [False, True]) | ||
| def test_wrapped_linear_matches_torch_for_small_batch(m, bias): | ||
| _require_device() | ||
| _ready_ops() | ||
| from vllm_vulkan.model_runner import _wrap_linear | ||
|
|
||
| torch.manual_seed(0) | ||
| k, n = 896, 896 | ||
| lin = nn.Linear(k, n, bias=bias) | ||
| ref = nn.Linear(k, n, bias=bias) | ||
| ref.load_state_dict(lin.state_dict()) | ||
|
|
||
| _wrap_linear(lin) # dispatch lin.forward to Vulkan | ||
|
|
||
| x = torch.randn(m, k) | ||
| got = lin(x) | ||
| exp = ref(x) | ||
| assert torch.allclose(got, exp, atol=1e-2, rtol=1e-2), f"M={m} bias={bias} mismatch" | ||
|
|
||
|
|
||
| def test_raw_kernel_exact_for_m_at_least_4(): | ||
| """Guard the working path: the matmul kernel is exact for M>=4. (The M<4 | ||
| path is wrong, which is what _wrap_linear pads around; a future shader fix | ||
| should make this hold for all M and let the workaround be removed.) | ||
| """ | ||
| _require_device() | ||
| ops = _ready_ops() | ||
| torch.manual_seed(0) | ||
| k, n = 896, 896 | ||
| w = torch.randn(n, k) | ||
| for m in (4, 8, 32): | ||
| x = torch.randn(m, k) | ||
| assert torch.allclose( | ||
| ops.linear(x, w, None), torch.nn.functional.linear(x, w), atol=1e-2, rtol=1e-2 | ||
| ), f"M={m}" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Critical Caching and Empty Batch Issues
Weight Caching Bypass (Performance Disaster): Passing
weight.float()tovulkan_ops.linearcreates a temporary float32 tensor with a new untyped storage on every single forward pass. Because_get_or_upload_weightusesid(weight.untyped_storage())as the cache key, this temporary tensor results in a cache miss every single step. Consequently, the entire weight matrix is re-uploaded to the GPU on every single token generation step, completely defeating the weight caching mechanism and causing massive overhead. Passing the originalweightdirectly avoids this, asvulkan_ops.linearand_vulkan_matvecare already designed to handle the original weight and cache it correctly.Empty Batch Crash: If
m == 0(which can happen in vLLM during empty iterations or warmup phases),xf[-1:]will raise anIndexError: index -1 is out of bounds. Handlingm == 0explicitly by returning an empty tensor avoids this crash and skips unnecessary Vulkan kernel launches.References