sycl, hip: fix two pre-existing build breaks in fork-added code - #293
sycl, hip: fix two pre-existing build breaks in fork-added code#293TheTom wants to merge 3 commits into
Conversation
SYCL: the SET_ROWS case of ggml_backend_sycl_device_supports_op ends in an unconditional return, leaving upstream's trailing 'return res;' orphaned. 'res' is not declared in that scope, so the SYCL backend fails to compile: ggml-sycl.cpp:5817:24: error: use of undeclared identifier 'res' The dead statement is simply removed. HIP: ~hip_f16_alloc() calls cudaStreamSynchronize and cudaFree and discards their results. Under HIP these are [[nodiscard]], which is fatal with -Werror in the quality-check job: fattn-common.cuh:1409: error: ignoring return value of type 'hipError_t' declared with 'nodiscard' attribute [-Werror,-Wunused-value] A destructor cannot propagate the error, so the results are discarded explicitly with a (void) cast. Both files are fork-added code and both breaks predate #287; they are visible on any commit that builds the SYCL or HIP backends.
|
CI result on this branch, with a finding that is more interesting than the fix. HIP fix works, and it unmasked something
So the nodiscard fix did its job. What it revealed is that this quality gate has, as far as I can tell, never actually run on this fork, because the build died before reaching it. What it reports: Reading the type ids: 47 = TURBO4_0, 44 = TURBO3_0, 43 = TURBO2_0, 8 = Q8_0, 1 = F16. The first template parameter is the head size, and it is 256 in every flagged flash-attention case. The parenthesised pair is architectural plus spilled. So these kernels are spilling 295 to 330 registers, roughly doubling their register footprint. That is not a marginal overrun, it is a level of spilling that would wreck occupancy and make turbo flash attention very slow on CDNA/GCN. This is not caused by my change. Adding Possible connection to #252 / #253I want to flag this without overclaiming, because it is circumstantial rather than causal. @jasstrong reported turbo4 V-cache corruption on gfx1100 with head dim 256. Every flash-attention kernel flagged here is at head size 256 with turbo types. That is the same corner of the space. Spilling normally costs performance rather than correctness, so this is not by itself an explanation for garbled output, and the check runs against gfx908 (CDNA) rather than his gfx1100 (RDNA3). But "the turbo FA kernels at hsk=256 are in serious trouble on AMD" is now a measured fact rather than a hypothesis, and it is the same neighborhood #253 has been stuck in. Worth a look from anyone with AMD hardware. It also raises the stakes on my #279, still open, which adds the first hsk=256 turbo FA test coverage the suite has ever had. If those kernels are this stressed at 256, the shapes deserve exercising. The SYCL half is still unprovenThe three SYCL jobs no longer fail at the compiler either, but not because they got further in a good way. They now die here: which is a dependency install failure before any of our code is touched. So my SYCL fix remains unvalidated. I am not going to claim it works on the strength of a job that never reached the compiler. If someone can re-run once the runner image is healthy, or has an Intel GPU handy, that would settle it. Suggested dispositionThe two fixes in this PR are narrow and I think they stand on their own: one removes an unreachable statement referencing an undeclared identifier, the other discards two return values in a destructor that cannot act on them. Neither can change behavior. The VGPR spilling is a separate and much larger problem that I will file on its own rather than expand this PR into it. |
000eeff to
fdaa94b
Compare
Third build break in the same family as the other two here, and it was
hidden behind the dead 'return res;' in ggml-sycl.cpp: once that is fixed
the SYCL build gets further and fails on this:
fattn-vec.hpp:108: error: declaration of 'nthreads' shadows template parameter
fattn-vec.hpp:108: error: no matching function for call to
ggml_sycl_fattn_vec_get_nthreads_device()
fattn-vec.hpp:109: error: redefinition of 'nthreads_KQ'
A rebase left two generations of the same declarations in one scope:
- line 108 declares a local 'nthreads', which both shadows the template
parameter upstream added and calls the helper with no arguments when it
takes a gpu_arch. The caller already computes the value host-side and
passes it as the template argument (nthreads_hw), so the local is dead.
- lines 91-92 keep upstream's nthreads_KQ/nthreads_V while the fork's
turbo-aware versions are declared again 17 lines later.
Drops the three superseded declarations. The turbo-aware versions reduce to
upstream's exact expressions when neither K nor V is a turbo type:
K: K_is_turbo ? 1 : (type_K == F16 ? 128/cpy_nb : nthreads_KQ_q)
V: V_is_unquantized ? (V_is_turbo ? ... : 128/cpy_nb) : nthreads_V_q
so behaviour is unchanged for every non-turbo configuration and the turbo
paths now compile at all.
NOT verified locally: there is no Intel GPU or SYCL toolchain here, so this
rests on reading the code plus the CI error text. CI is the test.
2 participants
Two independent build breaks in fork-added code. Neither is caused by the roll-up stack, both are visible on the base branch, and together they account for 4 of the 5 non-flaky red jobs on #287.
SYCL: backend does not compile
The
GGML_OP_SET_ROWScase ofggml_backend_sycl_device_supports_opwas rewritten to add the turbo cache types, and now ends in an unconditionalreturn (...). Upstream's trailingreturn res;was left behind, andresis not declared in that scope. The statement is unreachable as well as invalid, so it is simply removed.Affects
ubuntu-24-sycl (fp16),ubuntu-24-sycl (fp32)andwindows-latest-sycl.HIP: quality check fails under -Werror
~hip_f16_alloc()callscudaStreamSynchronizeandcudaFreeand discards both results. Under HIP those map tohipStreamSynchronizeandhipFree, which are[[nodiscard]]. A destructor cannot propagate the error, so the right fix is to discard explicitly with a(void)cast rather than to add error handling that cannot do anything useful.Affects
ubuntu-22-hip-quality-check.hip_f16_allocis fork code, introduced in5fd308947with the TurboQuant CUDA kernels, so upstream never saw this.Testing, and its limits
Being straightforward about what I could and could not check: I have neither an Intel GPU nor a ROCm machine, so I have not compiled either backend locally. What I did verify:
fattn-common.cuhchange, which at least exercises that header's non-HIP path.CI on this PR is the real test. If either job still fails, the fix is wrong and I would rather find that out here than after it merges.
Why separate from the stack
These predate #287 and are unrelated to the MoE cache work, so they do not belong in that stack. Landing them separately means the SYCL and HIP jobs go green independently, which makes the stack's own CI readable instead of drowning in pre-existing red.