Repository navigation
Conversation
|
Hi @edenfunf, thanks for your contribution! Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:
Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below. |
|
@ggerganov If you have a chance, could you take a look at this PR? Thanks! |
JohannesGaessler
left a comment
There was a problem hiding this comment.
Sorry for the long radio silence. I took a break from llama.cpp and am currently working through my backlog.
|
|
||
| x += sample*stride_sample + channel*stride_channel + row*stride_row; | ||
| dst += ((sample*nchannels + channel)*nrows + row)*ncols; | ||
| ggml_cuda_pdl_lc(); |
There was a problem hiding this comment.
| ggml_cuda_pdl_lc(); |
The PDL launch completion does not affect correctness has to be placed essentially experimentally. If should not just be moved around like this. So I think the safest bet is to just remove it. cc @aendk
There was a problem hiding this comment.
Removed ggml_cuda_pdl_lc() from l2_norm_f32 as suggested, so it now matches master.
Also rebased onto master with #29393 and integrated the do_scale path. Full test-backend-ops passes.
9a6d350 to
052341a
Compare
Overview
Fixes #27901, refs #27911.
The CUDA norm kernels use
(nrows, nchannels, nsamples)as the launch grid, butgridDim.y/zis limited to 65535. Whenne[2]orne[3]exceeds that, the launch fails withinvalid argument.Affected kernels:
norm_f32rms_norm_f32l2_norm_f32rms_norm_mul_rope_f32The fix follows #25103 and #22944: clamp
grid.y/zto 65535 and loop over the remaining channels/samples in the kernel.Since
gridDim.ycan no longer be used as the real channel count,nchannelsandnsamplesare now passed explicitly.Multi-warp variants also need a trailing
__syncthreads()becauseblock_reducereuses the same shared buffer across loop iterations.Since the previous version
do_scalepath.ggml_cuda_pdl_lc()inl2_norm_f32based on review feedback.rms_norm_mul_ropeNMSE override with the existing one on master.[[maybe_unused]]formulc/addcin non-fused instantiations.Testing
Tested on RTX 5070, CUDA 13.3.
Added 7 cases with
ne[2]orne[3] = 65536. They fail on master and pass against the CPU backend with this patch.Full
test-backend-ops:17689 / 17689passedAlso checked additional boundary cases for 1024-thread paths,
nrows > 1, sample-axis overflow, 65535 boundary, multiple loop iterations, non-contiguous views, and fused variants.Performance
RMS_NORM 4096x512RMS_NORM 8192x1NORM 4096x512RMS_NORM [4,1,65535,1]NORM [4,1,65535,1]Normal shapes are flat or slightly faster.
The slowdown is only on the degenerate ~65k-block cases with very few active threads per block. I tried a few loop variants and none improved it, so this looks like loop overhead rather than a specific implementation detail.
A split fast/slow path could avoid that regression, but would add another kernel instantiation. For now this keeps a single loop-based path, consistent with the existing
getrows.cuapproach.Requirements