gemini-code-assist[bot] commented on code in PR #18859:
URL: https://github.com/apache/tvm/pull/18859#discussion_r2868391731
##########
python/tvm/relax/frontend/nn/llm/position_embedding.py:
##########
@@ -814,8 +814,8 @@ def fused_rope_longrope_scaling( # pylint:
disable=too-many-locals
var_position_map, (seq_len,), "int32",
elem_offset=position_map_elem_offset
)
# long factors is the first half, short factors is the second half
- long_factors = T.Buffer((rotary_dim // 2,), "float32",
data=ext_factors.data)
- short_factors = T.Buffer(
+ long_factors = T.decl_buffer((rotary_dim // 2,), "float32",
data=ext_factors.data)
+ short_factors = T.decl_buffer(
(rotary_dim // 2,),
"float32",
data=ext_factors.data,
Review Comment:
There is significant code duplication between `llama_rope_with_position_map`
(lines 442-665) and `llama4_rope_with_position_map` (lines 668-892). This
change, which is correct, has to be applied in two identical
`fused_rope_longrope_scaling` functions. This highlights a maintainability
issue.
The two parent functions are almost identical, with the main difference
being the implementation of the nested `_rope` function, specifically how the
`sin` component is calculated.
To improve maintainability and reduce redundancy, consider refactoring this
into a single base function. This base function could accept a function pointer
or a parameter to select the `sin` calculation logic. This would make the code
more modular and easier to maintain, and would prevent bugs from being fixed in
one place but not the other.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
