Skip to content

Pad FP8 Marlin weights to valid thread tiles - #43910

Closed
mikekg wants to merge 3 commits into
vllm-project:mainfrom
mikekg:pad-fp8-marlin-tiles
Closed

mikekg wants to merge 3 commits into
vllm-project:mainfrom
mikekg:pad-fp8-marlin-tiles

Conversation

@mikekg

@mikekg mikekg commented May 28, 2026

Copy link
Copy Markdown
Contributor

Purpose

FP8 and MXFP8 Marlin repack and GEMM require rank-local N/K extents that
match a supported Marlin thread-tile family.

A fixed padding rule can be valid but overpad some partitions. This change
selects the lower-overhead padded rank-local extent from the two Marlin tile
families that are valid for both small and large batches:

  • N multiple 128 with K multiple 64
  • N multiple 64 with K multiple 128

The padding is applied after tensor-parallel partitioning, so global
checkpoint tensors are not expanded before TP slicing.

Changes

  • Pad FP8 and MXFP8 packed weights before Marlin repack.
  • Pad block/group scales consistently with the padded K extent.
  • Pad bias before Marlin bias permutation when bias is present.
  • Store the padded N/K extents on the layer.
  • Use those padded extents for Marlin GEMM.
  • Zero-pad activation K for the GEMM call.
  • Slice padded output columns back to the logical output extent.

Test Plan

Run Nemotron 3 Super 120B NVFP4 with TP4 on A100. This exercises the
dense FP8 Marlin path that failed in gptq_marlin_repack().

Repro vllm serve command:

  vllm serve nvidia/NVIDIA-Nemotron-3-Super-120B-A12B-NVFP4 \
    --quantization modelopt_mixed \
    --tensor-parallel-size 4 \
    --trust-remote-code \
    --max-model-len 4096 \
    --kv-cache-dtype fp8_e4m3

Test Result

Before this change, the TP4 A100 run failed during dense FP8 Marlin repack:

Selected MarlinFP8ScaledMMLinearKernel
RuntimeError: size_n = 4640 is not divisible by tile_n_size = 64

With this change, the dense FP8 Marlin repack failure no longer occurs. The
same run progressed past the FP8 dense path and then exposed a separate NVFP4
MoE Marlin padding issue:

RuntimeError: size_n = 672 is not divisible by tile_n_size = 64

That later failure is in the NVFP4 MoE path and is separate from the dense
FP8 Marlin padding fixed here. It is addressed by #43908.

After applying both the present PR and #43908, the TP4 NVFP4 serve sanity run completed successfully with Nemotron 3 Super:

TEST=nemo3super
MODEL_REPO=nvidia/NVIDIA-Nemotron-3-Super-120B-A12B-NVFP4
TP=4
QUANTIZATION=modelopt
server ready after 245s
...
Job Complete - Exit code: 0

This also requires #42610 to pass FP4 weights between Marlin operators when torch.compile() creates compiled execution paths.


Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.

@vadiklyutiy vadiklyutiy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's worth to add unit tests

Comment thread vllm/model_executor/layers/quantization/utils/marlin_utils_fp8.py Outdated
Comment thread vllm/model_executor/layers/quantization/utils/marlin_utils_fp8.py Outdated
@mikekg
mikekg force-pushed the pad-fp8-marlin-tiles branch 2 times, most recently from 124370a to c4f2c71 Compare June 5, 2026 05:56
@mikekg mikekg mentioned this pull request Jun 5, 2026
@mikekg
mikekg force-pushed the pad-fp8-marlin-tiles branch from c4f2c71 to 8ccc0e0 Compare June 6, 2026 02:42
@mikekg

mikekg commented Jun 6, 2026

Copy link
Copy Markdown
Contributor Author

@vadiklyutiy

It's worth to add unit tests

Done. Tested in tests/model_executor/test_fp8_marlin_padding.py
BTW, because I don't have 4 merged PRs yet, I can't run CI/CD on here, and need PRs tagged as "ready" to run ci/cd.

@mikekg
mikekg force-pushed the pad-fp8-marlin-tiles branch from 8ccc0e0 to 7435ce9 Compare June 8, 2026 20:30
@vadiklyutiy

Copy link
Copy Markdown
Member

@claude review

@vadiklyutiy vadiklyutiy added the ready ONLY add when PR is ready to merge/full CI is needed label Jun 8, 2026

@vadiklyutiy vadiklyutiy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@mergify

mergify Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Hi @mikekg, the pre-commit checks have failed. Please run:

uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-files

Then, commit the changes and push to your branch.

For future commits, pre-commit will run automatically on changed files before each commit.

@mikekg

mikekg commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Hi @mikekg, the pre-commit checks have failed. Please run:

Thank you for flagging this. After rebasing, the files changed by this PR pass pre-commit; the remaining pre-commit run --all-files failure is in .buildkite/scripts/ci-bake-rocm.sh, outside this PR’s diff.

The failing line appears to have been introduced on main by PR #36949 (87954eb50, [ROCm][CI] Optimize ROCm Docker build...). I opened a separate fix attempt here: #44937.

How would you like me to proceed? My preference is to keep this PR focused and handle that unrelated pre-commit failure separately. The CI failures I inspected also appear unrelated to this PR’s diff.

@mikekg
mikekg force-pushed the pad-fp8-marlin-tiles branch 7 times, most recently from d406660 to f0f0574 Compare June 10, 2026 23:04
@mergify

mergify Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Hi @mikekg, the pre-commit checks have failed. Please run:

uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-files

Then, commit the changes and push to your branch.

For future commits, pre-commit will run automatically on changed files before each commit.

mikekg added 2 commits June 10, 2026 20:37
FP8 and MXFP8 Marlin repack and GEMM require rank-local N/K extents that match a supported Marlin thread tile family. Padding everything to N64/K128 is valid but can overpad cases where the N128/K64 family is sufficient.

Select the lower-overhead rank-local padded size from the two non-dominated tile families that are valid for both small and large batches: N multiple 128 with K multiple 64, or N multiple 64 with K multiple 128. Include the quantization group size in the K multiple when needed so block-scale layouts remain consistent.

Use the selected extents consistently when repacking weights and scales and when launching the Marlin GEMM. Pad the activation K dimension with zeros, slice padded output columns after GEMM, and pad bias before Marlin bias permutation when bias is present.

This padding is applied after tensor-parallel partitioning via output_size_per_partition and input_size_per_partition, so global checkpoint tensors are not expanded before TP slicing.

Signed-off-by: Michael Gschwind <mgschwind@nvidia.com>

Signed-off-by: Mike G <180722391+mikekg@users.noreply.github.com>
Signed-off-by: Michael Gschwind <mgschwind@nvidia.com>

Signed-off-by: Mike G <180722391+mikekg@users.noreply.github.com>
@mergify

mergify Bot commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @mikekg.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify Bot added the needs-rebase label Jun 12, 2026
@mikekg

mikekg commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

This was merged into omnibus Marlin padding patch #45295 and the issue is now closed

@mikekg mikekg closed this Jun 12, 2026
@mikekg
mikekg deleted the pad-fp8-marlin-tiles branch June 15, 2026 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-rebase ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants