Skip to content

ci: ROCm gfx1151 fixes (draft, stacked on #26461) - #26544

Merged
CISC merged 12 commits into
ggml-org:masterfrom
AMD-Ecosystem:jimwu.rocm-ci-unified-memory
Aug 6, 2026
Merged

ci: ROCm gfx1151 fixes (draft, stacked on #26461)#26544
CISC merged 12 commits into
ggml-org:masterfrom
AMD-Ecosystem:jimwu.rocm-ci-unified-memory

Conversation

@jimw567

@jimw567 jimw567 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Overview

This branch is based on #26461's head, so the diff currently includes that PR's commits too. It adds three changes needed to get the gpu-rocm (gfx1151 / Strix Halo) self-hosted job green:

  1. CUDA: allow integrated-GPU host output buffer in debug assert — on integrated RDNA3.5 the scheduler can place a node's output on the host-visible buffer; the debug assert in ggml_cuda_graph_evaluate_and_capture rejected it, aborting ctest_debug. Extends the existing integrated && cuda_host exception (already applied to src tensors) to the node's own output buffer. Debug-only.

  2. ci: enable unified memory for the ROCm gfx1151 job — on gfx1151, GPU kernels reading mmap-loaded weights can return incorrect output (intermittent, ~0.3 NMSE vs CPU) due to a host/device coherence issue; --no-mmap, HIP_LAUNCH_BLOCKING=1, and GGML_CUDA_ENABLE_UNIFIED_MEMORY=1 all avoid it. The CI job sets GGML_CUDA_ENABLE_UNIFIED_MEMORY=1 (managed memory), which keeps mmap and restores coherence. Workaround pending a proper ROCm/HIP fix.

  3. test-llama-archs: skip jamba on HIP — jamba produces ~0.55 NMSE vs CPU on gfx1151 (a separate SSM-kernel accuracy bug, unaffected by the coherence workaround). Skipped via #ifdef GGML_USE_HIP in arch_supported(), matching the existing WebGPU per-arch carve-out, so the test still runs for the other ~40 architectures.

Additional information

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: Yes. AI helped me understand the issues and iterate different ways to fix the issue. All ideas are directed by me and solutions are reviewed and approved by me.

@github-actions github-actions Bot added testing Everything test related devops improvements to build systems and github actions ggml changes relating to the ggml tensor library for machine learning CUDA Related to the CUDA backend labels Aug 3, 2026
@ggml-gh-bot

This comment has been minimized.

@jimw567

jimw567 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

@taronaeo This PR needs approval for the workflow to start. Please help approve it.

@taronaeo

taronaeo commented Aug 4, 2026

Copy link
Copy Markdown
Member

I have just approved it :)

@taronaeo

taronaeo commented Aug 4, 2026

Copy link
Copy Markdown
Member

@jimw567 Looks like the CI is still failing, but now for a different reason. For some reason it is exiting the runner with

Process completed with exit code 20.

And I don't see any errors from the logs as well.

image

@jimw567

jimw567 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@jimw567 Looks like the CI is still failing, but now for a different reason. For some reason it is exiting the runner with

Process completed with exit code 20.

And I don't see any errors from the logs as well.

image

As a regular user I can't see any log. Is that disabled on purpose? In any case I am debugging it from the runner.

image

@taronaeo

taronaeo commented Aug 4, 2026

Copy link
Copy Markdown
Member

I can't see the logs after the job has failed too. I had to stay on the page while the runner was running to watch the logs fail. I think this is something new that GitHub did because we used to be able to see the failure.

@jimw567

jimw567 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@taronaeo I pushed two commits to this PR. The full CI has been validated locally with these two commits. The goal of this PR is to enable he gpu-rocm runner for the CI, so I worked around some CI issues. I will raise separate PRs to remove those workarounds as the work planned has nothing to do with enabling the runner. Key changes and workarounds:

  • use HIP_LAUNCH_BLOCKING for ROCm gfx1151 job to fix wrong inference output on gfx1151 (qwen3 perplexity ~88 -> 9.44).
  • skip top-k subtests on HIP backend test tests/test-backend-sampler.cpp: under as ROCm has no CUB, so TOP_K/ARGSORT is unsupported at vocab scale (bitonic argsort capped at ncols<=1024)

The new push doesn't seem to trigger a rerun of CI. Can you please approve it again? Having said that, would it be possible to whitelist my account so that my upstream PRs can trigger CI runs automatically?

@jimw567

jimw567 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

I can't see the logs after the job has failed too. I had to stay on the page while the runner was running to watch the logs fail. I think this is something new that GitHub did because we used to be able to see the failure.

That is a bummer. Luckily I have access to the runner to check all the logs.

taronaeo and others added 10 commits August 5, 2026 01:01
Signed-off-by: Aaron Teo <aaron.teo1@ibm.com>
Signed-off-by: Aaron Teo <aaron.teo1@ibm.com>
Signed-off-by: Aaron Teo <aaron.teo1@ibm.com>
Signed-off-by: Aaron Teo <aaron.teo1@ibm.com>
haha

Signed-off-by: Aaron Teo <aaron.teo1@ibm.com>
On integrated GPUs (APUs), the scheduler can legitimately place a graph
node's output on the host-visible buffer, which ggml_cuda_compute_forward
already handles. The debug assert in ggml_cuda_graph_evaluate_and_capture
required every node output to be on the device buffer, so a debug build
aborts on such a node (e.g. attn_residual ADD -> ROCm_Host on RDNA3.5).
The source-tensor assert directly below already permits this via the
integrated + cuda_host exception; apply the same exception to the node's
own output buffer. Debug-only; no effect on release/compute.

Fixes test-recurrent-state-rollback on gfx1151 (Strix Halo).
Work around a coherence issue on integrated RDNA3.5 (gfx1151) where GPU
kernels reading mmap-loaded weights can return incorrect output, which
makes test-llama-archs (and real inference) intermittently wrong.
GGML_CUDA_ENABLE_UNIFIED_MEMORY=1 uses managed memory, which restores
coherence. Remove once the underlying ROCm/HIP issue is fixed.
jamba produces incorrect output (~0.55 NMSE vs CPU) on the HIP backend on
RDNA3.5 (gfx1151); the SSM kernels need separate investigation. Skip it
for now, matching the existing per-backend carve-outs (WebGPU), so the
ROCm CI can run the test for the remaining architectures.
The gfx1151 ROCm CI job produced incorrect inference output (qwen3 perplexity ~88 vs ~9.4) due to an async-execution correctness issue in the HIP path. Serializing kernel launches with HIP_LAUNCH_BLOCKING=1 restores correctness. This replaces the earlier GGML_CUDA_ENABLE_UNIFIED_MEMORY workaround, which did not fix batched inference.
The ROCm backend does not support the TOP_K/ARGSORT op at vocab scale (no CUB; bitonic argsort is capped at ncols <= 1024), so top-k/top-p backend samplers cannot be offloaded. The penalties, set_sampler, mixed, and top_p subtests assert that offload happened, so they fail on HIP. Skip them until TOP_K is supported on the ROCm backend.
@jimw567
jimw567 force-pushed the jimwu.rocm-ci-unified-memory branch from d791ad6 to 59dd1ce Compare August 5, 2026 07:11
@taronaeo

taronaeo commented Aug 5, 2026

Copy link
Copy Markdown
Member

Its green now 🎉

@jimw567

jimw567 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Its green now 🎉

Yeah!!! I will raise a PR to your original PR so we get this runner properly added.

@jimw567

jimw567 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@taronaeo Looks like I can't raise a PR against your fork. If it's ok with you, we can merge this PR 26544 as it is. It includes all changes from your branch: taronaeo:ci/amd-rocm-hip

image

@taronaeo

taronaeo commented Aug 5, 2026

Copy link
Copy Markdown
Member

@taronaeo Looks like I can't raise a PR against your fork. If it's ok with you, we can merge this PR 26544 as it is. It includes all changes from your branch: taronaeo:ci/amd-rocm-hip

image

image

I am able to do it but it appears our baseline is different, and hence can't automatically merge.

I am OK with using this PR as the correct one now. Whenever you are ready, mark the PR as ready and I'll ping the maintainers to review this one instead :)

@jimw567
jimw567 marked this pull request as ready for review August 5, 2026 19:16
@jimw567
jimw567 requested review from a team and ggerganov as code owners August 5, 2026 19:16
@jimw567

jimw567 commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@taronaeo Thanks! I just marked the PR for reviewing.

Comment thread tests/test-backend-sampler.cpp Outdated
@taronaeo

taronaeo commented Aug 6, 2026

Copy link
Copy Markdown
Member

Btw is this failing CI of any concern?

https://github.com/ggml-org/llama.cpp/actions/runs/30984084124/job/92238741006#step:7:1

cc: @CISC for another review

Co-authored-by: Aaron Teo <taronaeo@gmail.com>
Comment thread tests/test-backend-sampler.cpp Outdated
Comment on lines +84 to +87
# HIP_LAUNCH_BLOCKING=1: workaround for an async-execution correctness
# issue on integrated RDNA3.5 (gfx1151) where batched inference returns
# incorrect output (perplexity ~88 vs ~9.4). Serializing kernel launches
# restores correctness. Remove once the underlying ROCm/HIP issue is fixed.

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.

Is the ROCm/HIP issue tracked somewhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's being tracked in our internal JIRA system (see the snapshot below).

image

Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
@CISC

CISC commented Aug 6, 2026

Copy link
Copy Markdown
Member

Btw is this failing CI of any concern?

https://github.com/ggml-org/llama.cpp/actions/runs/30984084124/job/92238741006#step:7:1

@jimw567 Please check.

@IMbackK

IMbackK commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Btw is this failing CI of any concern?

https://github.com/ggml-org/llama.cpp/actions/runs/30984084124/job/92238741006#step:7:1

cc: @CISC for another review

no its a known failure, need to add this kernel to the ignore list (and see if we can avoid the spill at a later time)

@CISC
CISC merged commit a1f96d4 into ggml-org:master Aug 6, 2026
25 of 31 checks passed
@CISC

CISC commented Aug 7, 2026

Copy link
Copy Markdown
Member

I can't see the logs after the job has failed too. I had to stay on the page while the runner was running to watch the logs fail. I think this is something new that GitHub did because we used to be able to see the failure.

This is frustrating, its value as a CI is not that great if we can't view the logs, please look into this.

@jimw567

jimw567 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

I can't see the logs after the job has failed too. I had to stay on the page while the runner was running to watch the logs fail. I think this is something new that GitHub did because we used to be able to see the failure.

This is frustrating, its value as a CI is not that great if we can't view the logs, please look into this.

Totally agree . I actually looked into this two days ago. This looks like an issue with our firewall preventing logs uploade. I am working with our IT to update firewall policy to address the issue. Having said that I did put some temp solution in the PR to keep the log around on the runner for each run. I will be more than happy to provide the log to help debug if needed.

stew675 added a commit to stew675/llama.cpp that referenced this pull request Aug 8, 2026
The v_perm_b32 asm in ggml_cuda_bf16_perm_ll/hh had its source operands
reversed on RDNA3.5, scrambling the V-row pairing in the packed BF16 PV
phase of the flash-attention tile kernel. Perplexity at 16K context went
from 7.96 to 14.60 (+83%). Swapping the operand order restores parity
with the FP32 PV default (7.9548 vs 7.9572).

Adds the perplexity validation numbers to GFX1151-BF16.md, including the
F32-KV reference (FA off, 7.9486) and the HIP_LAUNCH_BLOCKING=1
requirement (gfx1151 host/device coherence bug, upstream ggml-org#26544).

Assisted-by: DeepSeek-V4-Flash-0731
HarshDaniel added a commit to HarshDaniel/koboldcpp-rocm that referenced this pull request Aug 9, 2026
Backport the production gfx1151 assertion fix from llama.cpp ggml-org#26544 without importing its repository-specific CI changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CUDA Related to the CUDA backend devops improvements to build systems and github actions ggml changes relating to the ggml tensor library for machine learning testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants