mtmd: use ggml_rope_set_offset

[ngxson]

Overview

ref #27382 and #27120

apply same optimization to mtmd

Self-note: add docs to clarify about rope_vision vs 2d rope

Requirements

• I have read and agree with the contributing guidelines
• AI usage disclosure: yes, trivial change

[ngxson]

/bot review

[ggml-gh-bot]

Automated code review

I reviewed the diff, the referenced optimization in #27382, the ggml_rope_set_offset semantics, and the rope kernel implementations (CPU/CUDA/Metal/Vulkan/OpenCL/SYCL) to verify the refactors are behavior-preserving.

Summary

The PR replaces the ggml_view + ggml_rope_ext + ggml_concat 2D-RoPE pattern in three mtmd vision encoders (clip build_rope_2d, gemma4v add_pos, minimax-m3 apply_rope) with two sequential ggml_rope_ext calls on the full tensor, using ggml_rope_set_offset to place the second rotation window at [n_dim/2, n_dim). This mirrors the optimization already applied to the main text models in #27382.

Correctness

I traced each refactor against the old view/concat behavior:

• clip.cpp build_rope_2d: first rope rotates [0, n_dim/2) with pos_a (offset 0 default); second rope rotates [n_dim/2, n_dim) with pos_b and freq_scale_odd, offset set to n_dim/2. The n_dims = n_dim/2 trick that makes inv_freq pick up even indices still holds, and freq_scale_odd is applied identically. Mode is NORMAL (0), so adjacent-pair rotation within each window matches the old per-view behavior. Correct.
• gemma4v add_pos: both halves use freq_scale = 1.0 and NEOX mode; only the position tensor differs (pos_x vs pos_y). New code preserves that. Correct.
• minimax-m3 apply_rope: layout [t, h, w, pad] with axd-wide rotated windows. First rope (offset axd) rotates h with pos_h; second rope (offset 2*axd) rotates w with pos_w; t and pad stay unrotated. The GGML_ASSERT(3 * axd <= dh) is retained. Correct.

I confirmed the CPU rope kernel (and the other backends) copy unrotated channels through from src rather than zeroing them (the "fill the remain channels" loop in ggml-cpu/ops.cpp), so passing the full tensor and chaining two rope ops on disjoint dim ranges is equivalent to the old concat. The ggml_rope_set_offset assertion mode != GGML_ROPE_TYPE_VISION is satisfied everywhere (modes used are 0 and NEOX=2).

No blocking findings.

Non-blocking notes

• (point 1) Backend support for n_offs != 0 is not universal. cann and et return false from supports_op for RoPE when op_params[15] != 0 (falling back to CPU), and cann on ASCEND_310P also rejects n_dims != ne[0], which the new code now triggers (the old view trick happened to satisfy ne[0] == n_dims). This is consistent with model: use ggml_rope_set_offset() #27382's accepted direction, but for these vision encoders specifically it can mean a CPU fallback on Ascend/ET where the old code ran on-backend. Worth being aware of; not a reason to change the approach.

• (point 2) Trivial nit: in gemma4v.cpp a blank line was added before return cur; inside the add_pos lambda, which the old code did not have. Optional to drop for consistency with the surrounding style.

• (point 3) The remaining function-level comment in clip.cpp ("implementation of the 2D RoPE without adding a new op in ggml") is still accurate; the removed efficiency/TODO comment was correctly dropped since the implementation it described no longer applies.

The change is minimal, single-purpose, and follows the established ggml_rope_set_offset idiom already in use across src/models/. Looks good to merge from a static-review standpoint.

This review was generated automatically by pi coding agent using zai-org/GLM-5.2. It may contain mistakes. Maintainers make the final call.