Skip to content

[AMD] Use HIP Top-K for GLM-5.2 MI355X / 为 GLM-5.2 MI355X 使用 HIP Top-K - #3117

Open
jiejingzhangamd wants to merge 2 commits into
mainfrom
dev/jiejing/glm5.2-hip-topk
Open

jiejingzhangamd wants to merge 2 commits into
mainfrom
dev/jiejing/glm5.2-hip-topk

Conversation

@jiejingzhangamd

@jiejingzhangamd jiejingzhangamd commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Description / 描述

English

This PR switches the GLM-5.2-MXFP4 MI355X SGLang AgentX recipe from fused Top-K v2 to the precompiled HIP Top-K path.

At concurrency 8, the supplied A/B result shows a material P90 interactivity gain with essentially unchanged throughput:

DSA backend Top-K backend P90 interactivity (tok/s/user) Throughput per GPU (tok/s)
Triton Top-K v2 96.97 9,553.2
Triton HIP Top-K 106.69 9,539.0

HIP Top-K improves P90 interactivity by 10.0% while throughput per GPU changes by -0.15%.

中文

本 PR 将 GLM-5.2-MXFP4 MI355X SGLang AgentX 配置从 fused Top-K v2 切换到预编译 HIP Top-K 路径。

并发 8 的 A/B 数据显示:HIP Top-K 的 P90 interactivity 提升 10.0%,同时单卡吞吐变化仅为 -0.15%

Type of Change / 变更类型

  • Bug fix
  • New feature
  • Configuration change
  • Documentation update
  • Other

Validation / 验证

  • bash -n benchmarks/single_node/agentic/glm5.2_fp4_mi355x_sglang_mtp.sh
  • Parsed configs/amd-master.yaml, configs/runners.yaml, and perf-changelog.yaml with PyYAML.
  • Generated the exact glm5.2-fp4-mi355x-sglang-agentic-mtp matrix and confirmed all eight AgentX points are unchanged.
  • Validated the append-only changelog contract against origin/main.
  • git diff --check
  • Full AgentX sweep and eval (CI).

Checklist / 检查清单

  • The runtime behavior change is limited to the Top-K backend environment variable. / 运行时行为变更仅限 Top-K backend 环境变量。
  • No image, topology, concurrency, HiCache, or launch-argument changes are included. / 未修改镜像、拓扑、并发、HiCache 或其他启动参数。
  • A new changelog entry is appended without modifying historical entries. / 已追加新的 changelog 记录,未修改历史记录。

Note

Low Risk
Benchmark-only runtime env toggle and changelog; no application logic, auth, or serving topology changes.

Overview
Switches the GLM-5.2 FP4 MI355X SGLang AgentX benchmark from fused Top-K v2 to the precompiled HIP Top-K path by setting SGLANG_OPT_USE_TOPK_V2=false in glm5.2_fp4_mi355x_sglang_mtp.sh. Launch topology, HiCache, concurrency matrix, and other server flags stay the same; only the Top-K backend env var and its comment are updated.

Appends a perf-changelog entry for glm5.2-fp4-mi355x-sglang-agentic-mtp / agentic-coding documenting the tradeoff: at concurrency 8, P90 interactivity +10% (96.97 → 106.69 tok/s/user) with throughput per GPU essentially flat (-0.15%).

Reviewed by Cursor Bugbot for commit aac8318. Bugbot is set up for automated code reviews on this repo. Configure here.

关闭 SGLANG_OPT_USE_TOPK_V2,使 GLM-5.2 在 MI355X 上使用预编译 HIP Top-K 路径。
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase As a PR reviewer and CODEOWNER, I have reviewed this and have.

For PR verification, add the full-sweep-fail-fast label (strongly recommended) to this PR — the benchmark sweep only runs on labeled PRs. Use full-sweep-enabled only if you need matrix jobs to keep running past a failure.

PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs


感谢你的贡献!请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md,然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人,你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板,包括保留英文语句 As a PR reviewer and CODEOWNER, I have reviewed this and have

如需进行 PR 验证,请为此 PR 添加 full-sweep-fail-fast 标签(强烈推荐)— 基准测试 sweep 仅在带有标签的 PR 上运行。仅当需要矩阵任务在失败后继续运行时才使用 full-sweep-enabled

PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动(flake),重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

Thanks for the contribution! Please reach out to respective companies' CODEOWNER to fill in the latest PR_REVIEW_CHECKLIST.md before pinging core maintainer on Slack for review. In order for the signoff PR check bot to trigger, you must follow the PR_REVIEW_CHECKLIST.md template correctly, including the phrase As a PR reviewer and CODEOWNER, I have reviewed this and have.

For PR verification, add the full-sweep-fail-fast label (strongly recommended) to this PR — the benchmark sweep only runs on labeled PRs. Use full-sweep-enabled only if you need matrix jobs to keep running past a failure.

PR authors are responsible for ensuring that after merging, all GitHub Action jobs fully pass. A lot of the time, failures are just flakes and simply re-running the failed jobs will fix it. See GitHub's docs on re-running failed jobs


感谢你的贡献!请联系相应公司的 CODEOWNER 填写最新的 PR_REVIEW_CHECKLIST.md,然后再在 Slack 上联系核心维护者进行审阅。为了触发 signoff PR 检查机器人,你必须正确遵循 PR_REVIEW_CHECKLIST.md 模板,包括保留英文语句 As a PR reviewer and CODEOWNER, I have reviewed this and have

如需进行 PR 验证,请为此 PR 添加 full-sweep-fail-fast 标签(强烈推荐)— 基准测试 sweep 仅在带有标签的 PR 上运行。仅当需要矩阵任务在失败后继续运行时才使用 full-sweep-enabled

PR 作者有责任确保合并后所有 GitHub Action 任务完全通过。 很多时候失败只是偶发抖动(flake),重新运行失败的任务即可解决。参见 GitHub 关于重新运行失败任务的文档

将 GLM-5.2 HIP Top-K 性能记录关联到 PR #3117

@claude claude Bot 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.

Looks good, straightforward config change. Reviewed the env-var flip (SGLANG_OPT_USE_TOPK_V2=truefalse) and its comment update in the benchmark script, and the new bilingual perf-changelog.yaml entry appended at the tail with matching numbers and a valid config-key/scenario-type. No other launch args, topology, or matrix points are touched.

Extended reasoning...

Overview

This PR touches exactly two files: benchmarks/single_node/agentic/glm5.2_fp4_mi355x_sglang_mtp.sh, where a single environment variable SGLANG_OPT_USE_TOPK_V2 flips from true to false (switching GLM-5.2 on MI355X from the fused Top-K v2 path to the precompiled HIP Top-K path) with an updated explanatory comment, and perf-changelog.yaml, where one new bilingual entry is appended at the physical tail of the file documenting the change with concrete before/after numbers. I confirmed via git diff that this is the entirety of the change — no image, topology, concurrency matrix, or HiCache settings are altered, and the changelog entry is appended (not editing any historical entry, and not byte-shifting earlier content).

Security risks

None. This is a benchmark configuration/environment-variable change and a documentation-style changelog append. There is no auth, crypto, permissions, injection, or data-exposure surface involved.

Level of scrutiny

Low-to-moderate is appropriate: it's a single-line runtime behavior change plus a matching changelog entry, which is exactly the kind of small, self-contained, pattern-following change the approval guidelines describe. I verified the config-key glm5.2-fp4-mi355x-sglang-agentic-mtp exists in configs/amd-master.yaml and that agentic-coding is an established scenario-type already used throughout the changelog, so the new entry is well-formed and consistent with existing entries.

Other factors

The pr-link: TBD placeholder in the new entry initially looked like a defect (every other of 918 entries has a real PR URL), but CONTRIBUTING.md's own documented changelog template uses an equivalent placeholder (pull/XXX), confirming this is normal pre-merge practice since the PR number isn't known before the PR exists — consistent with it being ruled out by the bug hunter as a non-issue rather than reported as a finding. The bug hunter reported no bugs, and the only other candidate raised (single-operating-point validation applied to the full 8-point matrix) is an inherent, previously-established pattern for this kind of kernel-backend env-var change and was already treated as ruled out rather than a live finding.

This review covers commit 34299f7, which is no longer the latest commit on this pull request; later commits are not covered by it.

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

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant