Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

SYCL: Reduce most of the compiler warnings #10748

Merged
merged 24 commits into from
Dec 13, 2024
Merged

SYCL: Reduce most of the compiler warnings #10748

merged 24 commits into from
Dec 13, 2024

Conversation

qnixsynapse
Copy link
Contributor

Most of the warnings are from unused variables, incorrect typecasts and empty functions. I tried to fix them in this PR.

Also added code comments wherever necessary.

SYCL backend tests passing with this change.

I think it would be better if -Werror flag is enabled in CI which can improve the quality of code.

cc: @airMeng @NeoZhangJianyu

@github-actions github-actions bot added ggml changes relating to the ggml tensor library for machine learning SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language labels Dec 10, 2024
@qnixsynapse qnixsynapse changed the title Reduce most of the compiler warnings SYCL: Reduce most of the compiler warnings Dec 10, 2024
Rbiessy
Rbiessy reviewed Dec 10, 2024
View reviewed changes
Copy link
Collaborator

@Rbiessy Rbiessy left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I overall agree with the suggested changes. I would also be in favor of adding more warning flags.

ggml/src/ggml-sycl/ggml-sycl.cpp Outdated Show resolved Hide resolved
ggml/src/ggml-sycl/mmq.cpp Outdated Show resolved Hide resolved
ggml/src/ggml-sycl/mmq.cpp Outdated Show resolved Hide resolved
ggml/src/ggml-sycl/norm.cpp Outdated Show resolved Hide resolved
ggml/src/ggml-sycl/gemm.hpp Outdated Show resolved Hide resolved
ggml/src/ggml-sycl/common.cpp Outdated Show resolved Hide resolved
@qnixsynapse qnixsynapse marked this pull request as draft December 10, 2024 11:09
Rbiessy
Rbiessy reviewed Dec 11, 2024
View reviewed changes
ggml/src/ggml-sycl/ggml-sycl.cpp Outdated Show resolved Hide resolved
ggml/src/ggml-sycl/ggml-sycl.cpp Outdated Show resolved Hide resolved
ggml/src/ggml-sycl/ggml-sycl.cpp Outdated Show resolved Hide resolved
ggml/src/ggml-sycl/wkv6.cpp Outdated Show resolved Hide resolved
ggml/src/ggml-sycl/wkv6.cpp Outdated Show resolved Hide resolved
@qnixsynapse
Copy link
Contributor Author

qnixsynapse commented Dec 12, 2024 •
edited
Loading

Everything is finish I guess. I did not switch to tensor->buffer from tensor->backend for checking backend buffer type in this PR since I have no way to test multi GPU(I did switch to it locally and is working okay for single GPU), nor I fixed any of the SYCL deprecation warnings.

SYCL's clang compiler is not liking anonymous structures inside anonymous unions in ggml-common.h which I have not fixed.

Edit: At some point, I would like to enable -Werror flag in CI mirroring the CUDA backend. Also, mmq kernels need to be fixed and enabled before we can think of adding fp16 softmax and flash attention.

@qnixsynapse qnixsynapse marked this pull request as ready for review December 12, 2024 07:08
@abhilash1910
Copy link
Collaborator

Thanks @qnixsynapse for the cleanups. LGTM,
Ping @NeoZhangJianyu @airMeng , @Rbiessy @Alcpz for a look.

Rbiessy
Rbiessy reviewed Dec 12, 2024
View reviewed changes
ggml/src/ggml-sycl/ggml-sycl.cpp Outdated Show resolved Hide resolved
ggml/src/ggml-sycl/ggml-sycl.cpp Outdated Show resolved Hide resolved
@slaren
Copy link
Collaborator

slaren commented Dec 12, 2024

I did not switch to tensor->buffer from tensor->backend for checking backend buffer type in this PR since I have no way to test multi GPU

Hopefully this can be addressed soon. tensor->backend as been marked as deprecated for a very long time, and at this point only the SYCL backend uses it. We need to remove it.

Rbiessy
Rbiessy approved these changes Dec 12, 2024
View reviewed changes
Copy link
Collaborator

@Rbiessy Rbiessy left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes

@qnixsynapse
Copy link
Contributor Author

I did not switch to tensor->buffer from tensor->backend for checking backend buffer type in this PR since I have no way to test multi GPU

Hopefully this can be addressed soon. tensor->backend as been marked as deprecated for a very long time, and at this point only the SYCL backend uses it. We need to remove it.

I know. This PR is from an end user actually. I felt bad because this backend isn't receiving the love it deserves.

@abhilash1910 abhilash1910 merged commit 83ed24a into ggerganov:master Dec 13, 2024
47 checks passed
@qnixsynapse qnixsynapse deleted the refactor branch December 13, 2024 09:19
arthw pushed a commit to arthw/llama.cpp that referenced this pull request Dec 20, 2024
@qnixsynapse @abhilash1910 @arthw
SYCL: Reduce most of the compiler warnings (ggerganov#10748)
a674ffb 
* Try to reduce some unused and typecast warnings

* Reduce compiler warnings step 2

* add a newline at the end of the file

* Initialize nreduce as size_t

* [SYCL] Remove pragma directives from mmq.cpp

* SYCL: mmq add condition to prevent blocks_per_tile_x_row variable from becoming 0

* SYCL softmax: Initialize nreduce as size_t

* ggml-sycl.cpp: fix some trailing whitespaces

* SYCL: remove the unused variables instead of commenting it out

* SYCL poo2d kernel: set NAN for invalid pooling op

* SYCL gemm.hpp: remove pragma directives

* SYCL gemm.hpp: use const cast to properly support dnnl::memory

* SYCL: wkv6 remove a comment

* SYCL: clean comments step 2

* SYCL: clean comments and variables step 3

* SYCL: Use GGML_UNUSED for unused variables

* SYCL: remove extra empty lines and a comment

* Remove TODO

* cleanup spaces

* add a stdout for unsupported op

* use sycl printf over fprintf

* remove prints for CI

* SYCL ggml-sycl: pool2D use sycl::nan and remove if-else block

---------

Co-authored-by: Abhilash Majumder <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NeoZhangJianyu NeoZhangJianyu NeoZhangJianyu left review comments

@airMeng airMeng airMeng left review comments

@abhilash1910 abhilash1910 abhilash1910 left review comments

@Rbiessy Rbiessy Rbiessy approved these changes

Assignees
No one assigned
Labels
ggml changes relating to the ggml tensor library for machine learning SYCL https://en.wikipedia.org/wiki/SYCL - GPU programming language
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@qnixsynapse @abhilash1910 @slaren @Rbiessy @airMeng @NeoZhangJianyu