Skip to content

llama : meta split state for combined gate+up ffn_up (phi3)#24936

Open
krystophny wants to merge 2 commits into
ggml-org:masterfrom
krystophny:fix/meta-split-phi3-ffn-up-gate
Open

llama : meta split state for combined gate+up ffn_up (phi3)#24936
krystophny wants to merge 2 commits into
ggml-org:masterfrom
krystophny:fix/meta-split-phi3-ffn-up-gate

Conversation

@krystophny

Copy link
Copy Markdown
Contributor

Overview

phi3 packs the FFN gate and up projections into one ffn_up tensor of
width 2*n_ff. In llama_meta_device_get_split_state
(get_split_segments) this tensor reaches the default branch, which
divides the concatenated gate|up evenly across devices. Each device
then holds a slice that does not line up with its ffn_down input, and
the meta backend aborts on
GGML_ASSERT(split_states_equal(src_ss[0], src_ss[1])).

Splitting the tensor into two n_ff groups gives every device a
matching gate and up half, the layout the fused ffn_gate_up path
already produces. The guard ne[axis] == 2*n_ff(il) leaves models with
separate gate and up tensors (each n_ff) on the default branch.

Additional information

Reproduced on clean master (dec5ca557).

Before:

|  phi3 | Meta | Dense | ggml/src/ggml-backend-meta.cpp:590:
  GGML_ASSERT(split_states_equal(src_ss[0], src_ss[1])) failed
0% tests passed, 1 tests failed out of 1
    25 - test-llama-archs (Subprocess aborted)

After:

1/1 Test #25: test-llama-archs ...Passed
100% tests passed, 0 tests failed out of 1

This sits in the tensor-parallel / meta-backend line (#22489, #22616,
#23525, #24180).

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES. The five-line split case originates in my
    earlier work; AI assisted with reproducing the phi3 abort, building,
    and running test-llama-archs. I reviewed every line, own the change,
    and can explain it.

Architectures that pack gate and up into a single ffn_up tensor of
width 2*n_ff (phi3) were split naively across devices, so the
ffn_down source split state did not match and the meta backend
aborted on GGML_ASSERT(split_states_equal(...)). Split such tensors
into two n_ff groups, mirroring the fused ffn_gate_up handling.

Fixes the test-llama-archs abort on phi3 with the meta (TP) backend.
@krystophny krystophny requested a review from CISC as a code owner June 23, 2026 07:54
krystophny added a commit to krystophny/llama.cpp that referenced this pull request Jun 23, 2026
The synchronize-on-teardown fix (src/llama-context.cpp) and the phi3
meta split-state fix (src/llama-model.cpp) entered this branch via an
upstream merge and are unrelated to the Responses API work. They are
now filed independently as ggml-org#24935 and ggml-org#24936. Remove them here so this
PR is limited to the server Responses changes.
@ggml-gh-bot

ggml-gh-bot Bot commented Jun 23, 2026

Copy link
Copy Markdown

Hi @krystophny, thanks for your contribution!

Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:

  • Multiple open PRs from a new contributor: We limit new contributors (those without a previously merged PR) to 1 open PR at a time. You currently have 3 open PRs.

Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below.

@JohannesGaessler JohannesGaessler left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me why we have different values in n_ff_exp and n_ff_arr. But only one of those values should ever be defined. So I would suggest to fetch hparams.n_ff(il) first. If and only if that value is 0, fetch n_ff_exp and assert that that value is not 0. Assert that the tensor dimension size is exactly equal to twice the nonzero value. Return 2 segments with the nonzero value.

Keep the 2*n_ff dimension check as a guard so ordinary separate
ffn_up/ffn_gate tensors fall through to the default even split. The
combined gate|up ffn_up is dense-only (phi3, modern-bert), so n_ff(il)
is the correct width; fetch it once.
@krystophny

Copy link
Copy Markdown
Contributor Author

Done in a0caeb1n_ff(il) is now fetched once. Two notes on unifying with n_ff_exp:

  • The combined ffn_up is dense-only (phi3, modern-bert), so n_ff(il) is always the right width and n_ff_exp is 0 for these. The fused MoE case is a different tensor name (ffn_gate_up_exps, the branch just below with n_ff_exp). So the two values map to two disjoint tensor classes, not one ambiguous case — a fallback would be unreachable.
  • It has to stay a guard, not an assert: pattern_ffn_up_gate_weight also matches ordinary separate ffn_up/ffn_gate at width n_ff, which must fall through to the default even split. An assert there would abort every dense model with a normal FFN.

test-llama-archs stays green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants