-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Add 'sycl' devices to the context #9691
Conversation
Hi @razdoburdin , good to see you again, and thank you for continuing the work on sycl support! Would be great if you could share some more info:
|
std::string s_device = std::regex_replace(input, std::regex{"gpu"}, DeviceSym::CUDA()); | ||
std::string s_device = input; | ||
if (!std::regex_match(s_device, std::regex("sycl(:cpu|:gpu)?(:-1|:[0-9]+)?"))) | ||
s_device = std::regex_replace(s_device, std::regex{"gpu"}, DeviceSym::CUDA()); |
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.
Shouldn't this mean
User specify any GPU on the system -> xgboost check available devices and pickup GPU device that i can execute on.
I.e. in case there is only CUDA device - it will be used
In case there is only SYCL device - it will be used
The question what to do in case both are available.. Might be warning about non deterministic selection or might be even error. Not sure how realistic this case is
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.
I guess in the future it should work like you have described. But let's postpone this fix for future.
Hi, @trivialfis
Sycl allows the same code to be executed on cpu or on gpu. Currently it has less features and performance than openMP realization. But we plan to work on it later.
I guess in the ideal case user should have ability just to install xgboost (like
xgboost/plugin/sycl looks nice, great. Would you like to have a call next week to discuss the integration stages? |
Would add couple cents. So we will not be working on 2nd until 1st would get improved. So we should keep this as an option to run sycl code on CPUs, but not recommend this to users at this point. But this might be usefull for validation/development purposes at this point On CI - we in meantime would work on some form of GPU systems availability via https://cloud.intel.com/ so code also can be tested on Intel GPUs |
That would be great! |
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.
Regarding the interface, please consider describing the semantics precisely. For instance:
- cpu: using openmp with cpu threads.
- sycl: default device selected by sycl runtime.
- sycl:cpu using sycl rt to manage threads and async evaluation model.
- sycl:cpu:0 ???
- sycl:gpu: using sycl gpu impl.
- sycl:gpu:0: first sycl gpu device.
- sycl:0 ???
- gpu: ???
@@ -74,6 +109,12 @@ struct DeviceOrd { | |||
return DeviceSym::CPU(); | |||
case DeviceOrd::kCUDA: | |||
return DeviceSym::CUDA() + (':' + std::to_string(ordinal)); | |||
case DeviceOrd::kSyclDefault: |
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.
What is "sync:{ordinal}"?
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 means using the SYCL device with index {ordinal}
. It can be CPU or GPU, depending on user's system settings.
include/xgboost/context.h
Outdated
case DeviceOrd::kSyclDefault: | ||
return DeviceSym::SYCL_default() + (':' + std::to_string(ordinal)); | ||
case DeviceOrd::kSyclCPU: | ||
return DeviceSym::SYCL_CPU() + (':' + std::to_string(ordinal)); |
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.
What is "sycl:cpu:ordinal"
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 means using CPU with the specific index for multi CPU systems.
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.
multi-socket or multi-core?
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.
multi-socket
include/xgboost/context.h
Outdated
* | ||
* @param ordinal SYCL device ordinal. | ||
*/ | ||
[[nodiscard]] constexpr static auto SYCL_default(bst_d_ordinal_t ordinal = -1) { |
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.
Please follow the function naming convention "SYCLDefault"/"SyclDefault"
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
include/xgboost/context.h
Outdated
static auto constexpr SYCL_default() { return "sycl"; } | ||
static auto constexpr SYCL_CPU() { return "sycl:cpu"; } | ||
static auto constexpr SYCL_GPU() { return "sycl:gpu"; } |
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.
So, what's the default by specification?
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.
SYCL try to run on GPU. If GPU isn't available for some reason, it launches on CPU.
I updated the PR description. |
src/context.cc
Outdated
- cuda:<device ordinal> # e.g. cuda:0 | ||
- gpu | ||
- gpu:<device ordinal> # e.g. gpu:0 | ||
- gpu:<device ordinal> # e.g. gpu:0 | ||
- sycl | ||
- sycl:<device ordinal> # e.g. sycl:0 | ||
- sycl:<cpu, gpu> | ||
- sycl:<cpu, gpu>:<device ordinal> # e.g. sycl:gpu:0 |
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.
Let's remove the error message change for now and get it back when there's a public ready feature that can be enabled. At this point, the message is only going to confuse users.
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
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.
Overall looks good to me, please keep the change internal for now.
Please fix the tidy error |
fixed |
@razdoburdin Apologies for my mistake of not catching it, could you please help fix the CI by conditioning the regex match based on https://github.com/dmlc/xgboost/actions/runs/6662511294/job/18106961526 |
got it, will fix asap |
Hi,
I want to restart the discussion about the perspectives of adding SYCL support as a plugin for XGBoost.
Supporting of new devices will require some changes in the main part of the code, especially in the context of the
Context
class. The current PR adds SYCL devices to theContex
object.Supported values of the
device
parameter are:cpu
: using OpenMP backend with CPU threads.cuda
: using cuda backend on cuda device.gpu
: the same as cuda for now.sycl
: using SYCL backend on default SYCL device.sycl:cpu
: using SYCL backend on CPU; threads and async evaluation are under control by SYCL runtime.sycl:gpu
: using SYCL backend on GPU.sycl:N
: using SYCL backend on SYCL device with indexN
, can be CPU or GPU depending on user's setup.sycl:cpu:N
: using SYCL backend on CPU with indexN
.sycl:gpu:N
: use SYCL backend on GPU with indexN
.