Skip to content

Conversation

GittyBurstein
Copy link

This PR adds full SYCL support for the SET operator in llama.cpp, ensures test coverage on f32/i32 tensors, and updates documentation to show ✅ for SYCL in docs/ops.md.


🔍 Summary of Work

  1. Documentation updates

    • Cleaned and deduplicated docs/ops/SYCL.csv entries for SET.
    • Ensured canonical format with support=1 and error_message="yes".
    • Regenerated docs/ops.md → now SET shows ✅ under SYCL.
  2. Operator implementation (element_wise.cpp)

    • Corrected ggml_sycl_op_set:
      • Base: dst = src0 (fast memcpy where possible).
      • Patch: overlay src1 onto dst with correct offset, nb1, nb2, nb3.
      • Works for both f32 and i32.
    • Replaced additive logic with true "set" semantics.
  3. SYCL backend support (ggml-sycl.cpp)

    • Enabled GGML_OP_SET for GGML_TYPE_I32 in ggml_backend_sycl_device_supports_op.
  4. Testing

    • All test variants (f32/i32, dim=1..3) pass:
      ./bin/test-backend-ops test -b SYCL0 -o SET
      
    • Confirmed correctness via sentinel/NMSE checks.

🛠️ Key Fixes and Lessons

  • CSV deduplication + error_message field drives doc table ✅.
  • Handling of offset and strides (nb*) was crucial.
  • Corrected misuse of additive behavior in previous implementation.

📂 Helpful Commands (Developer Reference)

# Rebuild SYCL with tests
cmake -S . -B build_sycl -DGGML_SYCL=ON -DLLAMA_BUILD_TESTS=ON -DCMAKE_BUILD_TYPE=Release
cmake --build build_sycl -j"$(nproc)"

# Run SET tests
SYCL_DEVICE_FILTER=opencl:gpu ./build_sycl/bin/test-backend-ops test -b SYCL0 -o SET

# Regenerate docs
python3 scripts/create_ops_docs.py
awk -F'|' '/^\|[[:space:]]*Operation/||/^\|[[:space:]]*SET[[:space:]]*\|/ {print}' docs/ops.md

@github-actions github-actions bot added documentation Improvements or additions to documentation ggml changes relating to the ggml tensor library for machine learning SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language labels Sep 15, 2025
@GittyBurstein GittyBurstein force-pushed the feature/sycl-set-operator branch from 1332bcf to e654008 Compare September 16, 2025 15:01
@GittyBurstein
Copy link
Author

Hi, I’ve completed the implementation of the SET operator with SYCL support.
All checks are now green ✅. Could a maintainer please approve the workflows and review this PR?
Thanks in advance!

@GittyBurstein
Copy link
Author

@ggerganov @ggerganov/ggml-maintainers
I’d greatly appreciate your review of this PR.
It adds SYCL support for the SET operator (following the ARANGE operator that was already integrated).
The implementation includes full functionality, tests, and updated documentation.
Feedback on the implementation, code structure, or any adjustments needed to meet project standards would be very helpful. Thank you!

@ggerganov
Copy link
Member

See #16032 (review)

@GittyBurstein
Copy link
Author

@ggerganov @ggerganov/ggml-maintainers
I didn’t quite understand — what exactly needs to be split?
There’s already CPU support, so I’m not sure what additional separation is expected here.

@GittyBurstein
Copy link
Author

@NeoZhangJianyu
I’d like your review on my code.
I was advised by someone from the team to split the implementation into GPU and CPU parts.
However, since the CPU implementation already exists in the library, my code only handles the GPU side.

Could you please confirm if this approach is correct, or if you’d prefer me to restructure it differently?
A quick response would be very helpful. Thanks!

@GittyBurstein
Copy link
Author

@ggerganov @ggml-org/ggml-maintainers @NeoZhangJianyu

Hi! I would really appreciate it if you could review my code and guide me on the next steps.
I’m eager to get this contribution merged and will address any feedback quickly.
Thanks a lot!

@GittyBurstein
Copy link
Author

I refactored the SET operator implementation:

  • Separated out CPU-related logic, the SYCL version is now GPU-only
  • Moved the function into its own file (set.cpp), consistent with other SYCL operators
  • Standardized the implementation style (using q->memcpy / parallel_for, no std::memcpy fallback)

@NeoZhangJianyu – please review this change from the SYCL side
@ggerganov @ggerganov/ggml-maintainers – tagging for overall maintainers’ review

@ggerganov
Copy link
Member

@GittyBurstein Please stop spamming - this is not going to make things faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation ggml changes relating to the ggml tensor library for machine learning SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants