-
Notifications
You must be signed in to change notification settings - Fork 769
[SYCL] Fix use_root_sync handling #17739
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
Conversation
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
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.
LGTM
@intel/llvm-gatekeepers Please merge this, thanks! |
Hi @HPS-1 , Also, @sarnex private email workflow isn't working. |
Also, should #16451 be resolved after this PR? If so, please consider linking this issue to your PR. Here's how to do so: https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue |
Hi @uditagarwal97 ! I've made my emails public, can you try merging again? Thanks! |
Linked! |
@intel/llvm-gatekeepers Can you try merge this again? Thanks! |
Address issue #16451 , where property `use_root_sync` is not processed properly. Also revised `sycl/test-e2e/GroupAlgorithm/root_group.cpp` to not use the deprecated version of `parallel_for`. (Which was previously blocked by this issue about `use_root_sync`). Also here's some explanation for the change in `handler.hpp`: This is where the previous code doesn't handle `use_root_sync` correctly: `processLaunchProperties` will be called twice, first for the property list returned by the kernel functor's `get(properties_tag)` method, and then for `Props` that is passed in as a parameter to `parallel_for`. Therefore, if the `get(properties_tag)` method specifies `use_root_sync` and `Props` is empty or doesn't contain `use_root_sync`, what will be done is: - first, the property list returned by the kernel functor's `get(properties_tag)` method get processed. And since it contains `use_root_sync`, `setKernelIsCooperative(true)` is called; - then, the property list `Props` that is passed in as a parameter to `parallel_for` get processed. And since it doesn't contain `use_root_sync` (actually for the non-deprecated variants of `parallel_for`, `Props` should always be an empty property list), `setKernelIsCooperative(**false**)` is called And thus in the end the `MKernelIsCooperative` flag will be set to false, while it actually should be true. Revising the code like this solve the problem. Also `MKernelIsCooperative` is false by default, so we don't need to worry if `setKernelIsCooperative` is not called. --------- Signed-off-by: Hu, Peisen <peisen.hu@intel.com>
Address issue #16451 , where property
use_root_sync
is not processed properly. Also revisedsycl/test-e2e/GroupAlgorithm/root_group.cpp
to not use the deprecated version ofparallel_for
. (Which was previously blocked by this issue aboutuse_root_sync
).Also here's some explanation for the change in
handler.hpp
:This is where the previous code doesn't handle
use_root_sync
correctly:processLaunchProperties
will be called twice, first for the property list returned by the kernel functor'sget(properties_tag)
method, and then forProps
that is passed in as a parameter toparallel_for
. Therefore, if theget(properties_tag)
method specifiesuse_root_sync
andProps
is empty or doesn't containuse_root_sync
, what will be done is:get(properties_tag)
method get processed. And since it containsuse_root_sync
,setKernelIsCooperative(true)
is called;Props
that is passed in as a parameter toparallel_for
get processed. And since it doesn't containuse_root_sync
(actually for the non-deprecated variants ofparallel_for
,Props
should always be an empty property list),setKernelIsCooperative(**false**)
is calledAnd thus in the end the
MKernelIsCooperative
flag will be set to false, while it actually should be true. Revising the code like this solve the problem.Also
MKernelIsCooperative
is false by default, so we don't need to worry ifsetKernelIsCooperative
is not called.