-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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.
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
sycl: Add option to set the SYCL architecture for all targets #10266
Conversation
Rbiessy
commented
Nov 12, 2024
- Convert GGML_SYCL_HIP_TARGET to the more generic GGML_SYCL_ARCH option
- Document that setting GGML_SYCL_ARCH can improve the performance
- I have read the contributing guidelines
- Self-reported review complexity:
- Low
- Medium
- High
* Convert GGML_SYCL_HIP_TARGET to the more generic GGML_SYCL_ARCH option * Document that setting GGML_SYCL_ARCH can improve the performance
Hello @airMeng @NeoZhangJianyu I am working with @Alcpz on the SYCL backend. Would you be able to review this change along with the related PR #10267? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A common comment is change the name from GGML_SYCL_ARCH to GGML_SYCL_DEVICE_ARCH.
GGML_SYCL_ARCH is too simple to understand it.
docs/backend/SYCL.md
Outdated
@@ -644,6 +647,7 @@ use 1 SYCL GPUs: [0] with Max compute units:512 | |||
|--------------------|---------------------------------------|---------------------------------------------| | |||
| GGML_SYCL | ON (mandatory) | Enable build with SYCL code path.<br>FP32 path - recommended for better perforemance than FP16 on quantized model| | |||
| GGML_SYCL_TARGET | INTEL *(default)* \| NVIDIA \| AMD | Set the SYCL target device type. | | |||
| GGML_SYCL_ARCH | "" | Set the SYCL target architecture, optional except for AMD. Setting the architecture can improve the performance. See the table [here](https://github.com/intel/llvm/blob/sycl/sycl/doc/design/OffloadDesign.md#--offload-arch) for a list of valid architectures. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| GGML_SYCL_ARCH | "" | Set the SYCL target architecture, optional except for AMD. Setting the architecture can improve the performance. See the table [here](https://github.com/intel/llvm/blob/sycl/sycl/doc/design/OffloadDesign.md#--offload-arch) for a list of valid architectures. | | |
| GGML_SYCL_DEVICE_ARCH | Optional (except for AMD) | Set the SYCL device architecture, optional except for AMD. Setting the device architecture can improve the performance. See the table [--offload-arch](https://github.com/intel/llvm/blob/sycl/sycl/doc/design/OffloadDesign.md#--offload-arch) for a list of valid architectures. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that's done in 5430726
ggml/CMakeLists.txt
Outdated
@@ -166,6 +166,7 @@ option(GGML_SYCL "ggml: use SYCL" | |||
option(GGML_SYCL_F16 "ggml: use 16 bit floats for sycl calculations" OFF) | |||
set (GGML_SYCL_TARGET "INTEL" CACHE STRING | |||
"ggml: sycl target device") | |||
set (GGML_SYCL_ARCH "" CACHE STRING "ggml: sycl architecture") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set (GGML_SYCL_ARCH "" CACHE STRING "ggml: sycl architecture") | |
set (GGML_SYCL_DEVICE_ARCH "" CACHE STRING "ggml: sycl device architecture") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 5430726
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's good to create this new compiler parameter.
It could be used for AOT building to speed up startup in the future.
Thank you!
Hello @airMeng can you confirm you are fine with merging this PR? Your review is blocking the PR at the moment, thanks. |
…anov#10266) * Add option to set the SYCL architecture for all targets * Convert GGML_SYCL_HIP_TARGET to the more generic GGML_SYCL_ARCH option * Document that setting GGML_SYCL_ARCH can improve the performance