Skip to content

sycl : fix test-backend-ops CI break && restore Kronecker product FWHT support (#28016) - #28254

Merged
Titaniumtown merged 3 commits into
ggml-org:masterfrom
philip-jingxin:fix/sycl-fwht-kronecker-ci
Sep 5, 2026
Merged

Titaniumtown merged 3 commits into
ggml-org:masterfrom
philip-jingxin:fix/sycl-fwht-kronecker-ci

Conversation

@philip-jingxin

Copy link
Copy Markdown
Contributor

Overview

Reintroduces the sycl Kronecker product FWHT support originally merged in #28016 (and reverted in #28184 due to CI build failure).

The failure was due to tests/test-backend-ops.cpp containing an unused variable warning (unused variable 'M'), becoming an error with LLAMA_FATAL_WARNINGS=ON

Additional information

  • Tested with the following commands
cmake -B build_error   -DGGML_SYCL=ON   -DCMAKE_C_COMPILER=icx   -DCMAKE_CXX_COMPILER=icpx   -DCMAKE_BUILD_TYPE=Debug   -DLLAMA_FATAL_WARNINGS=ON   -DLLAMA_ALL_WARNINGS=ON   -DLLAMA_BUILD_TESTS=ON
cmake --build build_error --target test-backend-ops -j

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: YES - Gemini was used to generate the build options

@philip-jingxin
philip-jingxin requested review from a team and ggerganov as code owners September 2, 2026 16:07
@github-actions github-actions Bot added testing Everything test related ggml changes relating to the ggml tensor library for machine learning SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language labels Sep 2, 2026
@Titaniumtown

Copy link
Copy Markdown
Contributor

Running the workflows this time :p xd

@Titaniumtown

Copy link
Copy Markdown
Contributor

@philip-jingxin can you fix the trailing whitespace at the end of tests/test-backend-ops.cpp. Thanks!

@arthw arthw 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.

It's good job!
I test it on Arc770 and it's passed.

Thank you!

@arthw

arthw commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

@philip-jingxin
There is code format issue shown in CI task: EditorConfig Checker / editorconfig (pull_request)Failing after 27s.

Could you fix them?

Thank you!

@philip-jingxin

philip-jingxin commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor Author

@philip-jingxin There is code format issue shown in CI task: EditorConfig Checker / editorconfig (pull_request)Failing after 27s.

Could you fix them?

Thank you!

Hi @arthw I just fixed the bug. Thanks for reviewing!

@philip-jingxin

Copy link
Copy Markdown
Contributor Author

@philip-jingxin can you fix the trailing whitespace at the end of tests/test-backend-ops.cpp. Thanks!

@Titaniumtown Sorry for the delay, was quite busy during the day. I've cleaned up the trailing space. Thanks for pointing it out!

@Titaniumtown

Copy link
Copy Markdown
Contributor

Thank you for following through and fixing it!

@Titaniumtown
Titaniumtown merged commit 4d91760 into ggml-org:master Sep 5, 2026
23 of 29 checks passed
@philip-jingxin

Copy link
Copy Markdown
Contributor Author

Thank you for following through and fixing it!

@Titaniumtown Thanks! Happy to contribute!

@ggerganov ggerganov 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.

@Titaniumtown For changes in non-SYCL code like here in test-backend-ops.cpp wait for an additional review from another maintainer before merging.

Comment on lines +4768 to +4773
#ifdef GGML_USE_SYCL
const bool is_kronecker =
((n_cols % 12 == 0) && is_pow2(n_cols / 12)) || ((n_cols % 20 == 0) && is_pow2(n_cols / 20));
#else
const bool is_kronecker = false;
#endif

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.

Special-casing the backends like this is not allowed - the user logic should not depend on what ggml backend has been compiled. Please follow-up with a PR that removes all #ifdef GGML_USE_SYCL directives from test-backend-ops.cpp.

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.

Hi @ggerganov, just followed up in #28490. Removed the test special-casing and implemented Kronecker operations for CPU.

@Titaniumtown

Copy link
Copy Markdown
Contributor

@ggerganov Alright. sorry. I will make sure to wait on you next time.

x1250 pushed a commit to x1250/llama.cpp that referenced this pull request Sep 9, 2026
…T support (ggml-org#28016) (ggml-org#28254)

* Reapply "sycl : add Kronecker product FWHT support for sizes 384, 640, 768, 12…" (ggml-org#28184)

This reverts commit c845263.

* tests : fix unused variable M in test-backend-ops

* tests: fix trailing space error and isolate kronecker tests for sycl backend only
zbrad pushed a commit to zbrad/llama.cpp that referenced this pull request Sep 10, 2026
…T support (ggml-org#28016) (ggml-org#28254)

* Reapply "sycl : add Kronecker product FWHT support for sizes 384, 640, 768, 12…" (ggml-org#28184)

This reverts commit 79dc68c.

* tests : fix unused variable M in test-backend-ops

* tests: fix trailing space error and isolate kronecker tests for sycl backend only
@BrewTestBot BrewTestBot mentioned this pull request Sep 14, 2026
1 task done
pl752 pushed a commit to pl752/llama.cpp that referenced this pull request Sep 15, 2026
…T support (ggml-org#28016) (ggml-org#28254)

* Reapply "sycl : add Kronecker product FWHT support for sizes 384, 640, 768, 12…" (ggml-org#28184)

This reverts commit c845263.

* tests : fix unused variable M in test-backend-ops

* tests: fix trailing space error and isolate kronecker tests for sycl backend only
zsogitbe pushed a commit to zsogitbe/llama.cpp that referenced this pull request Sep 17, 2026
…T support (ggml-org#28016) (ggml-org#28254)

* Reapply "sycl : add Kronecker product FWHT support for sizes 384, 640, 768, 12…" (ggml-org#28184)

This reverts commit c845263.

* tests : fix unused variable M in test-backend-ops

* tests: fix trailing space error and isolate kronecker tests for sycl backend only
bri-prism added a commit to PrismML-Eng/llama.cpp that referenced this pull request Oct 2, 2026
…gml-org#28254, ggml-org#29243) (#302)

* sycl : fix test-backend-ops CI break && restore Kronecker product FWHT support (ggml-org#28016) (ggml-org#28254)

* Reapply "sycl : add Kronecker product FWHT support for sizes 384, 640, 768, 12…" (ggml-org#28184)

This reverts commit c845263.

* tests : fix unused variable M in test-backend-ops

* tests: fix trailing space error and isolate kronecker tests for sycl backend only

(cherry picked from commit 4d91760)

* sycl: FWHT kernels for block widths above 512 (ggml-org#29243)

The SYCL FWHT covers 64 to 512 via the standard butterfly network, plus
384/640/768/1280 via the Kronecker/Paley construction added separately in
Hadamard hint can produce (1024, 2048, 4096, 8192); those still fall through
to the default case and run as a dense GEMM against the materialized
rotation tensor, correct but O(n^2) instead of O(n log n).

fwht_kernel_wide runs one row per work-group instead of per sub-group, so
each work-item keeps N/NT values rather than N/WARP_SIZE. Butterflies below
the sub-group width still shuffle; those up to the work-group width go
through work-group local memory; the rest stay in registers. Same butterfly
and sign convention as the existing narrow kernel.

ggml's SYCL backend registration (dpct::dev_mgr) unconditionally requires a
GPU-labeled platform to exist and throws before any op-level test can run,
so test-backend-ops could not be exercised on this box (a GPU-less pod) even
via the CPU device. Verified instead with a standalone harness: the same
kernel body run through a real SYCL CPU device (Intel oneAPI DPC++ 2026.1,
OpenCL CPU backend), checked against an independent recursive-doubling
Hadamard reference, cross-validated by first running the existing unmodified
narrow kernel through the identical harness and confirming it passes (rules
out a reference-convention bug before trusting a pass on the new code).
Random-input results for all four widths, single- and multi-row:

  N=1024 NT=256 rows=1  max_abs_err=1.7e-07  max_rel_err=4.9e-04  PASS
  N=2048 NT=256 rows=1  max_abs_err=1.9e-07  max_rel_err=2.0e-04  PASS
  N=4096 NT=256 rows=1  max_abs_err=2.0e-07  max_rel_err=1.4e-04  PASS
  N=8192 NT=256 rows=1  max_abs_err=2.5e-07  max_rel_err=3.8e-03  PASS
  N=1024 NT=256 rows=7  max_abs_err=2.4e-07  max_rel_err=1.0e-03  PASS
  N=2048 NT=256 rows=5  max_abs_err=3.0e-07  max_rel_err=9.4e-04  PASS
  N=4096 NT=256 rows=3  max_abs_err=2.7e-07  max_rel_err=1.7e-03  PASS
  N=8192 NT=256 rows=2  max_abs_err=2.5e-07  max_rel_err=1.9e-03  PASS

This covers the kernel algorithm itself; it does not exercise the ggml
dispatch/supports_op integration end to end, which needs a real GPU (or a
SYCL GPU plugin) to get past backend registration. test-backend-ops build
is verified: fwht.cpp recompiles with zero warnings as part of ggml-sycl.

(cherry picked from commit c829670)

---------

Co-authored-by: Jingxin (Philip) Li <philipaslee@gmail.com>
frostyautumnleaf pushed a commit to frostyautumnleaf/llama.cpp that referenced this pull request Oct 5, 2026
…T support (ggml-org#28016) (ggml-org#28254)

* Reapply "sycl : add Kronecker product FWHT support for sizes 384, 640, 768, 12…" (ggml-org#28184)

This reverts commit c845263.

* tests : fix unused variable M in test-backend-ops

* tests: fix trailing space error and isolate kronecker tests for sycl backend only
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ggml changes relating to the ggml tensor library for machine learning SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants