Conversation
Ced
reviewed
Jul 8, 2026
Ced
left a comment
Ced
left a comment
Owner
There was a problem hiding this comment.
Nice. Few minor style suff plus two larger comments:
- please review RoPE mechanism and see whether we can consolidate in a single kernel like the existing mechanism. Note the existing mechanism is pre-computing cos/sin values. Can we do it there?
- better separate the various kernels (don't fuse them too aggressively) so we can better understand the process and identify kernels that could be replaced by a function call to an optimized implementation
| // Utility variables | ||
| float* rope_cos_sin; // [context_len][head_dim] | ||
| // linear attention buffers | ||
| float* la_conv_state; // [la_layer_count][la_qkv_dim][la_kernel_size-1] |
| float* x, | ||
| size_t head_dim, | ||
| size_t n_rot, | ||
| const size_t sections[4], |
Owner
There was a problem hiding this comment.
is this "4" guaranteed to be always 4 related to t/h/w/e/ -- btw what do they mean?), or is it another parameter like mrope_section_count?
| // (void)la_norm_weight; | ||
| // (void)la_out_weight; | ||
|
|
||
| const bool use_imrope = mrope_section_count > 0 && mrope_section != NULL; |
Owner
There was a problem hiding this comment.
All RoPE management may need another pass: what can be prepared outside the main kernel and can we generalize rope_head_inplace as a single kernel with different parameters?
Worth some additional review.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Ajout de l'implémentation du modèle Qwen3.5 pour l'inférence locale. Merci de jeter un coup d'œil avant le merge, notamment aux parseurs (modifications faites avec l'aide de Claude).